This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF][ARM] Test undefined weak symbol for Thumb narrow branch
ClosedPublic

Authored by peter.smith on Aug 3 2018, 3:03 AM.

Details

Summary

Add a test for the R_ARM_THM_JUMP11 relocation to an undefined symbol. We have to use a binary object assembled with the GNU assembler as llvm-mc relaxes the narrow branch to a b.w which uses the R_ARM_THM_JUMP24 relocation.

This adds code coverage for a line introduced in D26240

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.Aug 3 2018, 3:03 AM
grimar accepted this revision.Aug 3 2018, 4:08 AM

LGTM. Thanks for this patch, Peter!

This revision is now accepted and ready to land.Aug 3 2018, 4:08 AM
grimar added a comment.Aug 3 2018, 4:11 AM

I would add information about how the object was produced though. (version of GNU as + code used?)

I've updated the comment to mention the gcc version. The code for the object is in the comment, I've made that a bit clearer.

Will wait till Monday to give Rui a chance to take a look.

grimar added inline comments.Aug 3 2018, 4:32 AM
test/ELF/arm-thumb-undefined-weak-narrow.s
13 ↗(On Diff #158974)

btw, it is saying that you used "arm-linux-gnueabihf-gcc (Linaro GCC 5.3-2016.02) "

I wonder, can you use yaml2obj?

grimar added inline comments.Aug 3 2018, 5:56 AM
test/ELF/arm-thumb-undefined-weak-narrow.s
13 ↗(On Diff #158974)

I think using GCC is fine though. What I meant is that my concern is about using
the custom compiler ("Linaro GCC" in this case). I hope regular GCC has the same output.

Thanks for the suggestion to use yaml2obj. I keep forgetting about that. I've been able to make the same test case.

grimar accepted this revision.Aug 3 2018, 7:17 AM

LGTM, thanks!

test/ELF/arm-thumb-undefined-weak-narrow.test
34 ↗(On Diff #159002)

I think it should be possible to place R_ARM_THM_JUMP11 here instead of 102.

grimar added inline comments.Aug 3 2018, 7:19 AM
test/ELF/arm-thumb-undefined-weak-narrow.test
3 ↗(On Diff #159002)

You do not need it, btw, I think.

Thanks for the comments. I've updated the diff.

ruiu accepted this revision.Aug 3 2018, 10:30 AM

LGTM

This revision was automatically updated to reflect the committed changes.