lld-link: print demangled symbol names for "undefined symbol" diagnostics

[thakis]

Summary

For this, add a few toString() calls when printing the "undefined symbol" diagnostics; toString() already does demangling on Windows hosts.

Also make lld::demangleMSVC() (called by toString(Symbol*)) call LLVM's microsoftDemangle() instead of UnDecorateSymbolName() so that it works on non-Windows hosts – this makes both updating tests easier and provides a better user experience for people doing cross-links.

Update microsoftDemangle() to work more like itaniumDemangle() after which appears to be modeled, so that lld::demangleItanium() and lld::demangleMSVC() can look alike:

• Use same method of initializing the output stream and its buffer
• Allow a nullptr Status pointer
• Don't print the mangled name on demangling error
• Write to N (if it is non-nullptr)

This doesn't yet do the right thing for symbols starting with __imp_, but that can be improved in a follow-up.

[ruiu]

libcxxabi/src/demangle/Utility.h

173 ↗

(On Diff #165529)

Looks like there are two patterns how to use this function -- one way is to pass nullptr, nullptr as first two arguments, and the other is to pass an actual buffer. Can you separate that as two functions?

It looks like the latter use case is used only once, so you might be able to inline it.

lld/COFF/SymbolTable.cpp

69 ↗

(On Diff #165529)

Update comment.

84–86 ↗

(On Diff #165529)

You can just return Candidate.

[erik.pilkington]

libcxxabi/src/demangle/Utility.h

173 ↗

(On Diff #165529)

There are actually many uses in the LLVM version, see llvm/lib/Demangle/ItaniumDemangle.cpp. Personally, I like it as a single function that encapsulates the buffer initialization for __cxa_demangle. I agree its a bit awkward, but thats just the API that it's trying to implement.

[ruiu]

libcxxabi/src/demangle/Utility.h

173 ↗

(On Diff #165529)

Oh okay, never mind. If this function is being used in many places, it isn't worth to fix all caller places anyway.

179 ↗

(On Diff #165529)

Perhaps off-topic, but I'd perhaps call std::terminate() instead of returning true for memory exhaustion error, as we can't really handle such error situation in most cases.

[thakis]

ruiu comments

[thakis]

Thanks!

libcxxabi/src/demangle/Utility.h

179 ↗

(On Diff #165529)

This code is identical in lib/Demangle and in libcxxabi. In libcxxabi this is used to implement __cxa_demangle
which commands that an error code is returned on allocation failure: https://gcc.gnu.org/onlinedocs/libstdc++/libstdc++-html-USERS-4.3/a01696.html

I agree that regular client software can't do anything useful on oom, but this is dictated by an external api contract.

lld/COFF/SymbolTable.cpp

84–86 ↗

(On Diff #165529)

Err, yes :-D Done.

[zturner]

llvm/lib/Demangle/MicrosoftDemangle.cpp

1020–1022 ↗

(On Diff #165555)

Is this returning true if it fails? Gross. Can we make it return false if it fails?

[thakis]

llvm/lib/Demangle/MicrosoftDemangle.cpp

1020–1022 ↗

(On Diff #165555)

Returning true on error is reasonably common in the codebase: http://llvm-cs.pcc.me.uk/?q=true+on+error (e.g. https://reviews.llvm.org/D45898#inline-454138) I too wish it wasn't so.

(I'm also calling an existing function here, so I'd prefer to not change all the existing callers.)

[erik.pilkington]

llvm/lib/Demangle/MicrosoftDemangle.cpp

1020–1022 ↗

(On Diff #165555)

I think LLVM style is true on error, aka unix style. I've seen a lot of both though. I don't see any mention in the coding style guide, but I think this would be a really good thing to have there. Maybe we should start a bikeshed on llvm-dev.

[ruiu]

LGTM

We can discuss whether we should return true or false for success, but this change itself doesn't change the semantics of existing code, and it seems like a pure improvement, so LGTM.

[zturner]

llvm/lib/Demangle/MicrosoftDemangle.cpp

1020–1022 ↗

(On Diff #165555)

FWIW I think we should fix all of these, and in the past we've fixed instances of it on the grounds that it's bad form. The case of DiagnoseUseOfDecl is slightly more defensible because the presence of Diagnose sort of hints that it the function returns true, something is wrong. (I'd still change it or rename the function though). That said, it's true you are calling an existing function, but previously that function was only called 6 times, now it is called 11 times. I would prefer not to propagate this confusing usage pattern because it makes the code demonstrably worse imo.

[thakis]

Thanks! Submitting...

llvm/lib/Demangle/MicrosoftDemangle.cpp

1020–1022 ↗

(On Diff #165555)

I'll land this as-is since I'm just moving an existing function around, and then I'll send another patch for inverting the meaning of the return value and we'll see if we can convince Erik over there. I see where "unix style" comes from, but that's imho more convincing for functions returning int :-)

[thakis]

Also https://reviews.llvm.org/rL342331 and https://reviews.llvm.org/rLLD342332

Do I have to do something special to make phab auto-display multiple revisions per review? I put "Differential Revison: <this review" in all three commit descriptions.

[thakis]

Ah, looks like all changes are mentioned at the top, just not as comments further down.

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

OutputStream(char *StartBuf, size_t Size)
       : Buffer(StartBuf), CurrentPosition(0), BufferCapacity(Size) {}
   OutputStream() = default;
   void reset(char *Buffer_, size_t BufferCapacity_) {
     CurrentPosition = 0;
     Buffer = Buffer_;
     BufferCapacity = BufferCapacity_;
   }
 
-  /// Create an OutputStream from a buffer and a size.  If either of these are
-  /// null a buffer is allocated.
-  static OutputStream create(char *StartBuf, size_t *Size, size_t AllocSize) {
-    OutputStream Result;
-
-    if (!StartBuf || !Size) {
-      StartBuf = static_cast<char *>(std::malloc(AllocSize));
-      if (StartBuf == nullptr)
-        std::terminate();
-      Size = &AllocSize;
-    }
-
-    Result.reset(StartBuf, *Size);
-    return Result;
-  }
-
   /// If a ParameterPackExpansion (or similar type) is encountered, the offset
   /// into the pack that we're currently printing.
   unsigned CurrentPackIndex = std::numeric_limits<unsigned>::max();
   unsigned CurrentPackMax = std::numeric_limits<unsigned>::max();
 
   OutputStream &operator+=(StringView R) {
     size_t Size = R.size();
     if (Size == 0)
void restoreNow(bool Force) {
     Restore = std::move(OriginalValue);
     ShouldRestore = false;
   }
 
   SwapAndRestore(const SwapAndRestore &) = delete;
   SwapAndRestore &operator=(const SwapAndRestore &) = delete;
 };
 
+inline bool initializeOutputStream(char *Buf, size_t *N, OutputStream &S,
+                                   size_t InitSize) {
+  size_t BufferSize;
+  if (Buf == nullptr) {
+    Buf = static_cast<char *>(std::malloc(InitSize));
+    if (Buf == nullptr)
+      return true;
+    BufferSize = InitSize;
+  } else
+    BufferSize = *N;
+
+  S.reset(Buf, BufferSize);
+  return false;
+}
+
 #endif

llvm/trunk/lib/Demangle/ItaniumDemangle.cpp

template<typename T, typename ...Args> T *makeNode(Args &&...args) {
     return new (Alloc.allocate(sizeof(T)))
         T(std::forward<Args>(args)...);
   }
 
   void *allocateNodeArray(size_t sz) {
     return Alloc.allocate(sizeof(Node *) * sz);
   }
 };
-
-bool initializeOutputStream(char *Buf, size_t *N, OutputStream &S,
-                            size_t InitSize) {
-  size_t BufferSize;
-  if (Buf == nullptr) {
-    Buf = static_cast<char *>(std::malloc(InitSize));
-    if (Buf == nullptr)
-      return true;
-    BufferSize = InitSize;
-  } else
-    BufferSize = *N;
-
-  S.reset(Buf, BufferSize);
-  return false;
-}
 }  // unnamed namespace
 
 //===----------------------------------------------------------------------===//
 // Code beyond this point should not be synchronized with libc++abi.
 //===----------------------------------------------------------------------===//
 
 using Demangler = itanium_demangle::Db<DefaultAllocator>;
 

llvm/trunk/lib/Demangle/MicrosoftDemangle.cpp

NamedIdentifierNode *Demangler::demangleBackRefName(StringView &MangledName) {
 
   MangledName = MangledName.dropFront();
   return Backrefs.Names[I];
 }
 
 void Demangler::memorizeIdentifier(IdentifierNode *Identifier) {
   // Render this class template name into a string buffer so that we can
   // memorize it for the purpose of back-referencing.
-  OutputStream OS = OutputStream::create(nullptr, nullptr, 1024);
+  OutputStream OS;
+  if (initializeOutputStream(nullptr, nullptr, OS, 1024))
+    // FIXME: Propagate out-of-memory as an error?
+    std::terminate();
   Identifier->output(OS, OF_Default);
   OS << '\0';
   char *Name = OS.getBuffer();
 
   StringView Owned = copyString(Name);
   memorizeString(Owned);
   std::free(Name);
 }
Demangler::demangleStringLiteral(StringView &MangledName) {
   CrcEndPos = MangledName.find('@');
   if (CrcEndPos == StringView::npos)
     goto StringLiteralError;
   CRC = MangledName.substr(0, CrcEndPos);
   MangledName = MangledName.dropFront(CrcEndPos + 1);
   if (MangledName.empty())
     goto StringLiteralError;
 
-  OS = OutputStream::create(nullptr, nullptr, 1024);
+  if (initializeOutputStream(nullptr, nullptr, OS, 1024))
+    // FIXME: Propagate out-of-memory as an error?
+    std::terminate();
   if (IsWcharT) {
     Result->Char = CharKind::Wchar;
     if (StringByteSize > 64)
       Result->IsTruncated = true;
 
     while (!MangledName.consumeFront('@')) {
       assert(StringByteSize >= 2);
       wchar_t W = demangleWcharLiteral(MangledName);
Demangler::demangleLocallyScopedNamePiece(StringView &MangledName) {
   MangledName.consumeFront('?');
 
   assert(!Error);
   Node *Scope = parse(MangledName);
   if (Error)
     return nullptr;
 
   // Render the parent symbol's name into a buffer.
-  OutputStream OS = OutputStream::create(nullptr, nullptr, 1024);
+  OutputStream OS;
+  if (initializeOutputStream(nullptr, nullptr, OS, 1024))
+    // FIXME: Propagate out-of-memory as an error?
+    std::terminate();
   OS << '`';
   Scope->output(OS, OF_Default);
   OS << '\'';
   OS << "::`" << Number.first << "'";
   OS << '\0';
   char *Result = OS.getBuffer();
   Identifier->Name = copyString(Result);
   std::free(Result);
Demangler::demangleTemplateParameterList(StringView &MangledName) {
   return nullptr;
 }
 
 void Demangler::dumpBackReferences() {
   std::printf("%d function parameter backreferences\n",
               (int)Backrefs.FunctionParamCount);
 
   // Create an output stream so we can render each type.
-  OutputStream OS = OutputStream::create(nullptr, 0, 1024);
+  OutputStream OS;
+  if (initializeOutputStream(nullptr, nullptr, OS, 1024))
+    std::terminate();
   for (size_t I = 0; I < Backrefs.FunctionParamCount; ++I) {
     OS.setCurrentPosition(0);
 
     TypeNode *T = Backrefs.FunctionParams[I];
     T->output(OS, OF_Default);
 
     std::printf("  [%d] - %.*s\n", (int)I, (int)OS.getCurrentPosition(),
                 OS.getBuffer());
   }
   std::free(OS.getBuffer());
 
   if (Backrefs.FunctionParamCount > 0)
     std::printf("\n");
   std::printf("%d name backreferences\n", (int)Backrefs.NamesCount);
   for (size_t I = 0; I < Backrefs.NamesCount; ++I) {
     std::printf("  [%d] - %.*s\n", (int)I, (int)Backrefs.Names[I]->Name.size(),
                 Backrefs.Names[I]->Name.begin());
   }
   if (Backrefs.NamesCount > 0)
     std::printf("\n");
 }
 
 char *llvm::microsoftDemangle(const char *MangledName, char *Buf, size_t *N,
                               int *Status, MSDemangleFlags Flags) {
+  int InternalStatus = demangle_success;
   Demangler D;
+  OutputStream S;
+
   StringView Name{MangledName};
-  SymbolNode *S = D.parse(Name);
+  SymbolNode *AST = D.parse(Name);
 
   if (Flags & MSDF_DumpBackrefs)
     D.dumpBackReferences();
-  OutputStream OS = OutputStream::create(Buf, N, 1024);
-  if (D.Error) {
-    OS << MangledName;
-    *Status = llvm::demangle_invalid_mangled_name;
-  } else {
-    S->output(OS, OF_Default);
-    *Status = llvm::demangle_success;
+
+  if (D.Error)
+    InternalStatus = demangle_invalid_mangled_name;
+  else if (initializeOutputStream(Buf, N, S, 1024))
+    InternalStatus = demangle_memory_alloc_failure;
+  else {
+    AST->output(S, OF_Default);
+    S += '\0';
+    if (N != nullptr)
+      *N = S.getCurrentPosition();
+    Buf = S.getBuffer();
   }
 
-  OS << '\0';
-  return OS.getBuffer();
+  if (Status)
+    *Status = InternalStatus;
+  return InternalStatus == demangle_success ? Buf : nullptr;
 }