[demangler] Simplify OutputBuffer initialization

[urnathan]

Summary

This next cleanup of the demangler's OutputBuffer class addresses the initial allocation of the output buffer. Now that the buffer extension logic has hysteresis in it, there's no need for every use of OutputBuffer to explicitly create the initial buffer (using either 128 or 1024 as initial guesses).

For the default case we can rely on the buffer extension logic to create a good initial buffer. For the cases where the user is reusing a buffer, pass that information into the constructor. Thus the OutputBuffer's ownership of the buffer starts at its own lifetime start -- a nice bit of RAII. We can reduce the lifetime of this object in several cases.

That new constructor takes a 'size_t *' for the size argument, as all uses with a non-null buffer are passing through a malloc'd buffer from their own caller in this manner.

The buffer reset member function is never used, and is deleted.

The original buffer initialization code would return a failure code if that first malloc failed. Existing code either ignored that, called std::terminate with a FIXME, or returned an error code.

But that's not foolproof anyway, as a subsequent buffer extension failure ends up calling std::terminate. I am working on addressing that unfortunate failure mode in a manner more consistent with the C++ ABI design.

I have a patch dealing with the destructing part of this API coming up.

[urnathan]

rebase

[urnathan]

re-rebase

[urnathan]

Tomasz, your comments on the rust demangler were most helpful. Perhaps you have bandwidth to look at this?

[urnathan]

Rebased ontop of D123420

[dblaikie]

Sounds OK to me

[kstoimenov]

Looks like it night have broken a sanitizer build: https://lab.llvm.org/buildbot/#/builders/5/builds/22738

[urnathan]

In D122604#3475016, @kstoimenov wrote:

Looks like it night have broken a sanitizer build: https://lab.llvm.org/buildbot/#/builders/5/builds/22738

Yes, I got a note about that. Fix will be available shortly.

[urnathan]

rebased an updated to avoid the UB discovered in the earlier version.

I went with @dblaikie's suggestion of detecting the UB at point of use. This makes D124524 moot.

[dblaikie]

llvm/lib/Demangle/MicrosoftDemangle.cpp

249–253

Looks good to me as an alternative to D124524

llvm/unittests/Demangle/OutputBufferTest.cpp

19–21 ↗

(On Diff #426394)

I don't think this check is necessary though, is it? nullptr,nullptr should probably be valid iterators, yeah?

[urnathan]

Removed the OutputBufferTest change. I had thought iterators could never be null, but I see I was mistaken.

[urnathan]

rebase, hopefully fix the CI tester

[urnathan]

rebase

[urnathan]

hm, let's try that rebase again

[tmiasko]

Looks good.

Nit: reviewing from the perspective of OutputBuffer starting empty, the OutputBuffer::prepend might need an extra check to avoid passing nullptr to memmove / memcpy, in an unusual case an empty string were to be prepended to an empty output buffer.

As far as CI is concerned, if you are logged in the Phabricator, then following completed remote builds in B164300: Diff 429222 -> pre-merge checks -> plain text URL https://buildkite.com/llvm-project/diff-checks/builds/104149 shows:

Attention! D124524 is one of the dependencies of the target revision D122604.
No testing is possible because we couldn't apply the patch.

Looks like there is now a stale parent revision D124524 and patch fails to apply.

[urnathan]

ah, thanks for the clue!

[urnathan]

ping?

[dblaikie]

Looks good

[urnathan]

how can I get libc++abi review here?

[dblaikie]

@ldionne / @compnerd perhaps one of you could take a quick look?

[urnathan]

ping pingedy ping?

[dblaikie]

I think it's OK to go ahead with this without further approval - the phab marker here is more of a guideline than a rule, I believe. This isn't breaking/touching any part of the public API, etc, so is probably less in need of scrutiny.

libcxxabi/src/cxa_demangle.cpp

__cxa_demangle(const char *MangledName, char *Buf, size_t *N, int *Status) {
   if (MangledName == nullptr || (Buf != nullptr && N == nullptr)) {
     if (Status)
       *Status = demangle_invalid_args;
     return nullptr;
   }
 
   int InternalStatus = demangle_success;
   Demangler Parser(MangledName, MangledName + std::strlen(MangledName));
-  OutputBuffer O;
-
   Node *AST = Parser.parse();
 
   if (AST == nullptr)
     InternalStatus = demangle_invalid_mangled_name;
-  else if (!initializeOutputBuffer(Buf, N, O, 1024))
-    InternalStatus = demangle_memory_alloc_failure;
   else {
+    OutputBuffer O(Buf, N);
     assert(Parser.ForwardTemplateRefs.empty());
     AST->print(O);
     O += '\0';
     if (N != nullptr)
       *N = O.getCurrentPosition();
     Buf = O.getBuffer();
   }
 
   if (Status)
     *Status = InternalStatus;
   return InternalStatus == demangle_success ? Buf : nullptr;
 }
 }  // __cxxabiv1

