Attention is currently required from: Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56719 )
Change subject: tests/Makefile.inc: Add function wrapping mechanism ......................................................................
Patch Set 3:
(7 comments)
File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56719/comment/060df53a_c096dd9b PS2, Line 123: $(OBJCOPY) $$@.orig $$(OBJCOPY_FLAGS) $$@.orig2
nit: wouldn't it be easier to generate objcopy_wrap_flags first and then just run […]
We want to add __real_<funcname> symbols only if they are present in the object file as weak symbols. We do not want to have multiple instances of __real_<funcname> e.g. for mocka and original function.
https://review.coreboot.org/c/coreboot/+/56719/comment/0d11364a_77c8703b PS2, Line 126: $$$$sym)
Should put a (properly escaped) $ at the end of the regex to make sure it won't do substring matches […]
Great, you noticed. It would be problematic in the future.
https://review.coreboot.org/c/coreboot/+/56719/comment/60c5fd80_956a8c75 PS2, Line 126: objdump
Let's make a Makefile variable for this, same as we have for $(OBJCOPY)
Done
https://review.coreboot.org/c/coreboot/+/56719/comment/f0104aac_85ee56e4 PS2, Line 128: rev | awk '{ print $$$$2 }' | rev
You can just do […]
Oh, it really looks simpler. Thanks
https://review.coreboot.org/c/coreboot/+/56719/comment/f63a8eef_ebb57297 PS2, Line 129: .text.$$$${sym}
This should work in general, but there are rare cases where it may not (e.g. […]
This makes sense. It should cover these functions. However it still will not work. See comment above :)
https://review.coreboot.org/c/coreboot/+/56719/comment/4b500982_743819fc PS2, Line 129: 0 Oh I forgot to revert changes before pushing this CL. I'm sorry.
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.
The problem with objcopy is, there is no way (or I didn't find any) to set offset and size of symbol. Symbols created by this call have address as a value, and zero as a offset/size, when function symbols created by GCC have this values swapped.
https://review.coreboot.org/c/coreboot/+/56719/comment/2b76fb62_733b3a9b PS2, Line 129: before=$$$${sym}
I don't think you actually need the before=$$$${sym}? Does it not work without that? From my underst […]
It is not required. But due to behavior of objcopy (described in the comment below) I added it for safety. For addr=0 and before=symbol this solution was working (probably, I am some kind of lost...).
Additional info:
For `--add-symbol __real_cbfs_lookup=.text.cbfs_lookup:0x7d,function,global,before=cbfs_lookup` ``` $ objdump -t src/commonlib/bsd/cbfs_private.o.orig2 | grep cbfs_lookup
0000000000000000 l d .text.cbfs_lookup 0000000000000000 .text.cbfs_lookup 0000000000000000 w F .text.cbfs_lookup 000000000000007d cbfs_lookup ```
``` $ objdump -t src/commonlib/bsd/cbfs_private.o | grep cbfs_lookup
0000000000000000 l d .text.cbfs_lookup 0000000000000000 .text.cbfs_lookup 000000000000007d g F .text.cbfs_lookup 0000000000000000 __real_cbfs_lookup 0000000000000000 w F .text.cbfs_lookup 000000000000007d cbfs_lookup ``` For `--add-symbol` without `before=` the dump above has swapped `__real_cbfs_lookup` and `cbfs_lookup` lines.
``` $ objdump -t run | grep cbfs_lookup
00000000004025fd g F .text 0000000000000000 __real_cbfs_lookup 000000000040197e g F .text 000000000000004a cbfs_lookup ```
-
Jakub Czapiga (Code Review)