scripts: gen_handles: Sort the device handles #48195
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaronemassey
previously requested changes
Jul 25, 2022
keith-zephyr
force-pushed
the
gen_handles-sort
branch
2 times, most recently
from
fee903f to
b79cf07
Compare
|
Note that the twister failure is unrelated to this change and is fixed by PR48280 |
keith-zephyr
force-pushed
the
gen_handles-sort
branch
from
b79cf07 to
ce5ecd0
Compare
yperess
reviewed
Jul 26, 2022
|
Just bringing your attention to #47582, which completely reworks this script. |
coreboot-bot
pushed a commit
to coreboot/chrome-ec
that referenced
this pull request
Jul 27, 2022
Add a the option "--static" to zmake configure/build. This is passed through to the generate_ec_version.py script to generate static version information for reproducible builds. Note that Zephyr builds require zephyrproject-rtos/zephyr#48195 to compare correctly. BUG=none BRANCH=none TEST=zmake testall TEST=zmake build herobrine -B /tmp/zephyr-rev1 --static TEST=zmake build herobrine -B /tmp/zephyr-rev2 --static TEST=cmp /tmp/zephyr-rev1/herobrine/output/zephyr.bin \ /tmp/zephyr-rev2/herobrine/output/zephyr.bin Signed-off-by: Keith Short <[email protected]> Change-Id: I07361f21c865e2b233ebdb2dc730d25afe13a843 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3786857 Reviewed-by: Abe Levkoy <[email protected]>
keith-zephyr
force-pushed
the
gen_handles-sort
branch
2 times, most recently
from
e0425ac to
ae29f5f
Compare
|
Rebased on top of @JordanYates changes. |
JordanYates
requested changes
Aug 2, 2022
keith-zephyr
dismissed
aaronemassey’s stale review
Comments no longer apply after rebase on latest gen_handles script.
yperess
previously approved these changes
Aug 2, 2022
keith-zephyr
force-pushed
the
gen_handles-sort
branch
from
ae29f5f to
8c9aa5a
Compare
JordanYates
reviewed
Aug 3, 2022
JordanYates
reviewed
Aug 3, 2022
JordanYates
requested changes
Aug 3, 2022
Replicate the devicetree dependencies into a sorted list. This ensures that the structures added to the .__device_handles_pass2 section are reproducible from build to build. Tested with: west build -b native_posix tests/drivers/build_all/sensor Without this change, two consecutive builds do not compare. Signed-off-by: Keith Short <[email protected]>
keith-zephyr
force-pushed
the
gen_handles-sort
branch
from
8c9aa5a to
8fd018d
Compare
yperess
approved these changes
Aug 4, 2022
JordanYates
approved these changes
Aug 4, 2022
fabiobaltieri
approved these changes
Aug 5, 2022
|
@keith-zephyr how did you find this? While debugging something else, private CI, other? |
I was manually comparing Chromebook EC builds in our downstream repo and found that just running 2 consecutive builds yielded binaries that didn't compare. I isolated the problem to the generated dev_handles.c file. We don't have a CI test in our downstream repo, but rather developers can manually run a script to compare builds. |
|
Coincidence?
|
marc-hb
added a commit
to marc-hb/zephyr
that referenced
this pull request
Nov 4, 2022
Temporary bugs, corner cases and obsolete toolchains aside, the Zephyr build is most of the time reproducible: zephyrproject-rtos#50205 and zephyrproject-rtos#14593. This means two different build machines using the same toolchain will always produce the same binary output. The one-line addition in this commit makes it trivial to verify that binary outputs are indeed the same by adding a single checksum line in the build logs: ``` [16/16] Linking C executable zephyr/zephyr.elf Memory region Used Size Region Size %age Used RAM: 53280 B 3 MB 1.69% IDT_LIST: 0 GB 2 KB 0.00% fdd2ddf2ad7d5da5bbd79b41cef...7b16ef549a8281111d8e205 zephyr.strip ``` This commit makes a non-measurable build time difference. Build reproducibility matters for (at least) two important reasons: - Security / supply chain attacks, see https://www.cisa.gov/sbom, zephyrproject-rtos#50205, https://reproducible-builds.org/ and many others. - Making sure build configurations are strictly identical when trying to reproduce elusive issues or when issuing releases. Displaying a reproducible checksum accelerates the investigation of temporary reproducibility issues like zephyrproject-rtos#48195. Signed-off-by: Marc Herbert <[email protected]>
marc-hb
added a commit
to marc-hb/zephyr
that referenced
this pull request
Nov 4, 2022
Temporary bugs, corner cases and obsolete toolchains aside, the Zephyr build is reproducible most of the time: zephyrproject-rtos#50205 and zephyrproject-rtos#14593 This means two different build machines using the same toolchain will always produce the same binary output. The previous, one-line commit made it trivial to verify that binary outputs are indeed the same by adding this single line in the buid logs: ``` [16/16] Linking C executable zephyr/zephyr.elf Memory region Used Size Region Size %age Used RAM: 53280 B 3 MB 1.69% IDT_LIST: 0 GB 2 KB 0.00% fdd2ddf2ad7d5da5bbd79b41cef8d7...1a896b989a8281111d8e205 zephyr.strip ``` This commit enables that feature by default because build reproducibility matters for (at least) two important reasons: - Security / supply chain attacks, see https://www.cisa.gov/sbom, zephyrproject-rtos#50205, https://reproducible-builds.org/ and many others. - Making sure build configurations are strictly identical when trying to reproduce elusive issues or when issuing releases. It was of course already possible to _manually_ make this Kconfig change and manually compute this checksum. However this can be impossible when dealing with an automated build system that does not archive all _intermediate_ (zephyrproject-rtos#5009) files like `zephyr.elf`. Tweaking the build configuration can also be difficult and error-prone for people who are not Zephyr developers. Most automated CI systems preserve build logs by default. Displaying the reproducible checksum by default accelerates the discovery of reproducibility bugs like zephyrproject-rtos#48195. When measured with `west build -p -b qemu_x86 samples/hello_world/`, the additional `build/zephyr/zephyr.strip` disk space required is 43 kilobytes compared to a total of 11 Megabytes. Measuring a more realistic SOF example, `zephyr.strip` weighed 690 kb which was about 0.1% of a total `build/` directory weighing 65M. To measure the build time cost I ran `west build -p -b qemu_x86 samples/hello_world/` many times in a loop with and without this PR on my Linux workstation. Stripping and checksumming made literally no time difference compared to the "noise" observed when building the same configuration. This is not surprising considering how small `zephyr.strip`: so the extra cost is most likely dominated by process creation and the total number of processes created during a Zephyr build dwarfs the few extra processes required by this feature. More surprisingly, I measured incremental builds by running `touch kernel/timer.c; west build ...` in a loop and I could not observe any visible time difference either. Signed-off-by: Marc Herbert <[email protected]>
|
@keith-zephyr would these two additional lines and checksum have sped things up? |
I find this timing ironic: while this PR was making the build reproducible, 47582 was adding a non-deterministic graph. Fix just submitted one year later: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sort the device handles stored in the __device_handles section. This
ensures Zephyr builds are reproducible.
Signed-off-by: Keith Short [email protected]