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

[davide]

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

1

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.

[davide]

tools/llvm-objdump/llvm-objdump.cpp

1526

This comment is probably too trivial to be useful.

1531–1532

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

[alexander-shaposhnikov]

tools/llvm-objdump/llvm-objdump.cpp

1525

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

[paulsemel]

Added @davide suggestions

tools/llvm-objdump/llvm-objdump.cpp

1525

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

1531–1532

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

[jhenderson]

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

4

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

1520

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

1529

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]

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

tools/llvm-objdump/llvm-objdump.cpp

1520

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

[jhenderson]

tools/llvm-objdump/llvm-objdump.cpp

1236

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?).

1520

Warning is fine for me.

[paulsemel]

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

[paulsemel]

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

[jhenderson]

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

333

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]

Added Jame's suggestions

tools/llvm-objdump/llvm-objdump.cpp

333

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

[paulsemel]

ping @davide

[jhenderson]

tools/llvm-objdump/llvm-objdump.cpp

333

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

[paulsemel]

tools/llvm-objdump/llvm-objdump.cpp

333

hmm alright, sound weird to me, doesn't make really sense.. but thanks for the link 😄

[davide]

I have no more comments.

[paulsemel]

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 :)

[paulsemel]

ping @davide @jhenderson

[jhenderson]

In D49043#1161224, @paulsemel wrote:

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?

[paulsemel]

In D49043#1161402, @jhenderson wrote:

In D49043#1161224, @paulsemel wrote:

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 ? :)

[jhenderson]

In D49043#1161463, @paulsemel wrote:

In D49043#1161402, @jhenderson wrote:

In D49043#1161224, @paulsemel wrote:

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]

Added Jame's suggestions

[jhenderson]

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.

[paulsemel]

In D49043#1163274, @jhenderson wrote:

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 ! :)

[echristo]

In D49043#1163285, @paulsemel wrote:

In D49043#1163274, @jhenderson wrote:

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.

[paulsemel]

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

[paulsemel]

Move test to X86 folder to make it x86 specific

[echristo]

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.

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

+# RUN: llvm-objdump -d -C %p/Inputs/mangled.x86_64 | FileCheck %s
+
+# We just want to check that the symbols are demangled
+# CHECK: foo()
+# CHECK: foo(int)

tools/llvm-objdump/llvm-objdump.h

 extern cl::opt<std::string> TripleName;
 extern cl::opt<std::string> ArchName;
 extern cl::opt<std::string> MCPU;
 extern cl::list<std::string> MAttrs;
 extern cl::list<std::string> FilterSections;
 extern cl::opt<bool> AllHeaders;
 extern cl::opt<bool> Disassemble;
 extern cl::opt<bool> DisassembleAll;
+extern cl::opt<bool> Demangle;
 extern cl::opt<bool> NoShowRawInsn;
 extern cl::opt<bool> NoLeadingAddr;
 extern cl::opt<bool> PrivateHeaders;
 extern cl::opt<bool> FileHeaders;
 extern cl::opt<bool> FirstPrivateHeader;
 extern cl::opt<bool> ExportsTrie;
 extern cl::opt<bool> Rebase;
 extern cl::opt<bool> Bind;

tools/llvm-objdump/llvm-objdump.cpp

 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/CodeGen/FaultMaps.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/Symbolize/Symbolize.h"
+#include "llvm/Demangle/Demangle.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDisassembler/MCDisassembler.h"
 #include "llvm/MC/MCDisassembler/MCRelocationInfo.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstPrinter.h"
 #include "llvm/MC/MCInstrAnalysis.h"
 #include "llvm/MC/MCInstrInfo.h"

 
 cl::opt<bool>
 llvm::DisassembleAll("disassemble-all",
   cl::desc("Display assembler mnemonics for the machine instructions"));
 static cl::alias
 DisassembleAlld("D", cl::desc("Alias for --disassemble-all"),
              cl::aliasopt(DisassembleAll));
 
+cl::opt<bool> llvm::Demangle("demangle", cl::desc("Demangle symbols names"));
+static cl::alias DemangleShort("C", cl::desc("Alias for --demangle"),
+                               cl::aliasopt(Demangle));
+
 static cl::list<std::string>
 DisassembleFunctions("df",
                      cl::CommaSeparated,
                      cl::desc("List of functions to disassemble"));
 static StringSet<> DisasmFuncsSet;
 
 cl::opt<bool>
 llvm::Relocations("r", cl::desc("Display the relocation entries in the file"));
if (!EC)
     return;
 
   errs() << ToolName << ": error reading file: " << EC.message() << ".\n";
   errs().flush();
   exit(1);
 }
 
 LLVM_ATTRIBUTE_NORETURN void llvm::error(Twine Message) {
   errs() << ToolName << ": " << Message << ".\n";
   errs().flush();
   exit(1);
 }
 
 LLVM_ATTRIBUTE_NORETURN void llvm::report_error(StringRef File,
                                                 Twine Message) {
   errs() << ToolName << ": '" << File << "': " << Message << ".\n";
   exit(1);

 }
 
 static void DisassembleObject(const ObjectFile *Obj, bool InlineRelocs) {
   if (StartAddress > StopAddress)
     error("Start address should be less than stop address");
 
   const Target *TheTarget = getTarget(Obj);
 
   // Package up features to be passed to target/subtarget
   SubtargetFeatures Features = Obj->getFeatures();
   if (MAttrs.size()) {
     for (unsigned i = 0; i != MAttrs.size(); ++i)
       Features.AddFeature(MAttrs[i]);
   }
 
   std::unique_ptr<const MCRegisterInfo> MRI(
       TheTarget->createMCRegInfo(TripleName));
for (unsigned si = 0, se = Symbols.size(); si != se; ++si) {
           const uint64_t EndAlign = 256;
           const auto Limit = End - (std::min)(EndAlign, End - Start);
           while (End > Limit &&
             *reinterpret_cast<const support::ulittle32_t*>(&Bytes[End - 4]) == 0)
             End -= 4;
         }
       }
 
-      outs() << '\n' << std::get<1>(Symbols[si]) << ":\n";
+      StringRef SymbolName = std::get<1>(Symbols[si]);
+      if (Demangle) {
+        char *DemangledSymbol = nullptr;
+        size_t Size = 0;
+        int Status;
+        DemangledSymbol =
+            itaniumDemangle(SymbolName.data(), DemangledSymbol, &Size, &Status);
+        if (Status == 0) // Success
+          outs() << '\n' << DemangledSymbol << ":\n";
+        else
+          outs() << '\n' << SymbolName << ":\n";
+
+        if (Size != 0)
+          free(DemangledSymbol);
+      } else
+        outs() << '\n' << SymbolName << ":\n";
 
       // Don't print raw contents of a virtual section. A virtual section
       // doesn't have any contents in the file.
       if (Section.isVirtual()) {
         outs() << "...\n";
         continue;
       }