libcxxabi/src/demangle/Utility.h

OutputBuffer &writeUnsigned(uint64_t N, bool isNeg = false) {
     if (isNeg)
       *--TempPtr = '-';
 
     return operator+=(StringView(TempPtr, Temp.data() + Temp.size()));
   }
 
 public:
   OutputBuffer(char *StartBuf, size_t Size)
-      : Buffer(StartBuf), CurrentPosition(0), BufferCapacity(Size) {}
+      : Buffer(StartBuf), BufferCapacity(Size) {}
+  OutputBuffer(char *StartBuf, size_t *SizePtr)
+      : OutputBuffer(StartBuf, StartBuf ? *SizePtr : 0) {}
   OutputBuffer() = default;
   // Non-copyable
   OutputBuffer(const OutputBuffer &) = delete;
   OutputBuffer &operator=(const OutputBuffer &) = delete;
 
   operator StringView() const { return StringView(Buffer, CurrentPosition); }
 
-  void reset(char *Buffer_, size_t BufferCapacity_) {
-    CurrentPosition = 0;
-    Buffer = Buffer_;
-    BufferCapacity = BufferCapacity_;
-  }
-
   /// 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();
 
   /// When zero, we're printing template args and '>' needs to be parenthesized.
   /// Use a counter so we can simply increment inside parentheses.
   unsigned GtIsGt = 1;
ScopedOverride(T &Loc_, T NewVal) : Loc(Loc_), Original(Loc_) {
     Loc_ = std::move(NewVal);
   }
   ~ScopedOverride() { Loc = std::move(Original); }
 
   ScopedOverride(const ScopedOverride &) = delete;
   ScopedOverride &operator=(const ScopedOverride &) = delete;
 };
 
-inline bool initializeOutputBuffer(char *Buf, size_t *N, OutputBuffer &OB,
-                                   size_t InitSize) {
-  size_t BufferSize;
-  if (Buf == nullptr) {
-    Buf = static_cast<char *>(std::malloc(InitSize));
-    if (Buf == nullptr)
-      return false;
-    BufferSize = InitSize;
-  } else
-    BufferSize = *N;
-
-  OB.reset(Buf, BufferSize);
-  return true;
-}
-
 DEMANGLE_NAMESPACE_END
 
 #endif

llvm/include/llvm/Demangle/Utility.h

OutputBuffer &writeUnsigned(uint64_t N, bool isNeg = false) {
     if (isNeg)
       *--TempPtr = '-';
 
     return operator+=(StringView(TempPtr, Temp.data() + Temp.size()));
   }
 
 public:
   OutputBuffer(char *StartBuf, size_t Size)
-      : Buffer(StartBuf), CurrentPosition(0), BufferCapacity(Size) {}
+      : Buffer(StartBuf), BufferCapacity(Size) {}
+  OutputBuffer(char *StartBuf, size_t *SizePtr)
+      : OutputBuffer(StartBuf, StartBuf ? *SizePtr : 0) {}
   OutputBuffer() = default;
   // Non-copyable
   OutputBuffer(const OutputBuffer &) = delete;
   OutputBuffer &operator=(const OutputBuffer &) = delete;
 
   operator StringView() const { return StringView(Buffer, CurrentPosition); }
 
