extensions.cmake: need unique strings, not random ones #14776

marc-hb · 2019-03-21T05:46:20Z

To support being called multiple times, the function
zephyr_library_compile_options() uses unique
options_interface_lib_${random} names. These only need to be unique, not
random. So replace them with a simple MD5 hash of the ${item} argument.

To reproduce quickly run:
sanitycheck -b -p qemu_xtensa --tag shell
... a few times and compare the output directories.

This bug sits for now at the top of the list of build reproducibility
issues the most bizarre and difficult to root cause. When running
sanitycheck over a large sample of configurations it was affecting
only qemu_xtensa/**/zephyr/arch/arch/xtensa/core/startup/Makefile
files (and their corresponding CMakeFiles/TargetDirectories.txt),
randomly ordering the following three Make targets and only these
three: rebuild_cache, edit_cache, arch__xtensa__core__startup.

The key to root causing was cmake --trace-expand which prints the random
string.

generate_unique_target_name_from_filename() also generated a random
string for the same uniqueness reason. This one was easier to root cause
and reproduce with: sanitycheck --tag gen_inc_file

Signed-off-by: Marc Herbert [email protected]

1. To support being called multiple times, the function zephyr_library_compile_options() uses unique options_interface_lib_${random} names. These only need to be unique, not random. So replace them with a simple MD5 hash of the ${item} argument. To reproduce quickly run: sanitycheck -b -p qemu_xtensa --tag shell ... a few times and compare the output directories. This bug sits for now at the top of the list of build reproducibility issues the most bizarre and difficult to root cause. When running sanitycheck over a large sample of configurations it was affecting only qemu_xtensa/**/zephyr/arch/arch/xtensa/core/startup/Makefile files (and their corresponding CMakeFiles/TargetDirectories.txt), randomly ordering the following three Make targets and only these three: rebuild_cache, edit_cache, arch__xtensa__core__startup. The key to root causing was cmake --trace-expand which prints the random string. 2. generate_unique_target_name_from_filename() also generated a random string for the same uniqueness reason. This one was easier to root cause and reproduce with: sanitycheck --tag gen_inc_file Signed-off-by: Marc Herbert <[email protected]>

codecov-io · 2019-03-21T22:43:20Z

Codecov Report

Merging #14776 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #14776   +/-   ##
=======================================
  Coverage   52.51%   52.51%           
=======================================
  Files         309      309           
  Lines       45048    45048           
  Branches    10419    10419           
=======================================
  Hits        23656    23656           
  Misses      16584    16584           
  Partials     4808     4808

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb44b7e...74783a7. Read the comment docs.

mbolivar

LGTM if it passes CI

galak · 2019-03-28T14:01:04Z

Would appreciate @SebastianBoe review on this.

SebastianBoe · 2019-04-01T14:04:12Z

cmake/extensions.cmake

@@ -1219,7 +1219,7 @@ function(generate_unique_target_name_from_filename filename target_name)
  string(REPLACE "." "_" x ${basename})
  string(REPLACE "@" "_" x ${x})

-  string(RANDOM LENGTH 8 random_chars)
+  string(MD5 unique_chars ${filename})


Is filename relative or absolute?

Can we get into trouble if we invoke 'generate_unique_target_name_from_filename' twice on two different generated files with the same relative address?

If not, then this looks good :)

Thanks for questioning this. I've done some research and experimentation. I think this collision issue:

might be possible in theory,

is extremely unlikely in practice,

if ever, will trigger a very clear error message like this one:

CMake Error at zephyr/cmake/extensions.cmake:512 (add_custom_target): add_custom_target cannot create target "gen_lets_encrypt_x3_der_inc_7ea3a619643a01cb9f7b7914adc3ad38" because another target with the same name already exists. The existing target is a custom target created in source directory "zephyr/samples/net/sockets/big_http_download". See documentation for policy CMP0002 for more details. Call Stack (most recent call first): src2/CMakeLists.txt:7 (generate_inc_file_for_target)

To trigger the above error message I hacked samples/net/sockets/big_http_download/ and invoked generate_inc_file_for_target() with the same include/generated//lets_encrypt_x3.der.inc relative argument twice. This is really not how CMakeLists.txt files are normally written because CMake generally hates/discourages/deprecates relative paths https://cmake.org/cmake/help/v3.3/variable/CMAKE_USE_RELATIVE_PATHS.html (gone in 3.4)

I scanned the current code base. There are currently eight places where this function is used:

cfb.cmake: generate_cfb_font_for_target()

samples/display/cfb_custom_font/

extensions.cmake: generate_inc_file_for_target()

samples/net/sockets/big_http_download/ samples/net/sockets/dumb_http_server/ samples/net/sockets/echo_client/ samples/net/sockets/echo_server/ samples/net/sockets/http_get/ soc/arm/nxp_lpc/lpc54xxx/ tests/application_development/gen_inc_file/

I traced these invocations and they all use absolute paths because they all rely after a (scary...) number of indirections on MAKE_CURRENT_BINARY_DIR. CMake's love of absolute paths makes sure CMAKE_CURRENT_BINARY_DIR is one https://cmake.org/cmake/help/v3.13/variable/CMAKE_CURRENT_BINARY_DIR.html

Thoughts?

Thank you for the thorough analysis. I agree with your findings and conclusion that this is safe.

SebastianBoe · 2019-04-01T14:07:28Z

cmake/extensions.cmake

-  string(RANDOM random)
-  set(lib_name options_interface_lib_${random})
+  string(MD5 uniqueness ${item})
+  set(lib_name options_interface_lib_${uniqueness})


What if we add the same item twice?

