Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56719 )
Change subject: tests/Makefile.inc: Add function wrapping mechanism ......................................................................
Patch Set 2:
(8 comments)
Patchset:
PS2: Oh, cool, so does this actually work? Nice!
File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56719/comment/6b36a127_da988b65 PS2, Line 123: $(OBJCOPY) $$@.orig $$(OBJCOPY_FLAGS) $$@.orig2 nit: wouldn't it be easier to generate objcopy_wrap_flags first and then just run
$(OBJCOPY) $$@.orig $$(OBJCOPY_FLAGS) $$$$objcopy_wrap_flags $$@
in one step, rather than calling it twice?
https://review.coreboot.org/c/coreboot/+/56719/comment/35b447d8_fd8a6b00 PS2, Line 126: $$$$sym) Should put a (properly escaped) $ at the end of the regex to make sure it won't do substring matches (e.g. mocking `fmap_locate_area` shouldn't find the line for `fmap_locate_area_as_rdev`).
https://review.coreboot.org/c/coreboot/+/56719/comment/7f6e4135_e6c6729c PS2, Line 126: objdump Let's make a Makefile variable for this, same as we have for $(OBJCOPY)
https://review.coreboot.org/c/coreboot/+/56719/comment/8af49480_abcbc7fa PS2, Line 128: rev | awk '{ print $$$$2 }' | rev You can just do
awk '{ print $(NF - 1) }'
and leave out the `rev`s.
https://review.coreboot.org/c/coreboot/+/56719/comment/6942cf57_66afb986 PS2, Line 129: .text.$$$${sym} This should work in general, but there are rare cases where it may not (e.g. functions from assembly files, or if something uses an explicit __attribute__((section)) for some reason). I think it would be better if you take the section from the objdump output (field $(NF - 2)) to make sure it's the right one.
https://review.coreboot.org/c/coreboot/+/56719/comment/bc2c9ebe_872f1acf PS2, Line 129: before=$$$${sym} I don't think you actually need the before=$$$${sym}? Does it not work without that? From my understanding of the man page all that does is affect the order of symbols in the symbol table of the object file, which should make no difference for what we're doing here.
https://review.coreboot.org/c/coreboot/+/56719/comment/d29f76ca_fe2ec6cc PS2, Line 129: 0 So why are you going through the effort of extracting `addr` if you then hardcode a 0 here? Again, in 99% of cases this offset will be 0, but to preclude problems with rare edge cases it would be better to use the real offset given by objdump.