This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - For SHT_REL relocations, don't display an addend.
ClosedPublic

Authored by grimar on Dec 10 2020, 6:59 AM.

Details

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=44257.

In LLVM style we always print 0 as addend when dumping
SHT_REL relocations. It is confusing, this patch stops
printing it as the first comment on the bug page suggests.

Diff Detail

Event Timeline

grimar created this revision.Dec 10 2020, 6:59 AM
grimar requested review of this revision.Dec 10 2020, 6:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 6:59 AM
grimar updated this revision to Diff 310882.Dec 10 2020, 7:05 AM
grimar set the repository for this revision to rG LLVM Github Monorepo.
  • Fix failing LLD test case.

Thanks, this is a lot less confusing that printing 0. Ideally, we'd read the REL addend from the offset, but that's obviously a lot more effort.

My only concern about removing the 0x0 from the CHECK lines is that these tests will still pass if we do add support for reading REL addends. I see that many of the tests now contain {{$}}, maybe add this for all of them?

My only concern about removing the 0x0 from the CHECK lines is that these tests will still pass if we do add support for reading REL addends. I see that many of the tests now contain {{$}}, maybe add this for all of them?

I've updated those that are related to llvm-readobj (i.e. tests from test/tools/llvm-readobj). I haven't updated others because they are not testing llvm-readobj output or relocation addend values (they were always 0x0 previously,
what is simply wrong technically). Given that I feel that spreading of {{$}} past the test/tools/llvm-readobj folder is perhaps a bit excessive.

jhenderson accepted this revision.Dec 11 2020, 6:31 AM

Seems reasonable to me. I guess in some of the existing tests, they should be checking the addend in the relocated data, but if they don't already, we aren't making the situation worse. LGTM, with nits.

llvm/test/MC/ARM/thumb-far-jump.s
27

Nit: no new line at EOF.

llvm/test/tools/llvm-readobj/ELF/relr-relocs.test
15
This revision is now accepted and ready to land.Dec 11 2020, 6:31 AM
MaskRay accepted this revision.Dec 11 2020, 9:07 AM

Looks great! I agree that {{$}} outside of test/tools/llvm-readobj may be too excessive.

arichardson accepted this revision.Dec 11 2020, 9:12 AM

My only concern about removing the 0x0 from the CHECK lines is that these tests will still pass if we do add support for reading REL addends. I see that many of the tests now contain {{$}}, maybe add this for all of them?

I've updated those that are related to llvm-readobj (i.e. tests from test/tools/llvm-readobj). I haven't updated others because they are not testing llvm-readobj output or relocation addend values (they were always 0x0 previously,
what is simply wrong technically). Given that I feel that spreading of {{$}} past the test/tools/llvm-readobj folder is perhaps a bit excessive.

I only skimmed the testsuite changes and didn't notice that this was deliberate based on the folder. Adding {{$}} it in test/tools/llvm-readobj only makes sense to me.

This revision was landed with ongoing or failed builds.Dec 14 2020, 1:03 AM
This revision was automatically updated to reflect the committed changes.
grimar marked 2 inline comments as done.