This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump][AArch64] Fix ADRP target label calculation
ClosedPublic

Authored by krisb on Dec 6 2022, 2:34 AM.

Details

Summary

This patch makes ADRP target label address calculations the same as
label address calculations (see AArch64InstPrinter::printAdrpLabel()).

Otherwise the target label looks misleading as ADRP's immediate offset is,
actually, not an offset to this PC, but an offset to the current PC's
page address in pages.

See for example, llvm-objdump/ELF/AArch64/pcrel-address.yaml.
Before this patch the target label <_start+0x80> represents the
address 0x200100 + 0x80 while 0x220000 is expected.

Note that with this patch llvm-objdump output matches GNU objdump.

Diff Detail

Event Timeline

krisb created this revision.Dec 6 2022, 2:34 AM
krisb requested review of this revision.Dec 6 2022, 2:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 2:34 AM
simon_tatham accepted this revision.Dec 6 2022, 4:48 AM
This revision is now accepted and ready to land.Dec 6 2022, 4:48 AM
MaskRay added inline comments.Dec 6 2022, 11:09 AM
llvm/test/tools/llvm-objdump/ELF/AArch64/pcrel-address.yaml
19

Having the label is an interesting case. Consider adding another case without .data?

krisb added inline comments.Dec 6 2022, 12:32 PM
llvm/test/tools/llvm-objdump/ELF/AArch64/pcrel-address.yaml
19

Could you please give more details why the case with .data is an interesting one?
If we keep the .data in the example, objdump will not be able to find target label for 0x220000 address since it'll reject .text, considering only .data one, which doesn't contain any symbols.
I'm okay to add another test w/o data, just want to understand what intent was/is under the .data section, if you have some time to give the details.

krisb updated this revision to Diff 482186.Dec 12 2022, 10:30 AM

Rebase & ping

krisb updated this revision to Diff 482465.Dec 13 2022, 7:18 AM

Keep original test case and add another one.

krisb added inline comments.Dec 13 2022, 7:24 AM
llvm/test/tools/llvm-objdump/ELF/AArch64/pcrel-address.yaml
19

@MaskRay
I added another case where adrp's label points inside .text keeping original one that points inside .data.
But since there are no symbols in .data we do not have target address label calculated for the address from the original case.
Hope, I got you right.

This revision was landed with ongoing or failed builds.Dec 18 2022, 4:25 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Dec 18 2022, 10:26 AM
llvm/test/tools/llvm-objdump/ELF/AArch64/pcrel-address.yaml
19

Thanks!