Repository navigation
[BUG] logger "ABI" dictionary check ignores configuration and other changes #3890
Description
While comparing various build outputs I realized "by chance" why using a source content hash for the logger ABI check (#3195)
is flawed. It's quite obvious with hindsight: that's simply because the "source" that git knows about is not the source code that gets compiled. git misses at least: pre-processing, generated config.h, compiler flags and version and maybe others.
These are the same reasons why ccache hashes much more than source code and even some compiler information:
https://ccache.dev/manual/4.2.html#_how_ccache_works
EDIT: the "most obvious" reproduction is with CONFIG_TRACEV, see below.
Sample reproductions as of latest commit f20b3ee:
-
CONFIG_DEBUG changes the size of one of the .static.log.1 section in src/ipc/handler.c.o, which changes the .ldc file. Yet .ldc Source content hash does not change.
-
CONFIG_OPTIMIZE_FOR_SIZE changes the size of one of the .static.log.1 section in src/audio/asrc/asrc_farrow.c.o which changes the .ldc file. Yet .ldc Source content hash does not change.
-
CONFIG_FORMAT_S32LE changes the size of some .static.log.N sections in both src/audio/tdfb/tdfb.c.o and src/audio/eq_fir/eq_fir.c.o and probably others which changes the .ldc file. Yet .ldc Source content hash does not change.
Having an approximate .ldc check is probably better than none at all. However it misses most configuration changes right now and gives the wrong impression that everything is fine when the logger is actually broken.
I think the proper solution is to simply ask CMake to checksum the .ldc dictionary file itself because the dictionary is the thing and only thing that really matters here and then ask objcopy to inject the checksum into the final firmware file. This was already recommended in PR #3195 and rejected for confusing SMEX reasons.
Activity
slawblauciak commentedon Mar 9, 2021
This issue most likely doesn't cause any functional problems.
However, it's to be checked if the CI logger issues are not caused by this.
@marc-hb can you check?
marc-hb commentedon Mar 9, 2021
I don't remember using the logger before it had a "ABI check" but I assume using a wrong .ldc file was pretty bad otherwise no one would have spent so much effort implementing this ABI check in the first place. @ktrzcinx , others? Wrong log messages, logger crashes, others?
Now it can be:
Developers who automate the deployment of the .ldc file at the same time than the .ri firmware file are not affected either - but they were not affected before either and the ABI check did not solve a problem they had.
marc-hb commentedon Apr 14, 2021
The simplest way to reproduce this issue (and most obvious with hindsight) is to use a firmware compiled with CONFIG_TRACEV and a
.ldcdictionary file created without CONFIG_TRACEV. They have the same source checksum but the latter misses alltr_dbg()statements. In one attempt of doing sosof-logger -tfails withinvalid text lengthandsof-loggerwithout-tprints random garbage.The converse surprisingly seems to work.
marc-hb commentedon Jan 14, 2022
wontfix
version.cmake: include scripts/ and zephyr/ in the .ldc dictionary hash
version.cmake: include scripts/ and zephyr/ in the .ldc dictionary hash
[-][BUG] logger "ABI" check ignores configuration and other changes[/-][+][BUG] logger "ABI" dictionary check ignores configuration and other changes[/+]marc-hb commentedon Feb 9, 2022
COMP_BASEFW_IPC4will also be ignored and cause a collision.marc-hb commentedon Jan 11, 2023
This
git hash-objecttechnique is also creatingautocrlfissues: