This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] [NFC] Use DisassemblerTarget for primary target in disassembleObject.
ClosedPublic

Authored by jacek on Apr 24 2023, 1:06 PM.

Details

Summary

Further preparation for ARM64EC/ARM64X support.

Diff Detail

Event Timeline

jacek created this revision.Apr 24 2023, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2023, 1:06 PM
jacek requested review of this revision.Apr 24 2023, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2023, 1:06 PM
jacek updated this revision to Diff 516786.Apr 25 2023, 6:42 AM

Rebased.

jacek updated this revision to Diff 544122.Jul 25 2023, 3:23 PM
jhenderson added inline comments.Jul 26 2023, 12:42 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1593

I'm concerned that having the Kind of a MappingSymbol stored as a char is unclear, and a little fragile, if we start diverging from the actual letter used in the mapping symbol as you are doing here in this patch. More generally, I'm not entirely convinced by the fact that here you are making decisions about the implications of the mapping symbols now, whereas before those decisions were made at the point of impact (i.e. the point at which the right target is selected). If possible, I'd prefer it if you removed this change from this patch. I'm not necessarily opposed to it going in in a different patch, but I'd want it reviewed independently and by others who are more familiar with mapping symbols.

jacek updated this revision to Diff 544755.Jul 27 2023, 7:03 AM

I removed mapping change. Handling PrimaryIsThumb while creating mapping felt right to me, because it's not relevant in ARM64EC case and made that code path a bit easier to follow. But as you said, it's a separated change.

jhenderson accepted this revision.Jul 28 2023, 12:50 AM

Thanks, LGTM.

This revision is now accepted and ready to land.Jul 28 2023, 12:50 AM
This revision was landed with ongoing or failed builds.Jul 28 2023, 5:29 AM
This revision was automatically updated to reflect the committed changes.