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(a)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 $$(a)).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 $$@
--
To view, visit https://review.coreboot.org/c/coreboot/+/57009
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1f9011f444248544de7a71bbefc54edc006ae0cd
Gerrit-Change-Number: 57009
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-MessageType: newchange
Malik Hsu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57007 )
Change subject: mb/google/brya: Enable ADL_ENABLE_USB4_PCIE_RESOURCES for primus
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57007/comment/07695ee5_54f88f9d
PS1, Line 9: Enable ADL_ENABLE_USB4_PCIE_RESOURCES support for primus.
> suggestion: primus supports USB4 and so needs to reserve bus numbers and prefmem and mem resources f […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/57007
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1d1f8cc3460c1b89dade4f01690c77efcd799098
Gerrit-Change-Number: 57007
Gerrit-PatchSet: 2
Gerrit-Owner: Malik Hsu <malik_hsu(a)wistron.corp-partner.google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Casper Chang <casper_chang(a)wistron.corp-partner.google.com>
Gerrit-Comment-Date: Wed, 18 Aug 2021 03:10:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Malik Hsu.
Hello build bot (Jenkins), Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/57007
to look at the new patch set (#2).
Change subject: mb/google/brya: Enable ADL_ENABLE_USB4_PCIE_RESOURCES for primus
......................................................................
mb/google/brya: Enable ADL_ENABLE_USB4_PCIE_RESOURCES for primus
primus supports USB4 and so needs to reserve bus numbers and prefmem and
mem resources for potential hotplugs of devices.
BUG=b:193377625
BRANCH=None
TEST=build pass
Signed-off-by: Malik_Hsu <malik_hsu(a)wistron.corp-partner.google.com>
Change-Id: I1d1f8cc3460c1b89dade4f01690c77efcd799098
---
M src/mainboard/google/brya/Kconfig.name
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/57007/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/57007
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1d1f8cc3460c1b89dade4f01690c77efcd799098
Gerrit-Change-Number: 57007
Gerrit-PatchSet: 2
Gerrit-Owner: Malik Hsu <malik_hsu(a)wistron.corp-partner.google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Casper Chang <casper_chang(a)wistron.corp-partner.google.com>
Gerrit-Attention: Malik Hsu <malik_hsu(a)wistron.corp-partner.google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Malik Hsu.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57007 )
Change subject: mb/google/brya: Enable ADL_ENABLE_USB4_PCIE_RESOURCES for primus
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57007/comment/7d841223_03b0d87e
PS1, Line 9: Enable ADL_ENABLE_USB4_PCIE_RESOURCES support for primus.
suggestion: primus supports USB4 and so needs to reserve bus numbers and prefmem and mem resources for potential hotplugs
of devices.
--
To view, visit https://review.coreboot.org/c/coreboot/+/57007
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1d1f8cc3460c1b89dade4f01690c77efcd799098
Gerrit-Change-Number: 57007
Gerrit-PatchSet: 1
Gerrit-Owner: Malik Hsu <malik_hsu(a)wistron.corp-partner.google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Casper Chang <casper_chang(a)wistron.corp-partner.google.com>
Gerrit-Attention: Malik Hsu <malik_hsu(a)wistron.corp-partner.google.com>
Gerrit-Comment-Date: Wed, 18 Aug 2021 02:49:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Paul Menzel, EricR Lai, Karthik Ramasubramanian.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57000 )
Change subject: mb/google/brya: set tcc_offset value to 10
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57000/comment/cbcf5ad3_71b80641
PS1, Line 10: Circuit (TCC) activation feature.
> Why 10? Please document, where you got the value from.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/57000
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I22573e8ca935d99a16b0876768df169db4e61c4d
Gerrit-Change-Number: 57000
Gerrit-PatchSet: 2
Gerrit-Owner: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Wed, 18 Aug 2021 02:34:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Sumeet R Pawnikar, EricR Lai, Karthik Ramasubramanian.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, EricR Lai, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/57000
to look at the new patch set (#2).
Change subject: mb/google/brya: set tcc_offset value to 10
......................................................................
mb/google/brya: set tcc_offset value to 10
Set tcc_offset value to 10 in devicetree for Thermal Control
Circuit (TCC) activation feature. This value is suggested by
Thermal team.
BUGb=b:195706434
BRANCH=None
TEST=Built for brya platform and verified the MSR value
Change-Id: I22573e8ca935d99a16b0876768df169db4e61c4d
Signed-off-by: Sumeet Pawnikar <sumeet.r.pawnikar(a)intel.com>
---
M src/mainboard/google/brya/variants/baseboard/brya/devicetree.cb
1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/57000/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/57000
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I22573e8ca935d99a16b0876768df169db4e61c4d
Gerrit-Change-Number: 57000
Gerrit-PatchSet: 2
Gerrit-Owner: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Attention: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Kangheui Won, Paul Menzel.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56990 )
Change subject: mb/google/zork: only enable RTD2141 when present
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56990/comment/8fd60d19_bc7f202c
PS2, Line 10: fw_config
: probe
> With three options?
I'm not clear on what you're asking here. Are you questioning that it's a single probe, or asking that this be changed to clarify the nature of the probes as matching on variant-specific options?
--
To view, visit https://review.coreboot.org/c/coreboot/+/56990
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4ada9b492ab221fa98350bf2faf27a23342f3a55
Gerrit-Change-Number: 56990
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Kangheui Won <khwon(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Wed, 18 Aug 2021 02:18:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Hello build bot (Jenkins), Tim Wawrzynczak, Paul Menzel,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/56981
to look at the new patch set (#4).
Change subject: mb/google/brya/variants/primus: Fix GL9755S power sequence
......................................................................
mb/google/brya/variants/primus: Fix GL9755S power sequence
- Enable EN_PP3300_SD
- Configure SD_PE_RST_L correctly
BUG=b:195625340
TEST=Able to boot with SD card
Signed-off-by: Malik_Hsu <malik_hsu(a)wistron.corp-partner.google.com>
Change-Id: I33c17e88cabdc9b13634fc8f341aa6a09b7bfde5
---
M src/mainboard/google/brya/variants/primus/gpio.c
1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/56981/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/56981
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I33c17e88cabdc9b13634fc8f341aa6a09b7bfde5
Gerrit-Change-Number: 56981
Gerrit-PatchSet: 4
Gerrit-Owner: Malik Hsu <malik_hsu(a)wistron.corp-partner.google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Casper Chang <casper_chang(a)wistron.corp-partner.google.com>
Gerrit-MessageType: newpatchset