This is an archive of the discontinued LLVM Phabricator instance.

[aarch64/mac] Correctly disassemble @TLVPPAGE(OFF) relocs
ClosedPublic

Authored by thakis on Oct 29 2021, 12:53 PM.

Details

Summary

llvm-otool -tV foo.o and llvm-objdump --macho -d foo.o would
previously fail on object files containing @TLVPPAGE or @TLVPPAGEOFF relocs.

Move llvm-objdump-specific test from
llvm/test/MC/AArch64/arm64-tls-modifiers-darwin.s to new
llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.test
and put test for this fix to that new file.

Fixes PR52356.

Diff Detail

Event Timeline

thakis created this revision.Oct 29 2021, 12:53 PM
thakis requested review of this revision.Oct 29 2021, 12:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2021, 12:53 PM
thakis retitled this revision from [aarch64/mac] Correctly disassembly @TLVPPAGE(OFF) relocs to [aarch64/mac] Correctly disassemble @TLVPPAGE(OFF) relocs.Oct 31 2021, 3:38 PM

maskray, would you be comfortable reviewing this?

llvm-objdump --macho -d can reproduce the issue as well. Perhaps perfer llvm-objdump which is used more frequently in tests.

The disassembly logic appears to be llvm/tools/llvm-objdump/MachODump.cpp specific.
Perhaps a better place is llvm/test/tools/llvm-objdump/MachO/ (may need to duplicate the test a bit)

llvm/test/MC/AArch64/arm64-tls-modifiers-darwin.s
11

If you switch to llvm-objdump --macho -d --no-show-raw-insn, you can omit the leading addresses and use CHECK-DIS-NEXT: instead.

thakis updated this revision to Diff 385816.Nov 9 2021, 7:41 AM
thakis marked an inline comment as done.

move test

thakis edited the summary of this revision. (Show Details)Nov 9 2021, 7:42 AM
thakis added a comment.Nov 9 2021, 7:45 AM

Thanks! I moved the test over.

llvm-otool -tV is basically the same as llvm-objdump --macho -d --no-show-raw-insn and shorter (cf parseOtoolOptions() in llvm/tools/llvm-objdump/llvm-objdump.cpp -- absence of -j is --no-show-raw-insn), so I kept the otool invocation. If you feel strongly about changing it to llvm-objdump, I'll change it.

Thanks! I moved the test over.

llvm-otool -tV is basically the same as llvm-objdump --macho -d --no-show-raw-insn and shorter (cf parseOtoolOptions() in llvm/tools/llvm-objdump/llvm-objdump.cpp -- absence of -j is --no-show-raw-insn), so I kept the otool invocation. If you feel strongly about changing it to llvm-objdump, I'll change it.

Not strong. I don't know which of llvm-otool and llvm-objdump should be canonical now...

llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.test
1 ↗(On Diff #385816)

For newer tests in many test/tools directories (and lld/test), comments start with two comment markers, i.e. ;;

7 ↗(On Diff #385816)

Prefer using llvm-mc to create the object file. It does duplicate a bit of work which is tested in test/MC, but it makes the test much easier to update. For test/tools directories we try very hard to avoid prebuilt binaries.

12 ↗(On Diff #385816)

May be better checking the assembly as well to show that the relocation is attached to the correct instruction.
Then you'll need -NEXT:

Late to the party, sorry.

MaskRay's feedback covers all my questions, so as long as that's addressed -- LGTM. Thanks for working on this Nico! (and for the review MaskRay)

thakis added a comment.Nov 9 2021, 1:39 PM

(will update in a bit)

llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.test
7 ↗(On Diff #385816)

Will do in a bit. _All_ tests in llvm/test/tools/llvm-objdump/MachO currently use checked in binaries as far as I can tell, that's why I went with this.

12 ↗(On Diff #385816)

Will do, but this part is just moved over from the other test.

thakis updated this revision to Diff 385965.Nov 9 2021, 1:52 PM
thakis marked an inline comment as done.

tweak test

MaskRay accepted this revision.Nov 9 2021, 6:56 PM

LGTM. @jhenderson usually wants to review llvm-objdump changes.

This revision is now accepted and ready to land.Nov 9 2021, 6:56 PM

I've got nothing else really to add. LGTM, with or without my test comment addressed.

llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.s
7–9

Optional, but you could slightly simplify the prefixes by dropping "CHECK-". Also, I have a personal preference for the inline edit for continuing RUN lines. The logic is that the indentation makes it clear that the second line is a continuation, whilst the trailing | makes it clear that the first line is continued - again, entirely optional though.

thakis added inline comments.Nov 10 2021, 7:34 AM
llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.s
7–9

Dropped the CHECK prefix, kept the indentation :) Thanks!

This revision was automatically updated to reflect the committed changes.