Page MenuHomePhabricator

[llvm-objdump] Add -M {att,intel} & deprecate --x86-asm-syntax={att,intel}
ClosedPublic

Authored by MaskRay on May 1 2021, 12:27 PM.

Details

Summary

The internal cl::opt option --x86-asm-syntax sets the AsmParser and AsmWriter
dialect. The option is used by llc and llvm-mc tests to set the AsmWriter dialect.

This patch adds -M {att,intel} as GNU objdump compatible aliases (PR43413).

Note: the dialect is initialized when the MCAsmInfo is constructed.
MCInstPrinter::applyTargetSpecificCLOption is called too late and its MCAsmInfo
reference is const, so changing the cl::opt in
MCInstPrinter::applyTargetSpecificCLOption is not an option, at least without
large amount of refactoring.

Diff Detail

Event Timeline

MaskRay created this revision.May 1 2021, 12:27 PM
MaskRay requested review of this revision.May 1 2021, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2021, 12:27 PM
MaskRay retitled this revision from [llvm-objdump] Add -M {att,intel} as aliases for --x86-asm-syntax={att,intel} to [llvm-objdump] Add -M {att,intel} as user-facing alternatives to --x86-asm-syntax={att,intel}.May 1 2021, 12:32 PM
MaskRay edited the summary of this revision. (Show Details)
thakis accepted this revision.May 3 2021, 12:53 PM
thakis added a subscriber: thakis.

Thanks!

As mentioned in D101761, consider using the code bits there (especially if the plan is to remove the old-style options after the next branch).

llvm/test/tools/llvm-objdump/X86/syntax-mode.s
5

Maybe spell it -dMIntel here to cover that spelling

llvm/tools/llvm-objdump/llvm-objdump.cpp
2468

equals_lower() for better objdump compat?

2472

(same)

This revision is now accepted and ready to land.May 3 2021, 12:53 PM
hoy added a comment.May 3 2021, 1:15 PM

Thanks for working on this.

llvm/docs/CommandGuide/llvm-objdump.rst
153

Do these RISC-V switches just work? Wondering if testing is needed.

llvm/tools/llvm-objdump/llvm-objdump.cpp
2468

Can this be made an alias switch of OBJDUMP_x86_asm_syntax_att?

MaskRay updated this revision to Diff 342534.May 3 2021, 1:41 PM

Add release note.
Improve code

MaskRay marked 4 inline comments as done.May 3 2021, 1:45 PM
MaskRay added inline comments.
llvm/docs/CommandGuide/llvm-objdump.rst
153

It just works (D66139), but ideally there should be test/tools/llvm-objdump specific tests.

llvm/tools/llvm-objdump/llvm-objdump.cpp
2468

I think GNU objdump uses startswith for case-sensitive "intel" and "att" and ignores unrecognized values.

I don't think anyone wants to use "intel*" (other than the unimplemented "intel-mnenomic" so I'll just use rigid == here.

2468

Can this be made an alias switch of OBJDUMP_x86_asm_syntax_att?

No. llvm-objdump --x86-asm-syntax will retire.

MaskRay marked 3 inline comments as done.May 3 2021, 1:45 PM
hoy added inline comments.May 3 2021, 2:24 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
2468

Or the other way around? The AsmSyntax assignments looks a bit redundant. It's fine if you are going to remove the OBJDUMP_x86_asm cases later.

MaskRay marked an inline comment as done.May 3 2021, 9:53 PM
MaskRay added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
2468

No, it is needed to avoid duplicate --x86-asm-syntax.

jhenderson added inline comments.
llvm/docs/CommandGuide/llvm-objdump.rst
249

If this option is to be deprecated, we should update this part of the docs one way or another. What's the motivation to remove it though? We have many other cases of keeping older option names around in the LLVM tools. I'm not opposed to it. Just trying to understand the thought process.

(For the record, I don't think we have any internal users of this option - I'll make sure this is looked at downstream)

llvm/test/tools/llvm-objdump/X86/syntax-mode.s
2

Perhaps also worth the long-form version of the option (i.e. --disassembler-options)? With and without the =?

hoy added inline comments.May 4 2021, 8:44 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
2468

I'm confused. What do you mean by "--x86-asm-syntax will retire."? Are OBJDUMP_x86_asm_syntax_att and OBJDUMP_x86_asm_syntax_intel going away?

MaskRay updated this revision to Diff 342776.May 4 2021, 9:31 AM
MaskRay marked 2 inline comments as done.

comments

MaskRay marked 3 inline comments as done.May 4 2021, 9:32 AM
MaskRay added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
2468

llvm-objdump --x86-asm-syntax= will go away.

x86-asm-syntax is an internal cl::opt option, not intended to be exposed. -M intel will work by setting the internal x86-asm-syntax option.

MaskRay marked an inline comment as done.May 4 2021, 9:33 AM
hoy accepted this revision.May 4 2021, 10:12 AM

Current change LGTM. More thoughts may be needed on retiring the --x86-asm-syntax= options which may break downstream usage.

llvm/tools/llvm-objdump/llvm-objdump.cpp
2468

Thanks for the clarification.

jhenderson accepted this revision.May 5 2021, 12:08 AM

LGTM. Awaiting confirmation on likely lack of downstream usage, but I think even if there is no usage downstream, it should be easy enough to migrate over, given a period of at least 1 release between deprecation and removal.

MaskRay updated this revision to Diff 342957.May 5 2021, 12:20 AM
MaskRay retitled this revision from [llvm-objdump] Add -M {att,intel} as user-facing alternatives to --x86-asm-syntax={att,intel} to [llvm-objdump] Add -M {att,intel} & deprecate --x86-asm-syntax={att,intel}.

rebase

This revision was landed with ongoing or failed builds.May 5 2021, 12:20 AM
This revision was automatically updated to reflect the committed changes.