-  void reset(char *Buffer_, size_t BufferCapacity_) {
-    CurrentPosition = 0;
-    Buffer = Buffer_;
-    BufferCapacity = BufferCapacity_;
-  }
-
   /// 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();
 
   /// When zero, we're printing template args and '>' needs to be parenthesized.
   /// Use a counter so we can simply increment inside parentheses.
   unsigned GtIsGt = 1;
ScopedOverride(T &Loc_, T NewVal) : Loc(Loc_), Original(Loc_) {
     Loc_ = std::move(NewVal);
   }
   ~ScopedOverride() { Loc = std::move(Original); }
 
   ScopedOverride(const ScopedOverride &) = delete;
   ScopedOverride &operator=(const ScopedOverride &) = delete;
 };
 
-inline bool initializeOutputBuffer(char *Buf, size_t *N, OutputBuffer &OB,
-                                   size_t InitSize) {
-  size_t BufferSize;
-  if (Buf == nullptr) {
-    Buf = static_cast<char *>(std::malloc(InitSize));
-    if (Buf == nullptr)
-      return false;
-    BufferSize = InitSize;
-  } else
-    BufferSize = *N;
-
-  OB.reset(Buf, BufferSize);
-  return true;
-}
-
 DEMANGLE_NAMESPACE_END
 
 #endif

llvm/lib/Demangle/DLangDemangle.cpp

