-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
cmake: add control over inline source code disassembly #71535
cmake: add control over inline source code disassembly #71535
Conversation
5b43a09
to
97d3a27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you!
A bit more context in the commit message wouldn't hurt, maybe in the Kconfig help too. At the least the keyword "reproducible build" would be great. |
97d3a27
to
5110275
Compare
Updated. Not sure if explanation's good enough so lmk if it needs another update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
(Good opportunity for a general whine that the .lst file is now off by default. I'm constantly having to turn it back on and rebuild, and it wastes a minute or so for me almost every week. If CI can't handle the overhead we should have CI turn it off. Obviously not related to this PR, but while people's eyes are on this area...)
I timed it and it was VERY slow, 7 seconds on my beefy build server! Also: not concurrent with anything else so frequently doubling the total build time! Not just for CI but for everybody. I think very few people look at It will be interesting to measure it again without this -S. Linking source code to optimized code is really hard (and buggy as we just found) so maybe it was -S actually taking a lot of that time? Assuming it can be very fast now without -S, having it on by default again but without -S would probably not be very useful to users like you either? Default configuration is hard, really hard. You can never please all use cases. So the key is configuration flexibility and convenience with overlays etc. and I find Zephyr pretty good there.
Can't you set this "permanently" in some personal overlay? Using https://docs.zephyrproject.org/latest/develop/west/build-flash-debug.html#west-building-cmake-config |
This comment was marked as resolved.
This comment was marked as resolved.
I think I was the one turning it off. It doesn't matter for CI I guess, but for local development it sucks having to wait for objdump when you don't actually need it. I was certainly wasting more than a minute per week. It was more like 10-15s per-build and with 20+ builds every day it adds up. I think having a default overlay that you apply for local development makes sense. |
Zephyr CI builds hundreds of different configurations for each pull request, so CONFIG_OUTPUT_DISASSEMBLY does matter very much for it. Not just for time but for money too! CONFIG_OUTPUT_DISASSEMBLY could be easily overriden in Zephyr CI if needed though (not needed since you disabled it by default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code works, but would like to see the else() .. if()
transformed into a cleaner elseif()
.
CMakeLists.txt
Outdated
else() | ||
set(disassembly_type "$<TARGET_PROPERTY:bintools,disassembly_flag_inline_source>") | ||
if (CONFIG_OUTPUT_DISASSEMBLE_WITH_SOURCE) | ||
set(disassembly_type "$<TARGET_PROPERTY:bintools,disassembly_flag_inline_source>") | ||
endif() | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the extra if()
?
else() | |
set(disassembly_type "$<TARGET_PROPERTY:bintools,disassembly_flag_inline_source>") | |
if (CONFIG_OUTPUT_DISASSEMBLE_WITH_SOURCE) | |
set(disassembly_type "$<TARGET_PROPERTY:bintools,disassembly_flag_inline_source>") | |
endif() | |
endif() | |
elseif (CONFIG_OUTPUT_DISASSEMBLE_WITH_SOURCE) | |
set(disassembly_type "$<TARGET_PROPERTY:bintools,disassembly_flag_inline_source>") | |
endif() |
By default, the generated disassembly file (i.e: the .lst file) also contains inline source code. This has the potential to become problematic when attempting to compare the generated .lst files accross platforms since they may differ for various reasons. As such, add the option to control whether the disassembly file should contain inline source code or not. The need for this patch was sparked by the differences observed in the generated .lst file for Linux and Windows in one of SOF's CI jobs (details in thesofproject/sof/issues/9034), which were caused by the addition of the inline source code. With this, we can keep testing for reproducible builds while not having to deal with differences in the .lst files caused by things such as having ".." include paths. Signed-off-by: Laurentiu Mihalcea <[email protected]>
c492250
5110275
to
c492250
Compare
By default, the generated disassembly file (i.e: the .lst file) also contains inline source code. This has the potential to become problematic when attempting to compare the generated .lst files accross platforms since they may differ for various reasons. As such, add the option to control whether the disassembly file should contain inline source code or not.
Relevant SOF issue: thesofproject/sof#9034