Attention is currently required from: Jakub Czapiga. Hello Jakub Czapiga,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/57009
to review the following change.
Change subject: tests: Fix function mocking for clang ......................................................................
tests: Fix function mocking for clang
clang seems to like to do some aggressive optimizations that break our approach of mocking functions for test by using objcopy to turn them weak after the fact on individual compiled object files. For example, in CB:56601 the function cbfs_get_boot_device() is mocked this way. When compiling the cbfs_boot_lookup() function in src/lib/cbfs.c with clang, it will generate a normal callq instruction to a relocation for cbfs_boot_lookup(), which can then later be pointed to the mocked version of that function. However, it will also somehow infer that the version of cbfs_boot_lookup() in that file can only ever return a pointer to the static local `ro` variable (because CONFIG_VBOOT is disabled in the environment for that particular test), and instead generate instructions that directly load the address of a relocation for that variable into %rdi for the following call to cbfs_lookup(), rather than using the real function return value. (Why it would do that is anyone's guess because this seems unlikely to be faster than just moving the function return value from %rax into %rdi like a normal compiler.)
Long story short, this optimization breaks our tests because cbfs_lookup() will be called with the wrong pointer. clang doesn't provide many options to disable individual optimizations, so the only solution seems to be to make clang aware that the function is weak during the compilation stage already, so it can be aware that it may get replaced. This patch implements that by marking the mocked functions weak via #pragma weak lines in the per-test autogenerated config header. (This generates a new problem because clang apparently likes to complain about #pragma weak lines when there's no matching function declaration in the compilation unit, and there's literally no way to disable that warning[1]. There seems to be no great way around this, so the best option I could find is to disable -Werror for clang and live with the warning spam.)
[1]: https://github.com/llvm-mirror/clang/blob/master/test/Misc/warning-flags.c
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I1f9011f444248544de7a71bbefc54edc006ae0cd --- M tests/Makefile.inc 1 file changed, 27 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/57009/1
diff --git a/tests/Makefile.inc b/tests/Makefile.inc index f45638d..a14e8c9 100644 --- a/tests/Makefile.inc +++ b/tests/Makefile.inc @@ -44,7 +44,20 @@ # -Wmissing-prototypes just make working with the test framework cumbersome. # Only put conservative warnings here that really detect code that's obviously # unintentional. -TEST_CFLAGS += -Wall -Werror -Wundef -Wstrict-prototypes -Wno-inline-asm +TEST_CFLAGS += -Wall -Wundef -Wstrict-prototypes -Wno-inline-asm + +# clang is a terrible compiler and nobody should be using it. Unfortunately, +# people sometimes do anyway. While clang is not busy making dangerous +# optimizations that break our mocking feature without offering any flags to +# individually disable them, it sometimes likes to generate warnings that make +# compilation fail with -Werror but do not fall under any specific -Wno-xxx flag +# that could be explicitly disabled. Therefore, since the only way I managed to +# work around mock-breaking clang optimizations requires generating a few +# spurious "weak identifier 'xxx' never declared" warnings, the only recourse we +# have left with clang is to just leave -Werror disabled in general and spew a +# bunch of warnings in front of the test output. It's not pretty, but at least +# it allows the tests to run at all. +TEST_CFLAGS += $(if $(filter clang,$(shell $(HOSTCC) --version)),,-Werror)
# Path for Kconfig autoheader TEST_CFLAGS += -I$(dir $(TEST_KCONFIG_AUTOHEADER)) @@ -108,7 +121,10 @@ # $1 - test name define TEST_CC_template
-# Generate custom config.h redefining given symbols +# Generate custom config.h redefining given config symbols, and declaring mocked +# functions weak. It is important that the compiler already sees that they are +# weak (and they aren't just turned weak at a later stage) to prevent certain +# optimizations that would break if the function gets replaced. $(1)-config-file := $(testobj)/$(1)/config.h $$($(1)-config-file): $(TEST_KCONFIG_AUTOHEADER) mkdir -p $$(dir $$@) @@ -120,12 +136,17 @@ printf '#undef %s\n' "$$$$key" >> $$@; \ printf '#define %s %s\n\n' "$$$$key" "$$$$value" >> $$@; \ done + printf '#ifdef __TEST_SRCOBJ__\n' >> $$@; + for m in $$($(1)-mocks); do \ + printf '#pragma weak %s\n' "$$$$m" >> $$@; \ + done + printf '#endif\n' >> $$@;
$($(1)-objs): TEST_CFLAGS += -I$$(dir $$($(1)-config-file)) \ -D__$$(shell echo $$($(1)-stage) | tr '[:lower:]' '[:upper:]')__
-# Weaken symbols listed as mocks to enable overriding in the code -$($(1)-srcobjs): OBJCOPY_FLAGS += $$(foreach mock,$$($(1)-mocks),--globalize-symbol=$$(mock) --weaken-symbol=$$(mock)) +# Give us a way to distinguish between coreboot source files and test files in code. +$($(1)-srcobjs): TEST_CFLAGS += -D__TEST_SRCOBJ__
# Compile sources and apply mocking/wrapping of selected symbols. # For each listed mock add new symbol with prefix `__real_`, @@ -134,17 +155,16 @@ mkdir -p $$(dir $$@) $(HOSTCC) $(HOSTCFLAGS) $$(TEST_CFLAGS) $($(1)-cflags) -MMD \ -MF $$(basename $$@).d -MT $$@ -c $$< -o $$@.orig - $(OBJCOPY) $$@.orig $$(OBJCOPY_FLAGS) $$@.orig2 objcopy_wrap_flags=''; \ for sym in $$($(1)-mocks); do \ - sym_line="$$$$($(OBJDUMP) -t $$@.orig2 | grep -E "[0-9a-fA-F]+\s+w\s+F\s+.*\s$$$$sym$$$$")"; \ + sym_line="$$$$($(OBJDUMP) -t $$@.orig | grep -E "[0-9a-fA-F]+\s+w\s+F\s+.*\s$$$$sym$$$$")"; \ if [ ! -z "$$$$sym_line" ] ; then \ addr="$$$$(echo "$$$$sym_line" | awk '{ print $$$$1 }')"; \ section="$$$$(echo "$$$$sym_line" | awk '{ print $$$$(NF - 2) }')"; \ objcopy_wrap_flags="$$$$objcopy_wrap_flags --add-symbol __real_$$$${sym}=$$$${section}:0x$$$${addr},function,global"; \ fi \ done ; \ - $(OBJCOPY) $$@.orig2 $$$$objcopy_wrap_flags $$@ + $(OBJCOPY) $$@.orig $$$$objcopy_wrap_flags $$@
$($(1)-bin): $($(1)-objs) $(CMOCKA_LIB) $(HOSTCC) $$^ $($(1)-cflags) $$(TEST_LDFLAGS) -o $$@