const char *Demangler::parseMangle(OutputBuffer *Demangled) {
   return parseMangle(Demangled, this->Str);
 }
 
 char *llvm::dlangDemangle(const char *MangledName) {
   if (MangledName == nullptr || strncmp(MangledName, "_D", 2) != 0)
     return nullptr;
 
   OutputBuffer Demangled;
-  if (!initializeOutputBuffer(nullptr, nullptr, Demangled, 1024))
-    return nullptr;
-
   if (strcmp(MangledName, "_Dmain") == 0) {
     Demangled << "D main";
   } else {
 
     Demangler D = Demangler(MangledName);
     MangledName = D.parseMangle(&Demangled);
 
     // Check that the entire symbol was successfully demangled.

llvm/lib/Demangle/ItaniumDemangle.cpp

char *llvm::itaniumDemangle(const char *MangledName, char *Buf,
   if (MangledName == nullptr || (Buf != nullptr && N == nullptr)) {
     if (Status)
       *Status = demangle_invalid_args;
     return nullptr;
   }
 
   int InternalStatus = demangle_success;
   Demangler Parser(MangledName, MangledName + std::strlen(MangledName));
-  OutputBuffer OB;
-
   Node *AST = Parser.parse();
 
   if (AST == nullptr)
     InternalStatus = demangle_invalid_mangled_name;
-  else if (!initializeOutputBuffer(Buf, N, OB, 1024))
-    InternalStatus = demangle_memory_alloc_failure;
   else {
+    OutputBuffer OB(Buf, N);
     assert(Parser.ForwardTemplateRefs.empty());
     AST->print(OB);
     OB += '\0';
     if (N != nullptr)
       *N = OB.getCurrentPosition();
     Buf = OB.getBuffer();
   }
 
bool ItaniumPartialDemangler::partialDemangle(const char *MangledName) {
   Demangler *Parser = static_cast<Demangler *>(Context);
   size_t Len = std::strlen(MangledName);
   Parser->reset(MangledName, MangledName + Len);
   RootNode = Parser->parse();
   return RootNode == nullptr;
 }
 
 static char *printNode(const Node *RootNode, char *Buf, size_t *N) {
-  OutputBuffer OB;
-  if (!initializeOutputBuffer(Buf, N, OB, 128))
-    return nullptr;
+  OutputBuffer OB(Buf, N);
   RootNode->print(OB);
   OB += '\0';
   if (N != nullptr)
     *N = OB.getCurrentPosition();
   return OB.getBuffer();
 }
 
 char *ItaniumPartialDemangler::getFunctionBaseName(char *Buf, size_t *N) const {

 }
 
 char *ItaniumPartialDemangler::getFunctionDeclContextName(char *Buf,
                                                           size_t *N) const {
   if (!isFunction())
     return nullptr;
   const Node *Name = static_cast<const FunctionEncoding *>(RootNode)->getName();
 
-  OutputBuffer OB;
-  if (!initializeOutputBuffer(Buf, N, OB, 128))
-    return nullptr;
+  OutputBuffer OB(Buf, N);
 
  KeepGoingLocalFunction:
   while (true) {
     if (Name->getKind() == Node::KAbiTagAttr) {
       Name = static_cast<const AbiTagAttr *>(Name)->Base;
       continue;
     }
     if (Name->getKind() == Node::KNameWithTemplateArgs) {

 }
 
 char *ItaniumPartialDemangler::getFunctionParameters(char *Buf,
                                                      size_t *N) const {
   if (!isFunction())
     return nullptr;
   NodeArray Params = static_cast<FunctionEncoding *>(RootNode)->getParams();
 
-  OutputBuffer OB;
-  if (!initializeOutputBuffer(Buf, N, OB, 128))
-    return nullptr;
+  OutputBuffer OB(Buf, N);
 
   OB += '(';
   Params.printWithComma(OB);
   OB += ')';
   OB += '\0';
   if (N != nullptr)
     *N = OB.getCurrentPosition();
   return OB.getBuffer();
 }
 
 char *ItaniumPartialDemangler::getFunctionReturnType(
     char *Buf, size_t *N) const {
   if (!isFunction())
     return nullptr;
 
-  OutputBuffer OB;
-  if (!initializeOutputBuffer(Buf, N, OB, 128))
-    return nullptr;
+  OutputBuffer OB(Buf, N);
 
   if (const Node *Ret =
           static_cast<const FunctionEncoding *>(RootNode)->getReturnType())
     Ret->print(OB);
 
   OB += '\0';
   if (N != nullptr)
     *N = OB.getCurrentPosition();

llvm/lib/Demangle/MicrosoftDemangle.cpp

demanglePointerCVQualifiers(StringView &MangledName) {
   }
   // This function is only called if isPointerType() returns true,
   // and it only returns true for the six cases listed above.
   DEMANGLE_UNREACHABLE;
 }
 
 StringView Demangler::copyString(StringView Borrowed) {
   char *Stable = Arena.allocUnalignedBuffer(Borrowed.size());
+  // This is not a micro-optimization, it avoids UB, should Borrowed be an null
+  // buffer.
+  if (Borrowed.size())
     std::memcpy(Stable, Borrowed.begin(), Borrowed.size());
 
   return {Stable, Borrowed.size()};
 }
 
 SpecialTableSymbolNode *
 Demangler::demangleSpecialTableSymbolNode(StringView &MangledName,
                                           SpecialIntrinsicKind K) {
   NamedIdentifierNode *NI = Arena.alloc<NamedIdentifierNode>();
   switch (K) {
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.
   OutputBuffer OB;
-  if (!initializeOutputBuffer(nullptr, nullptr, OB, 1024))
-    // FIXME: Propagate out-of-memory as an error?
-    std::terminate();
   Identifier->output(OB, OF_Default);
   StringView Owned = copyString(OB);
   memorizeString(Owned);
   std::free(OB.getBuffer());
 }
 
 IdentifierNode *
 Demangler::demangleTemplateInstantiationName(StringView &MangledName,
Demangler::demangleStringLiteral(StringView &MangledName) {
   StringView CRC;
   uint64_t StringByteSize;
   bool IsWcharT = false;
   bool IsNegative = false;
   size_t CrcEndPos = 0;
 
   EncodedStringLiteralNode *Result = Arena.alloc<EncodedStringLiteralNode>();
 
-  // Must happen before the first `goto StringLiteralError`.
-  if (!initializeOutputBuffer(nullptr, nullptr, OB, 1024))
-    // FIXME: Propagate out-of-memory as an error?
-    std::terminate();
-
   // Prefix indicating the beginning of a string literal
   if (!MangledName.consumeFront("@_"))
     goto StringLiteralError;
   if (MangledName.empty())
     goto StringLiteralError;
 
   // Char Type (regular or wchar_t)
   switch (MangledName.popFront()) {
Demangler::demangleLocallyScopedNamePiece(StringView &MangledName) {
 
   assert(!Error);
   Node *Scope = parse(MangledName);
   if (Error)
     return nullptr;
 
   // Render the parent symbol's name into a buffer.
   OutputBuffer OB;
-  if (!initializeOutputBuffer(nullptr, nullptr, OB, 1024))
-    // FIXME: Propagate out-of-memory as an error?
-    std::terminate();
   OB << '`';
   Scope->output(OB, OF_Default);
   OB << '\'';
   OB << "::`" << Number << "'";
 
   Identifier->Name = copyString(OB);
   std::free(OB.getBuffer());
   return Identifier;

 }
 
 void Demangler::dumpBackReferences() {
   std::printf("%d function parameter backreferences\n",
               (int)Backrefs.FunctionParamCount);
 
   // Create an output stream so we can render each type.
   OutputBuffer OB;
-  if (!initializeOutputBuffer(nullptr, nullptr, OB, 1024))
-    std::terminate();
   for (size_t I = 0; I < Backrefs.FunctionParamCount; ++I) {
     OB.setCurrentPosition(0);
 
     TypeNode *T = Backrefs.FunctionParams[I];
     T->output(OB, OF_Default);
 
     StringView B = OB;
     std::printf("  [%d] - %.*s\n", (int)I, (int)B.size(), B.begin());
void Demangler::dumpBackReferences() {
   if (Backrefs.NamesCount > 0)
     std::printf("\n");
 }
 
 char *llvm::microsoftDemangle(const char *MangledName, size_t *NMangled,
                               char *Buf, size_t *N,
                               int *Status, MSDemangleFlags Flags) {
   Demangler D;
-  OutputBuffer OB;
 
   StringView Name{MangledName};
   SymbolNode *AST = D.parse(Name);
   if (!D.Error && NMangled)
     *NMangled = Name.begin() - MangledName;
 
   if (Flags & MSDF_DumpBackrefs)
     D.dumpBackReferences();
 
   OutputFlags OF = OF_Default;
   if (Flags & MSDF_NoCallingConvention)
     OF = OutputFlags(OF | OF_NoCallingConvention);
   if (Flags & MSDF_NoAccessSpecifier)
     OF = OutputFlags(OF | OF_NoAccessSpecifier);
   if (Flags & MSDF_NoReturnType)
     OF = OutputFlags(OF | OF_NoReturnType);
   if (Flags & MSDF_NoMemberType)
     OF = OutputFlags(OF | OF_NoMemberType);
   if (Flags & MSDF_NoVariableType)
     OF = OutputFlags(OF | OF_NoVariableType);
 
   int InternalStatus = demangle_success;
   if (D.Error)
     InternalStatus = demangle_invalid_mangled_name;
-  else if (!initializeOutputBuffer(Buf, N, OB, 1024))
-    InternalStatus = demangle_memory_alloc_failure;
   else {
+    OutputBuffer OB(Buf, N);
     AST->output(OB, OF);
     OB += '\0';
     if (N != nullptr)
       *N = OB.getCurrentPosition();
     Buf = OB.getBuffer();
   }
 
   if (Status)
     *Status = InternalStatus;
   return InternalStatus == demangle_success ? Buf : nullptr;
 }

llvm/lib/Demangle/MicrosoftDemangleNodes.cpp

case CallingConv::SwiftAsync:
     break;
   default:
     break;
   }
 }
 
 std::string Node::toString(OutputFlags Flags) const {
   OutputBuffer OB;
-  initializeOutputBuffer(nullptr, nullptr, OB, 1024);
   this->output(OB, Flags);
   StringView SV = OB;
   std::string Owned(SV.begin(), SV.end());
   std::free(OB.getBuffer());
   return Owned;
 }
 
 void PrimitiveTypeNode::outputPre(OutputBuffer &OB, OutputFlags Flags) const {

llvm/lib/Demangle/RustDemangle.cpp

if (MangledName == nullptr)
     return nullptr;
 
   // Return early if mangled name doesn't look like a Rust symbol.
   StringView Mangled(MangledName);
   if (!Mangled.startsWith("_R"))
     return nullptr;
 
   Demangler D;
-  if (!initializeOutputBuffer(nullptr, nullptr, D.Output, 1024))
-    return nullptr;
-
   if (!D.demangle(Mangled)) {
     std::free(D.Output.getBuffer());
     return nullptr;
   }
 
   D.Output += '\0';
 
   return D.Output.getBuffer();