Page MenuHomePhabricator

[llvm-objdump] Add -demangle (-C) option
ClosedPublic

Authored by paulsemel on Jul 6 2018, 2:01 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.Jul 6 2018, 2:01 PM
davide requested changes to this revision.Jul 6 2018, 2:08 PM
davide added a subscriber: davide.
davide added inline comments.
test/tools/llvm-objdump/disassemble-demangle.test
2

please use yaml2obj instead of checking in a binary.

tools/llvm-objdump/llvm-objdump.cpp
94โ€“97

This syntax breaks the GNU one.
--demangle/-C accepts a string, which is the STYLE.
I don't think it really matters a lot nowadays, but given you're going through the compatibility route, you might want to accept the style and ignore it.

This revision now requires changes to proceed.Jul 6 2018, 2:08 PM
davide added inline comments.Jul 6 2018, 2:09 PM
tools/llvm-objdump/llvm-objdump.cpp
1529

This comment is probably too trivial to be useful.

1534โ€“1535

Why not freeing always on success? Do you really need to look at the size?

alexshap added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
1528

just in case - why does it always use itaniumDemanlge / what about other ABIs ?

paulsemel updated this revision to Diff 154504.Jul 7 2018, 3:06 PM
paulsemel marked 3 inline comments as done.

Added @davide suggestions

tools/llvm-objdump/llvm-objdump.cpp
1528

This is related to @davide remark. As he pointed, for now, we will only handle this ABI :)

1534โ€“1535

Because not having success doesn't mean there was no allocation ?

jhenderson added inline comments.Jul 9 2018, 1:50 AM
test/tools/llvm-objdump/disassemble-demangle.test
3

Since you've added support for the string parameter for -C, please add tests cases for that style too, e.g. --demangle=none and --demangle=itanium.

Also, please add long-name versions of this switch in the test (i.e. have both -C and --demangle).

tools/llvm-objdump/llvm-objdump.cpp
1523

Would it perhaps make sense to reject/warn for styles that don't make sense?

1532

To avoid us duplicating the printing code (which could lead to divergence, if we decide the format needs to be slightly different at some point), how would you feel about putting this into a small lambda, which can then be called with the corresponding name? Something like:

auto PrintSymbol = [](StringRef Name){ outs() << '\n' << Name << ":\n"; };
paulsemel updated this revision to Diff 154763.Jul 10 2018, 1:01 AM
paulsemel marked 3 inline comments as done.

Warn for bad demangling styles.
Added tests for different styles.

tools/llvm-objdump/llvm-objdump.cpp
1523

Maybe we should just warn, as we still want the disassembling happen right ?

jhenderson added inline comments.Jul 10 2018, 3:37 AM
tools/llvm-objdump/llvm-objdump.cpp
1239

I think this should be at command-line processing time, not here, since we should warn for bad patterns, even if no disassembling is required.

Additionally, this will warn every time this function is called, which if llvm-objdump were to accept multiple objects or presumably parse objects in an archive, would be bad.

What do other LLVM tools do when they get warnings? I'd sort of expect this to be prefixed with "warning:" or similar. It might be worth adding a helper, alongside error(), for this use case. You should also make sure that the style of message matches that of errors in the tool (i.e. first letter capitalization and trailing full stop - should they be exist or not?).

1523

Warning is fine for me.

paulsemel updated this revision to Diff 154792.Jul 10 2018, 6:56 AM
paulsemel marked an inline comment as done.

Added warning function and checked for bad style in the option checking part

paulsemel updated this revision to Diff 154797.Jul 10 2018, 7:06 AM

Added "warning" to the test to ensure this is printed as a warning.

jhenderson accepted this revision.Jul 10 2018, 7:10 AM

LGTM, with a few minor points.

You should get @davide to confirm he's happy though before submission, since he previously requested changes.

test/tools/llvm-objdump/disassemble-demangle.test
46

I'd add "warning:" to the front of this check. And maybe rename the check "BAD-STYLE".

tools/llvm-objdump/llvm-objdump.cpp
336

