Move llvm-objdump demangle function into demangler library

[jhenderson]

Summary

I'm currently working on https://bugs.llvm.org/show_bug.cgi?id=40054. In order to avoid duplication of the demangling code in llvm-objdump, this change moves that code into the demangler library ready for reuse in llvm-readobj. The new function in the library provides memory management via std::string, and also calls both the Itanium and Microsoft demanglers, as per the old behaviour. One minor change is that the microsoftDemangle function will always be called if the input string does not start with "_Z", since the pre-existing "?" check is unnecessary (the microsoftDemangle function does this same check).

One aspect I'm uncertain on is the usage of std::string in Demangle.h. If this is an issue, I would welcome suggestions for an alternative location for this function.

[erik.pilkington]

Hi, thanks for working on this! This seems like a useful function.

include/llvm/Demangle/Demangle.h

42 ↗

(On Diff #181782)

MangledName

lib/Demangle/Demangle.cpp

18–19 ↗

(On Diff #181782)

nit: Maybe just explicitly write this as std::string llvm::demangle(...) { instead of the namespace?

23 ↗

(On Diff #181782)

You should probably also send strings that start with "___Z" to the itanium demangler.

26 ↗

(On Diff #181782)

nit: may as well just pass in nullptr instead of Demangled.

tools/llvm-objdump/llvm-objdump.cpp

531 ↗

(On Diff #181782)

StringRef::data doesn't guarantee a null-terminator. This may not be a problem in practice here, but to be safe you should call demangle(Name.str().c_str()). Alternatively, making demangle take a std::string probably wouldn't be the end of the world, since it isn't the "high performance" demangling API anyways since it copies the result buffer (which is likely much larger than then the input).

2088 ↗

(On Diff #181782)

(ditto)

[grimar]

lib/Demangle/Demangle.cpp

20 ↗

(On Diff #181782)

This line does not seem to be clang-formatted, but I also think it is not useful.
In case MangledName is null, you'll have a crash anyways, the crash is an obvious assert by itself.
From what I saw in LLVM's code, assert is more commonly used to check the more complex logic flows, so I would remove it.

23 ↗

(On Diff #181782)

This patch just moves the existent code, I would not add any additional functionality in it.

26 ↗

(On Diff #181782)

The same.

tools/llvm-objdump/llvm-objdump.cpp

530 ↗

(On Diff #181782)

Not sure what is the point to have this assert either. I would remove it. If you want to add an additional check like this, it should be something that would report an error in release build then I think.

[jhenderson]

Address review comments:

• Switch to .str().c_str() to ensure null-termination instead of assert
• clang-format
• Fix doc comment
• Pass in nullptr into demangle functions explicitly
• change namespacing style

[jhenderson]

lib/Demangle/Demangle.cpp

20 ↗

(On Diff #181782)

This protects us against undefined behaviour in strncmp below. It may not be a crash if nullptr is passed in.

23 ↗

(On Diff #181782)

I agree, though I don't mind it being in a follow-up change, if desired. The main advantage of this change as is, is that it is a pure refactor (there might be some minor differences in performance, but the end behaviour should be identical to what llvm-objdump previously did).

26 ↗

(On Diff #181782)

In this case, passing in nullptr is identical to passing in Demangled. It's safe to change.

tools/llvm-objdump/llvm-objdump.cpp

531 ↗

(On Diff #181782)

In this particular case, that was the purpose of the earlier assert. However, in the other two places, it's not straightforward to construct an assert that checks: although we know that the string tables in practice are null-terminated currently (and checked as such), this is not easily proven in this area.

I think being safe is important, so I'll change to the .str().c_str() suggestion, and if there's a clear performance issue, it can be revisited later.

[grimar]

lib/Demangle/Demangle.cpp

20 ↗

(On Diff #181782)

But this does not protect us from anything in release builds.
Should it be

if (!MangledName)
  llvm_unreachable(...);

then?

[grimar]

lib/Demangle/Demangle.cpp

20 ↗

(On Diff #181782)

Seems you can just change the argument to const std::string&?

[labath]

But this does not protect us from anything in release builds.

Isn't that the whole point of asserts? "Catch the things that are not supposed to happen in debug builds so that you don't have to pay the cost of checking them in release builds."

[jhenderson]

In D56721#1359558, @labath wrote:

But this does not protect us from anything in release builds.

Isn't that the whole point of asserts? "Catch the things that are not supposed to happen in debug builds so that you don't have to pay the cost of checking them in release builds."

Exactly. The code isn't supposed to get into this state in the first place (it would be a programmer error if it did), so we can assume it isn't in our release builds.

lib/Demangle/Demangle.cpp

20 ↗

(On Diff #181782)

I'd rather not do that, if I can avoid it. If the user already knows their string is a null-terminated string (e.g. because it's a string literal), they can just pass in the relevant pointer, without being forced to incur the cost of copying to a std::string. In other situations, I'd use a StringRef, to avoid the copy, but that would introduce a dependency on the Support library, which we can't do here.

[grimar]

OK, I have no more comments.

[erik.pilkington]

LGTM, thanks!

lib/Demangle/Demangle.cpp

23 ↗

(On Diff #181782)

Sure, doing this in a follow-up is fine.

[erik.pilkington]

lib/Demangle/Demangle.cpp

20 ↗

(On Diff #181782)

For the record, I would also prefer doing const string & parameter. The cost of copying the demangled string is already going to be much more than the input string, so I don't think the performance argument is that great. Your call though.

[jhenderson]

Use const std::string &. This allowed removal of the assert, the previously needed cassert and cstring headers (I'm using std::string::compare instead of strcmp), and the .str().c_str() calls. Also rebased.

[grimar]

LGTM.

[jhenderson]

The build of llvm-undname failed with my latest version, due to an ambiguous function name, so I've just renamed the function in that tool from demangle to msDemangle.

[grimar]

This is probably fine.

[thakis]

llvm/trunk/lib/Demangle/Demangle.cpp

18

FWIW Itanium symbols can have up to 4 leading underscores. For example, ___Z10blocksNRVOv_block_invoke is "___Z10blocksNRVOv_block_invoke" according to lib/Demangle.

[erik.pilkington]

llvm/trunk/lib/Demangle/Demangle.cpp

18

This is handled in a follow-up: https://reviews.llvm.org/D56855

[thakis]

llvm/trunk/lib/Demangle/Demangle.cpp

18

And your _Z3fooi test is __Z3fooi on darwin, which you probably want to support as well.

[erik.pilkington]

llvm/trunk/lib/Demangle/Demangle.cpp

18

Yeah, we probably want to just do the right thing there. That requires changes to ManglingParser::parse() first though.

[erik.pilkington]

llvm/trunk/lib/Demangle/Demangle.cpp

18

Fixed this in r351481.

llvm/trunk/include/llvm/Demangle/Demangle.h

 //===--- Demangle.h ---------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_DEMANGLE_DEMANGLE_H
 #define LLVM_DEMANGLE_DEMANGLE_H
 
 #include <cstddef>
+#include <string>
 
 namespace llvm {
 /// This is a llvm local version of __cxa_demangle. Other than the name and
 /// being in the llvm namespace it is identical.
 ///
 /// The mangled_name is demangled into buf and returned. If the buffer is not
 /// large enough, realloc is used to expand it.
 ///

 char *itaniumDemangle(const char *mangled_name, char *buf, size_t *n,
                       int *status);
 
 
 enum MSDemangleFlags { MSDF_None = 0, MSDF_DumpBackrefs = 1 << 0 };
 char *microsoftDemangle(const char *mangled_name, char *buf, size_t *n,
                         int *status, MSDemangleFlags Flags = MSDF_None);
 
+/// Attempt to demangle a string using different demangling schemes.
+/// The function uses heuristics to determine which demangling scheme to use.
+/// \param MangledName - reference to string to demangle.
+/// \returns - the demangled string, or a copy of the input string if no
+/// demangling occurred.
+std::string demangle(const std::string &MangledName);
+
 /// "Partial" demangler. This supports demangling a string into an AST
 /// (typically an intermediate stage in itaniumDemangle) and querying certain
 /// properties or partially printing the demangled name.
 struct ItaniumPartialDemangler {
   ItaniumPartialDemangler();
 
   ItaniumPartialDemangler(ItaniumPartialDemangler &&Other);
   ItaniumPartialDemangler &operator=(ItaniumPartialDemangler &&Other);

llvm/trunk/lib/Demangle/CMakeLists.txt

 add_llvm_library(LLVMDemangle
+  Demangle.cpp
   ItaniumDemangle.cpp
   MicrosoftDemangle.cpp
   MicrosoftDemangleNodes.cpp
 
   ADDITIONAL_HEADER_DIRS
   "${LLVM_MAIN_INCLUDE_DIR}/llvm/Demangle"
 )

llvm/trunk/lib/Demangle/Demangle.cpp

+//===-- Demangle.cpp - Common demangling functions ------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file This file contains definitions of common demangling functions.
+///
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Demangle/Demangle.h"
+
+std::string llvm::demangle(const std::string &MangledName) {
+  char *Demangled;
+  if (MangledName.compare(0, 2, "_Z") == 0)
+    Demangled = itaniumDemangle(MangledName.c_str(), nullptr, nullptr, nullptr);
+  else
+    Demangled =
+        microsoftDemangle(MangledName.c_str(), nullptr, nullptr, nullptr);
+
+  if (!Demangled)
+    return MangledName;
+
+  std::string Ret = Demangled;
+  free(Demangled);
+  return Ret;
+}

llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp

static const Target *getTarget(const ObjectFile *Obj = nullptr) {
   TripleName = TheTriple.getTriple();
   return TheTarget;
 }
 
 bool llvm::isRelocAddressLess(RelocationRef A, RelocationRef B) {
   return A.getOffset() < B.getOffset();
 }
 
-static std::string demangle(StringRef Name) {
-  char *Demangled = nullptr;
-  if (Name.startswith("_Z"))
-    Demangled = itaniumDemangle(Name.data(), Demangled, nullptr, nullptr);
-  else if (Name.startswith("?"))
-    Demangled = microsoftDemangle(Name.data(), Demangled, nullptr, nullptr);
-
-  if (!Demangled)
-    return Name;
-
-  std::string Ret = Demangled;
-  free(Demangled);
-  return Ret;
-}
-
 template <class ELFT>
 static std::error_code getRelocationValueString(const ELFObjectFile<ELFT> *Obj,
                                                 const RelocationRef &RelRef,
                                                 SmallVectorImpl<char> &Result) {
   typedef typename ELFObjectFile<ELFT>::Elf_Sym Elf_Sym;
   typedef typename ELFObjectFile<ELFT>::Elf_Shdr Elf_Shdr;
   typedef typename ELFObjectFile<ELFT>::Elf_Rela Elf_Rela;
 

llvm/trunk/tools/llvm-undname/llvm-undname.cpp

 using namespace llvm;
 
 cl::opt<bool> DumpBackReferences("backrefs", cl::Optional,
                                  cl::desc("dump backreferences"), cl::Hidden,
                                  cl::init(false));
 cl::list<std::string> Symbols(cl::Positional, cl::desc("<input symbols>"),
                               cl::ZeroOrMore);
 
-static void demangle(const std::string &S) {
+static void msDemangle(const std::string &S) {
   int Status;
   MSDemangleFlags Flags = MSDF_None;
   if (DumpBackReferences)
     Flags = MSDemangleFlags(Flags | MSDF_DumpBackrefs);
 
   char *ResultBuf =
       microsoftDemangle(S.c_str(), nullptr, nullptr, &Status, Flags);
   if (Status == llvm::demangle_success) {
while (true) {
       // If the user is manually typing in these decorated names, don't echo
       // them to the terminal a second time.  If they're coming from redirected
       // input, however, then we should display the input line so that the
       // mangled and demangled name can be easily correlated in the output.
       if (!sys::Process::StandardInIsUserInput()) {
         outs() << Line << "\n";
         outs().flush();
       }
-      demangle(Line);
+      msDemangle(Line);
       outs() << "\n";
     }
   } else {
     for (StringRef S : Symbols) {
       outs() << S << "\n";
       outs().flush();
-      demangle(S);
+      msDemangle(S);
       outs() << "\n";
     }
   }
 
   return 0;
 }

llvm/trunk/unittests/Demangle/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Demangle
   Support
 )
 
 add_llvm_unittest(DemangleTests
+  DemangleTest.cpp
   ItaniumDemangleTest.cpp
   PartialDemangleTest.cpp
 )

llvm/trunk/unittests/Demangle/DemangleTest.cpp

+//===-- DemangleTest.cpp --------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Demangle/Demangle.h"
+#include "gmock/gmock.h"
+
+using namespace llvm;
+
+TEST(Demangle, demangleTest) {
+  EXPECT_EQ(demangle("_Z3fooi"), "foo(int)");
+  EXPECT_EQ(demangle("?foo@@YAXH@Z"), "void __cdecl foo(int)");
+  EXPECT_EQ(demangle("foo"), "foo");
+}