Attention is currently required from: Alex Thiessen, Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/libhwbase/+/27144?usp=email )
Change subject: Makefile: Add bincompare target ......................................................................
Patch Set 1: Code-Review+1
(4 comments)
Commit Message:
https://review.coreboot.org/c/libhwbase/+/27144/comment/36464ddf_811f00c9 : PS1, Line 7: Makefile: Add bincompare target
What is the problem you are trying to solve?
From a quick glance, looks like this allows doing reproducibility tests. There are some changes (e.g. fixing whitespace, renaming things) that do not affect the produced executables, so there's no need to test on actual hardware.
This is especially useful for coreboot, as boot testing changes requires having access to specific hardware. And even then, one can't possibly test that everything still works.
In coreboot, this is especially important when touching RAM init code: RAM init is very delicate arcane magic that causes system instability and/or boot failures when something goes wrong, and depends on the DIMMs, CPU, mainboard and even environmental conditions (temperature). For example, CB:51880 would be unreviewable if it wasn't reproducible (see "Tested with BUILD_TIMELESS=1, Roda RK9 remains identical." in the commit message, "Roda RK9" is a mainboard that uses the gm45 northbridge code).
That being said, the commit message is indeed a bit thin on words. It would be nice to have a brief description of what this target does (and instructions on how to use it, although these would be better placed inside a README or similar).
Patchset:
PS1: Just realised this is a 5-year-old change, but it's worth rescuing from Gerrit-oblivion
File Makefile.compare:
PS1:
no copyright header?
Makefile doesn't have one either. Not sure how this should be addressed.
https://review.coreboot.org/c/libhwbase/+/27144/comment/1589531b_d3f7b8eb : PS1, Line 10: [ "$$(git rev-parse $(ref))" = "$$(git rev-parse HEAD)" ] || { \ : git checkout $(ref) && \ : $(MAKE) objprefix=bincmp.ref $(bincompare-targets) || \ : { git checkout HEAD@{1}; exit 1; } && \ : git checkout HEAD@{1} && \ : $(MAKE) objprefix=bincmp.head $(bincompare-targets) && \ : res=0 && \ : for t in $(patsubst bincompare-%,%,$(bincompare-targets)); \ : do \ : b=$$t/$(notdir $(binary)); \ : r=$$(objdump -d bincmp.ref/$$b | grep -v bincmp); \ : h=$$(objdump -d bincmp.head/$$b | grep -v bincmp); \ : if [ "$$r" != "$$h" ]; then \ : printf "Binaries for %s differ.\n" "$$t"; \ : res=1; \ : fi; \ : done; exit $$res; }
hard to comprehend, making it an extra script should help
@Nico Huber: is this designed so that projects using libhwbase (e.g. libgfxinit) can also use the bincompare target?
A script may be more flexible (one could specify the two git references to compare), but may also be harder to reuse.
-
Angel Pons (Code Review)