Do you need to do errs().flush() like in the other error methods?

Also, I'd move this to be before or after the two error() functions rather than in between them.

paulsemel updated this revision to Diff 154888.Jul 10 2018, 2:45 PM

Added Jame's suggestions

tools/llvm-objdump/llvm-objdump.cpp
336

I can add errs().flush(), but with the \n, it should be flushed !

jhenderson added inline comments.Jul 11 2018, 1:38 AM
tools/llvm-objdump/llvm-objdump.cpp
336

Actually, I don't think that's the case! See https://stackoverflow.com/a/213977.

paulsemel added inline comments.Jul 11 2018, 1:49 AM
tools/llvm-objdump/llvm-objdump.cpp
336

hmm alright, sound weird to me, doesn't make really sense.. but thanks for the link ๐Ÿ˜„

davide accepted this revision.Jul 11 2018, 7:43 AM

I have no more comments.

This revision is now accepted and ready to land.Jul 11 2018, 7:43 AM
This revision was automatically updated to reflect the committed changes.
paulsemel reopened this revision.Jul 13 2018, 12:24 AM

The buildbots were not passing. Again it seemed to be because of the disassembling option on other architectures (arm etc..).
If someone have any insight about what to do to test this disassembling option, I'll be glad :)

This revision is now accepted and ready to land.Jul 13 2018, 12:24 AM

The buildbots were not passing. Again it seemed to be because of the disassembling option on other architectures (arm etc..).
If someone have any insight about what to do to test this disassembling option, I'll be glad :)

Is the problem as simple as adding a "REQUIRES: x86" to the test? Do the arm build bots etc have x86 target support?

The buildbots were not passing. Again it seemed to be because of the disassembling option on other architectures (arm etc..).
If someone have any insight about what to do to test this disassembling option, I'll be glad :)

Is the problem as simple as adding a "REQUIRES: x86" to the test? Do the arm build bots etc have x86 target support?

I feel terrible, I don't know the REQUIRES thing... where should I put this ? :)

The buildbots were not passing. Again it seemed to be because of the disassembling option on other architectures (arm etc..).
If someone have any insight about what to do to test this disassembling option, I'll be glad :)

Is the problem as simple as adding a "REQUIRES: x86" to the test? Do the arm build bots etc have x86 target support?

I feel terrible, I don't know the REQUIRES thing... where should I put this ? :)

Take a look at the documentation for it here:
https://llvm.org/docs/TestingGuide.html#constraining-test-execution

paulsemel updated this revision to Diff 155633.Jul 16 2018, 2:30 AM

Added Jame's suggestions

jhenderson accepted this revision.Jul 16 2018, 3:15 AM

LGTM, with the caveat that I don't know for certain if that was the original issue, without looking at the failing job and what was going on.

If you need to make minor changes to get the build bots working, no need for further review from my point of view.

LGTM, with the caveat that I don't know for certain if that was the original issue, without looking at the failing job and what was going on.

If you need to make minor changes to get the build bots working, no need for further review from my point of view.

I just asked @echristo if he could give this patch a try on non-x86 architecture, to be sure it was the problem ! :)

LGTM, with the caveat that I don't know for certain if that was the original issue, without looking at the failing job and what was going on.

If you need to make minor changes to get the build bots working, no need for further review from my point of view.

I just asked @echristo if he could give this patch a try on non-x86 architecture, to be sure it was the problem ! :)

Haven't been able to without setting up an all new dev machine so... give it a shot and we can try again and see where we end up on the bots. Just stick around a little after to see if everything is ok.

Ok, I think I actually have to move this test to the X86 folder, but I don't need this REQUIRES thing.

paulsemel updated this revision to Diff 156091.Jul 18 2018, 9:10 AM

Move test to X86 folder to make it x86 specific

echristo accepted this revision.Jul 18 2018, 9:23 AM

Concerned about why... but let's just do it for now and figure out the test problem later. It's not a problem with the code as far as I can tell.

This revision was automatically updated to reflect the committed changes.