Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] logger "ABI" dictionary check ignores configuration and other changes #3890

Closed
marc-hb opened this issue Mar 3, 2021 · 6 comments
Closed
Labels
bug Something isn't working as expected logger Dictionary and logger P3 Low-impact bugs or features won't fix This will not be worked on atm (e.g. a bug closed for lack of user request, hardware etc)

Comments

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 3, 2021

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.

@marc-hb marc-hb added the bug Something isn't working as expected label Mar 3, 2021
@marc-hb marc-hb added the logger Dictionary and logger label Mar 3, 2021
@slawblauciak slawblauciak added the P3 Low-impact bugs or features label Mar 9, 2021
@slawblauciak
Copy link
Collaborator

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
Copy link
Collaborator Author

marc-hb commented Mar 9, 2021

However, it's to be checked if the CI logger issues are not caused by this.

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:

  • worse because the faulty ABI check will give a false sense of security and could distract the developer away from the root cause
  • of course much better for developers who don't change their (K)config because they're not affected. How often do developers change their configuration?

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
Copy link
Collaborator Author

marc-hb commented 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 .ldc dictionary file created without CONFIG_TRACEV. They have the same source checksum but the latter misses all tr_dbg() statements. In one attempt of doing so sof-logger -t fails with invalid text length and sof-logger without -t prints random garbage.

The converse surprisingly seems to work.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 14, 2022

wontfix

@marc-hb marc-hb closed this as completed Jan 14, 2022
@marc-hb marc-hb added the won't fix This will not be worked on atm (e.g. a bug closed for lack of user request, hardware etc) label Jan 14, 2022
marc-hb added a commit to marc-hb/sof that referenced this issue Jan 14, 2022
scripts/ has kconfig defaults and CMake code that can affect the
dictionary. Note this does not fix thesofproject#3890 because .config (and maybe
others) are still not hashed but it helps a bit.

Signed-off-by: Marc Herbert <[email protected]>
lgirdwood pushed a commit that referenced this issue Jan 14, 2022
scripts/ has kconfig defaults and CMake code that can affect the
dictionary. Note this does not fix #3890 because .config (and maybe
others) are still not hashed but it helps a bit.

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb marc-hb changed the title [BUG] logger "ABI" check ignores configuration and other changes [BUG] logger "ABI" dictionary check ignores configuration and other changes Feb 9, 2022
@marc-hb
Copy link
Collaborator Author

marc-hb commented Feb 9, 2022

COMP_BASEFW_IPC4 will also be ignored and cause a collision.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 11, 2023

This git hash-object technique is also creating autocrlf issues:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected logger Dictionary and logger P3 Low-impact bugs or features won't fix This will not be worked on atm (e.g. a bug closed for lack of user request, hardware etc)
Projects
None yet
Development

No branches or pull requests

8 participants