This is an archive of the discontinued LLVM Phabricator instance.

[AArch64InstPrinter] Change printADRPLabel to print the target address in hexadecimal form
ClosedPublic

Authored by MaskRay on Dec 14 2020, 1:27 PM.

Details

Summary

Similar to D77853. Change ADRP to print the target address in hex, instead of the raw immediate.
The behavior is similar to GNU objdump but we also include 0x.

Note: GNU objdump is not consistent whether or not to emit 0x for different architectures. We try emitting 0x consistently for all targets.

GNU objdump:       adrp x16, 10000000
Old llvm-objdump:  adrp x16, #0
New llvm-objdump:  adrp x16, 0x10000000

adrp Xd, 0x... assembles to a relocation referencing *ABS*+0x10000 which is not intended. We need to use a linker or use yaml2obj.
The main test is test/tools/llvm-objdump/ELF/AArch64/pcrel-address.yaml

Diff Detail

Event Timeline

MaskRay created this revision.Dec 14 2020, 1:27 PM
MaskRay requested review of this revision.Dec 14 2020, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 1:28 PM
MaskRay edited the summary of this revision. (Show Details)Dec 14 2020, 1:30 PM

Looks like there are plenty of failing tests still?

lld/test/ELF/aarch64-feature-btipac.s
174

Would you mind adding the missing new line whilst you are modifying the file, please?

llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp
1391

What's this calculation about?

psmith added inline comments.Dec 15 2020, 2:55 AM
llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp
1391

It looks we are doing more than just change the display the immediate in hex, we are calculating the destination of the instruction. Personally I think that's fine as the destination is almost always more useful than the raw immediate. It also matches GNU objdump in this area.

ADRP is a bit of strange instruction (https://developer.arm.com/docs/ddi0596/i/base-instructions-alphabetic-order/adrp-form-pc-relative-address-to-4kb-page) . It is scaled PC-relative offset from the 4Kib page address of the ADRP instruction.

MaskRay updated this revision to Diff 311954.Dec 15 2020, 9:57 AM
MaskRay marked an inline comment as done.
MaskRay edited the summary of this revision. (Show Details)

Fix lld/COFF tests

jhenderson accepted this revision.Dec 16 2020, 1:03 AM

LGTM, but I wouldn't mind @psmith or someone else with more AArch64 knowledge confirming the calculation is correct.

This revision is now accepted and ready to land.Dec 16 2020, 1:03 AM

The calculation looks good to me. It is logically equivalent to that given in the reference manual

imm = SignExtend(immhi:immlo:Zeros(12), 64); // const int64_t Offset = Op.getImm() << 12;
bits(64) base = PC[];                        // Address

base<11:0> = Zeros(12);                      // (Address & -4096)

X[d] = base + imm;                           // + Offset
This revision was landed with ongoing or failed builds.Dec 16 2020, 9:21 AM
This revision was automatically updated to reflect the committed changes.
morehouse added inline comments.
llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp
1389

This line is causing a UBSan error: http://lab.llvm.org:8011/#/builders/5/builds/2510
Please fix.

/b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp:1389:40: runtime error: left shift of negative value -32770
MaskRay marked an inline comment as done.Dec 16 2020, 2:03 PM
MaskRay added inline comments.
llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp
1389