Expected behaviour would for it to be silently added once due to CMake's de-duplication pass. But now I expect we will get an obscure error.

Perhaps we need to add our own "if not exists" check to silently de-duplicate.

You're right, well-spotted and thx! I filed bug #15093 and submitted de-duplication fix #15094, thx for reviewing.

I also temporarily reverted this for testing and cmake is indeed de-duplicating identical compile options coming from multiple pseudo-libraries.

PR zephyrproject-rtos#14776 / commit 915a353 changed function zephyr_library_compile_options() to use MD5 instead of RANDOM for build reproduction reasons. As later predicted by Sebastian Bøe in the PR (thx), the names generated for these pseudo-libraries won't be unique anymore if the exact same flag gets added more than once. To reproduce the issue, simply add the following two lines in some CMakeLists.txt file like samples/hello_world/CMakeLists.txt: zephyr_library_compile_options("-Dfubar=barfu") zephyr_library_compile_options("-Dfubar=barfu") cmake will then fail like this: CMake Error at zephyr/cmake/extensions.cmake:403 (add_library): add_library cannot create target "options_interface_lib_e689fdb7eb15d801c2f95dd61301fc5e" because another target with the same name already exists. The existing target is an interface library created in source directory "zephyr/samples/hello_world". See documentation for policy CMP0002 for more details. Call Stack (most recent call first): CMakeLists.txt:9 (zephyr_library_compile_options) As requested by Sebastian, silently discard duplicate options just like CMake does. Fixes zephyrproject-rtos#15093 Signed-off-by: Marc Herbert <[email protected]>

PR #14776 / commit 915a353 changed function zephyr_library_compile_options() to use MD5 instead of RANDOM for build reproduction reasons. As later predicted by Sebastian Bøe in the PR (thx), the names generated for these pseudo-libraries won't be unique anymore if the exact same flag gets added more than once. To reproduce the issue, simply add the following two lines in some CMakeLists.txt file like samples/hello_world/CMakeLists.txt: zephyr_library_compile_options("-Dfubar=barfu") zephyr_library_compile_options("-Dfubar=barfu") cmake will then fail like this: CMake Error at zephyr/cmake/extensions.cmake:403 (add_library): add_library cannot create target "options_interface_lib_e689fdb7eb15d801c2f95dd61301fc5e" because another target with the same name already exists. The existing target is an interface library created in source directory "zephyr/samples/hello_world". See documentation for policy CMP0002 for more details. Call Stack (most recent call first): CMakeLists.txt:9 (zephyr_library_compile_options) As requested by Sebastian, silently discard duplicate options just like CMake does. Fixes #15093 Signed-off-by: Marc Herbert <[email protected]>

marc-hb requested review from nashif and SebastianBoe as code owners March 21, 2019 05:46

marc-hb requested review from mbolivar, carlescufi, tejlmand, ulfalizer and a user March 21, 2019 06:14

marc-hb mentioned this pull request Mar 21, 2019

meta issue for reproducible builds (was: tests/determinism.sh) #14593

Closed

ulfalizer approved these changes Mar 21, 2019

View reviewed changes

marc-hb added Enhancement Changes/Updates/Additions to existing features area: Build System labels Mar 21, 2019

marc-hb force-pushed the cmake-unique-not-random branch from f7258dc to 74783a7 Compare March 21, 2019 22:01

marc-hb changed the title ~~extensions.cmake: need a unique string, not a random one~~ extensions.cmake: need unique strings, not random ones Mar 21, 2019

marc-hb added the area: Toolchains Toolchains label Mar 21, 2019

mbolivar approved these changes Mar 28, 2019

View reviewed changes

galak approved these changes Mar 28, 2019

View reviewed changes

nashif approved these changes Mar 28, 2019

View reviewed changes

nashif added this to the v1.14.0 milestone Mar 28, 2019

galak merged commit 915a353 into zephyrproject-rtos:master Mar 28, 2019

SebastianBoe reviewed Apr 1, 2019

View reviewed changes

marc-hb deleted the cmake-unique-not-random branch April 1, 2019 19:17

marc-hb mentioned this pull request Apr 1, 2019

zephyr_library_compile_options() lost support for duplicates #15093

Closed

marc-hb mentioned this pull request Apr 1, 2019

zephyr_library_compile_options(): de-duplicate #15094

Merged

tejlmand mentioned this pull request Mar 18, 2022

cmake: reuse interface lib for compile options if it already exists #43909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extensions.cmake: need unique strings, not random ones #14776

extensions.cmake: need unique strings, not random ones #14776

marc-hb commented Mar 21, 2019 •

edited

Loading

codecov-io commented Mar 21, 2019

mbolivar left a comment

galak commented Mar 28, 2019

SebastianBoe Apr 1, 2019 •

edited

Loading

marc-hb Apr 1, 2019

SebastianBoe Apr 2, 2019

SebastianBoe Apr 1, 2019

marc-hb Apr 1, 2019

extensions.cmake: need unique strings, not random ones #14776

extensions.cmake: need unique strings, not random ones #14776

Conversation

marc-hb commented Mar 21, 2019 • edited Loading

codecov-io commented Mar 21, 2019

Codecov Report

mbolivar left a comment

Choose a reason for hiding this comment

galak commented Mar 28, 2019

SebastianBoe Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

marc-hb Apr 1, 2019

Choose a reason for hiding this comment

SebastianBoe Apr 2, 2019

Choose a reason for hiding this comment

SebastianBoe Apr 1, 2019

Choose a reason for hiding this comment

marc-hb Apr 1, 2019

Choose a reason for hiding this comment

marc-hb commented Mar 21, 2019 •

edited

Loading

SebastianBoe Apr 1, 2019 •

edited

Loading