Page MenuHomePhabricator

[llvm-objdump] Allow short options to be grouped
ClosedPublic

Authored by ormris on Feb 7 2019, 9:39 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ormris created this revision.Feb 7 2019, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 9:39 AM
Herald added a subscriber: rupprecht. · View Herald Transcript
rupprecht added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
110 ↗(On Diff #185795)

This conflicts with -d and -f when grouped args are allowed

200 ↗(On Diff #185795)

I'm very much interested in this being supported (i.e. I've seen objdump -sj.foo as mentioned in the bug), but according to http://llvm.org/docs/CommandLine.html#grouping, grouped args can't take values.

ormris added inline comments.Feb 7 2019, 10:47 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
110 ↗(On Diff #185795)

Is renaming this option possible? It doesn't seem to fit with the naming scheme...

200 ↗(On Diff #185795)

True. I'll remove this for now.

rupprecht added inline comments.Feb 11 2019, 11:38 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
110 ↗(On Diff #185795)

Yep, it should be fine to rename it to "disassemble-functions" or something equally reasonable. You'll have to update test/tools/llvm-objdump/X86/disasm-specific-funcs.test.

ormris marked an inline comment as done.Feb 13 2019, 1:32 PM
ormris added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
110 ↗(On Diff #185795)

Sounds good. I'll add that to the patch.

ormris marked an inline comment as not done.Feb 13 2019, 1:39 PM
ormris updated this revision to Diff 186764.Feb 13 2019, 3:38 PM
  • Don't allow options that take arguments to be grouped.
  • Rename df to disassemble-functions.
rupprecht accepted this revision.Feb 13 2019, 4:08 PM

Thanks!

This revision is now accepted and ready to land.Feb 13 2019, 4:08 PM

Thanks for the review! I'll go ahead and commit this.

This revision was automatically updated to reflect the committed changes.
ormris reopened this revision.Feb 14 2019, 11:49 AM

Reopening to fix bot failures.

This revision is now accepted and ready to land.Feb 14 2019, 11:49 AM
ormris updated this revision to Diff 186893.Feb 14 2019, 11:51 AM
  • Limit test to options that are more broadly supported
ormris requested review of this revision.Feb 14 2019, 11:51 AM

I think you can still add tests for the other options, e.g. test/tools/llvm-objdump/X86/macho-disassembly-g-dsym.test tests -d -g, so you could add another run statement to that test with -dg and use cmp to verify it produces the same thing

llvm/test/tools/llvm-objdump/option-grouping.test
1 ↗(On Diff #186893)

This should also verify it produces the same output as -a -f -x -z

Good point. I'll take a look.

ormris updated this revision to Diff 187029.EditedFeb 15 2019, 9:23 AM
  • Add grouped cases to tests that use more than one short switch.
  • Compare output with ungrouped variant directly in option grouping test.
rupprecht accepted this revision.Feb 15 2019, 12:58 PM

Just a couple comments, but feel free to commit once those are fixed. Thanks!

llvm/test/tools/llvm-objdump/AArch64/macho-fat-arm-disasm.test
2 ↗(On Diff #187029)

These should use cmp to verify the files are identical, instead of just checking they match the same patterns

llvm/test/tools/llvm-objdump/option-grouping.test
1 ↗(On Diff #187029)

Instead of tee, the standard way of doing this is to pass the file via FileCheck --input-file, e.g.
RUN: llvm-objdump -afxz ... > %t0
RUN: llvm-objdump -a -f -x -z ... > %t1
RUN: cmp %t0 %t1
RUN: FileCheck %s --input-file %t0

This revision is now accepted and ready to land.Feb 15 2019, 12:58 PM
ormris marked 2 inline comments as done.Feb 15 2019, 1:19 PM

Sounds good. Thanks again for the review!

llvm/test/tools/llvm-objdump/AArch64/macho-fat-arm-disasm.test
2 ↗(On Diff #187029)

OK. Will fix.

llvm/test/tools/llvm-objdump/option-grouping.test
1 ↗(On Diff #187029)

Good to know. Will fix.

jhenderson added a subscriber: rafauler.

Before relanding this @ormris, it's probably worth getting @rafauler to confirm he's okay with the option name change, as he was the original one to add it. I've added him as a reviewer. Personally, I think the justification is good, but if it is going to cause horrendous pain, perhaps we should consider alternatives?

That’s fine by me.

Sounds good. Relanding.

This revision was automatically updated to reflect the committed changes.