Add R_ARM_CALL runnable test#1225
Conversation
|
I'll appreciate some feedback on this, as this will serve as my template for adding more relocation tests. I'm sure these changes can be improved even more. |
quic-areg
left a comment
quic-areg
left a comment
There was a problem hiding this comment.
In general, I think we should prefer C inputs for runnable tests, get rid of the docs if they are going to just parrot the ABI, and separate the types of checks we make in static vs runtime tests.
|
Rename the test to R_ARM_CALL.test |
Sounds good. Thanks! |
|
I addressed the issues. Let me know if I missed something or if I can improve it even more. |
quic-areg
left a comment
quic-areg
left a comment
There was a problem hiding this comment.
LGTM mostly.
For future PRs, if there is no existing static test for the relocation it might be a good idea to add one in addition to the runtime test. R_ARM_CALL is already well tested, so that does not apply here, but it might for future relocations.
The doc looks better than before imo, but I still think it adds little value. Checking the list of supported relocations for a reviewer or developer was already a pretty trivial task (they are all defined in *RelocationFunctions.h) , and I worry that the doc risks going stale.
| # Test the ARM R_ARM_CALL relocation. | ||
| #END_COMMENT | ||
| #START_TEST | ||
| #RUN: %run_cc -o %t1.o -c %p/Inputs/1.c |
There was a problem hiding this comment.
can we establish that this generates a R_ARM_CALL relocation ?
There was a problem hiding this comment.
I can add the llvm-readelf again to check for the relocation like I was doing before. Is that enough?
|
I'm making progress. I'll leave the documentation for another issue to focus on 1 thing at a time. I improved the testing coverage of this relocation. I also started using Bare-Metal runs as much as possible to isolate the tests. |
Steven6798
changed the title
|
I also updated the lit.cfg to include qemu-system-target to use Bare-Metal runs |
Steven6798
force-pushed
the
issue-1224
branch
2 times, most recently
from
acfc516 to
3f94fee
Compare
Fix qualcomm#1224 Signed-off-by: Steven Ramirez Rosa <ramirezr@qti.qualcomm.com>
| shell: bash | ||
| run: | | ||
| sudo apt update | ||
| sudo apt install -y libsdl2-2.0-0 |
There was a problem hiding this comment.
I'm trying to use the qemu system emulator. the machine that runs the CI workflow is missing a library. Shakti suggested adding it to the workflow but clearly that did not work. If you have any suggestions about how to address this let me know.
| run_cxx = '' | ||
| sysroot = '' | ||
| emulator = '' | ||
| emulator_system = '' |
There was a problem hiding this comment.
Can you please explain why do we need system emulator? For which cases linux emulator is not enough?
There was a problem hiding this comment.
Shankar suggested using the system emulator to isolate the testing as much as possible. For example, when static linking a lot gets included in the binary, with this approach, only a minimal amount of code is included. What do you think?
There was a problem hiding this comment.
@quic-seaswara Hi Shankar, I am concerned that writing baremetal code and using system emulator requires more boilerplate and makes testing more difficult without providing any apparent benefit.
to isolate the testing as much as possible
We are only removing the libc dependency -- all other dependency are still there, and we are unlikely to encounter bugs/corner-cases in libc when doing extended testing of different relocations computations because we won't be using any advanced feature of libc.
For example, when static linking a lot gets included in the binary, with this approach, only a minimal amount of code is included.
I agree with this, but I am not sure if less size is worth the benefit here. We can always delete the test artifacts after the testing.
4 participants
Fix #1224