[demangler] Rust demangler buffer reuse

[urnathan]

Summary

The rust demangler has some odd buffer handling code, which will copy the demangled string into the provided buffer, if it will fit. Otherwise it uses the allocated buffer it made. But the length of the incoming buffer will have come from a previous call, which was the length of the demangled string -- not the buffer size. And of course, we're unconditionally allocating a temporary buffer in the first place. So we don't actually get buffer reuse, and we get a memcpy in somecases.

However, nothing in LLVM ever passes in a non-null pointer. Neither does anything pass in a status pointer, that is then made use of. The only exercise these have is in the test suite.

So let's just make the rust demangler have the same API as the dlang demangler.

[this is part of a series trying to clean up the demangler's APIs]

[tmiasko]

The API thus far was intended to match that of __cxa_demangle, where the caller retains the ownership of the provided buffer when demangling fails. This is no longer the case in the new implementation. I would rather avoid diverging from __cxa_demangle in such a subtle way.

The Itanium demangler gets away with the direct use of the buffer because it separates fallible parsing from infallible printing.

the length of the incoming buffer will have come from a previous call, which was the length of the demangled string

I think this is a bug in the implementation. The *N should have been updated to reflect allocated memory size. From Demangler API in Itanium C++ ABI:

In either case, the new buffer size will be stored in *n.

[urnathan]

In D123420#3440296, @tmiasko wrote:

The API thus far was intended to match that of __cxa_demangle, where the caller retains the ownership of the provided buffer when demangling fails. This is no longer the case in the new implementation. I would rather avoid diverging from __cxa_demangle in such a subtle way.

Ah, it took me a while to figure out where that's happening, I see it now. Awkward. I suppose the rust demangler could always return the new buffer (and free any incoming buffer) in the success case? Something like if (Buf) std::free(Buf); instead of that memcpy/free dance?

The Itanium demangler gets away with the direct use of the buffer because it separates fallible parsing from infallible printing.

Any thoughts about doing similar in the Rust demangler -- passing an output buffer reference around or something?

the length of the incoming buffer will have come from a previous call, which was the length of the demangled string

I think this is a bug in the implementation. The *N should have been updated to reflect allocated memory size. From Demangler API in Itanium C++ ABI:

In either case, the new buffer size will be stored in *n.

Indeed, that's the direction I'm trying to go in :)

FWIW, this is part of a series trying to make buffer ownership transfer clearer (and other issues)

[tmiasko]

In D123420#3442384, @urnathan wrote:

In D123420#3440296, @tmiasko wrote:

The API thus far was intended to match that of __cxa_demangle, where the caller retains the ownership of the provided buffer when demangling fails. This is no longer the case in the new implementation. I would rather avoid diverging from __cxa_demangle in such a subtle way.

Ah, it took me a while to figure out where that's happening, I see it now. Awkward. I suppose the rust demangler could always return the new buffer (and free any incoming buffer) in the success case? Something like if (Buf) std::free(Buf); instead of that memcpy/free dance?

Sounds good. At that point we might remove N and Buf parameters altogether? This functionality is not used anywhere in LLVM.

[urnathan]

In D123420#3442902, @tmiasko wrote:

Sounds good. At that point we might remove N and Buf parameters altogether? This functionality is not used anywhere in LLVM.

good point, updated to do exactly that.

[tmiasko]

LGTM. Thanks!

lldb/source/Core/Mangled.cpp

if (Log *log = GetLog(LLDBLog::Demangle)) {
     else
       LLDB_LOGF(log, "demangled itanium: %s -> error: failed to demangle", M);
   }
 
   return demangled_cstr;
 }
 
 static char *GetRustV0DemangledStr(const char *M) {
-  char *demangled_cstr = llvm::rustDemangle(M, nullptr, nullptr, nullptr);
+  char *demangled_cstr = llvm::rustDemangle(M);
 
   if (Log *log = GetLog(LLDBLog::Demangle)) {
     if (demangled_cstr && demangled_cstr[0])
       LLDB_LOG(log, "demangled rustv0: {0} -> \"{1}\"", M, demangled_cstr);
     else
       LLDB_LOG(log, "demangled rustv0: {0} -> error: failed to demangle", M);
   }
 

llvm/include/llvm/Demangle/Demangle.h

 /// *n_buf stores the size of buf on input if buf is non-nullptr, and it
 /// receives the size of the demangled string on output if n_buf is not nullptr.
 /// status receives one of the demangle_ enum entries above if it's not nullptr.
 /// Flags controls various details of the demangled representation.
 char *microsoftDemangle(const char *mangled_name, size_t *n_read, char *buf,
                         size_t *n_buf, int *status,
                         MSDemangleFlags Flags = MSDF_None);
 
-// Demangles a Rust v0 mangled symbol. The API follows that of __cxa_demangle.
-char *rustDemangle(const char *MangledName, char *Buf, size_t *N, int *Status);
+// Demangles a Rust v0 mangled symbol.
+char *rustDemangle(const char *MangledName);
 
 // Demangles a D mangled symbol.
 char *dlangDemangle(const char *MangledName);
 
 /// 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

llvm/lib/Demangle/Demangle.cpp

std::string llvm::demangle(const std::string &MangledName) {
   return MangledName;
 }
 
 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) {
   char *Demangled = nullptr;
   if (isItaniumEncoding(MangledName))
     Demangled = itaniumDemangle(MangledName, nullptr, nullptr, nullptr);
   else if (isRustEncoding(MangledName))
-    Demangled = rustDemangle(MangledName, nullptr, nullptr, nullptr);
+    Demangled = rustDemangle(MangledName);
   else if (isDLangEncoding(MangledName))
     Demangled = dlangDemangle(MangledName);
 
   if (!Demangled)
     return false;
 
   Result = Demangled;
   std::free(Demangled);
   return true;
 }

llvm/lib/Demangle/RustDemangle.cpp

private:
   bool consumeIf(char Prefix);
 
   bool addAssign(uint64_t &A, uint64_t B);
   bool mulAssign(uint64_t &A, uint64_t B);
 };
 
 } // namespace
 
-char *llvm::rustDemangle(const char *MangledName, char *Buf, size_t *N,
-                         int *Status) {
-  if (MangledName == nullptr || (Buf != nullptr && N == nullptr)) {
-    if (Status != nullptr)
-      *Status = demangle_invalid_args;
+char *llvm::rustDemangle(const char *MangledName) {
+  if (MangledName == nullptr)
     return nullptr;
-  }
 
   // Return early if mangled name doesn't look like a Rust symbol.
   StringView Mangled(MangledName);
-  if (!Mangled.startsWith("_R")) {
-    if (Status != nullptr)
-      *Status = demangle_invalid_mangled_name;
+  if (!Mangled.startsWith("_R"))
     return nullptr;
-  }
 
   Demangler D;
-  if (!initializeOutputBuffer(nullptr, nullptr, D.Output, 1024)) {
-    if (Status != nullptr)
-      *Status = demangle_memory_alloc_failure;
+  if (!initializeOutputBuffer(nullptr, nullptr, D.Output, 1024))
     return nullptr;
-  }
 
   if (!D.demangle(Mangled)) {
-    if (Status != nullptr)
-      *Status = demangle_invalid_mangled_name;
     std::free(D.Output.getBuffer());
     return nullptr;
   }
 
   D.Output += '\0';
-  char *Demangled = D.Output.getBuffer();
-  size_t DemangledLen = D.Output.getCurrentPosition();
-
-  if (Buf != nullptr) {
-    if (DemangledLen <= *N) {
-      std::memcpy(Buf, Demangled, DemangledLen);
-      std::free(Demangled);
-      Demangled = Buf;
-    } else {
-      std::free(Buf);
-    }
-  }
-
-  if (N != nullptr)
-    *N = DemangledLen;
-
-  if (Status != nullptr)
-    *Status = demangle_success;
 
-  return Demangled;
+  return D.Output.getBuffer();
 }
 
 Demangler::Demangler(size_t MaxRecursionLevel)
     : MaxRecursionLevel(MaxRecursionLevel) {}
 
 static inline bool isDigit(const char C) { return '0' <= C && C <= '9'; }
 
 static inline bool isHexDigit(const char C) {

llvm/tools/llvm-rust-demangle-fuzzer/llvm-rust-demangle-fuzzer.cpp

 //===--- llvm-demangle-fuzzer.cpp - Fuzzer for the Rust Demangler ---------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Demangle/Demangle.h"
 #include <cstdint>
 #include <cstdlib>
 #include <string>
 
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
   std::string NullTerminatedString((const char *)Data, Size);
-  int Status = 0;
-  char *Demangled = llvm::rustDemangle(NullTerminatedString.c_str(), nullptr,
-                                       nullptr, &Status);
+  char *Demangled = llvm::rustDemangle(NullTerminatedString.c_str());
   std::free(Demangled);
   return 0;
 }

llvm/unittests/Demangle/RustDemangleTest.cpp

 //===------------------ RustDemangleTest.cpp ------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Demangle/Demangle.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include <cstdlib>
 
 TEST(RustDemangle, Success) {
-  char *Demangled =
-      llvm::rustDemangle("_RNvC1a4main", nullptr, nullptr, nullptr);
-  EXPECT_STREQ(Demangled, "a::main");
-  std::free(Demangled);
-
-  // With status.
-  int Status = 0;
-  Demangled = llvm::rustDemangle("_RNvC1a4main", nullptr, nullptr, &Status);
-  EXPECT_EQ(Status, llvm::demangle_success);
-  EXPECT_STREQ(Demangled, "a::main");
-  std::free(Demangled);
-
-  // With status and length.
-  size_t N = 0;
-  Demangled = llvm::rustDemangle("_RNvC1a4main", nullptr, &N, &Status);
-  EXPECT_EQ(Status, llvm::demangle_success);
-  EXPECT_EQ(N, 8u);
+  char *Demangled = llvm::rustDemangle("_RNvC1a4main");
   EXPECT_STREQ(Demangled, "a::main");
   std::free(Demangled);
 }
 
 TEST(RustDemangle, Invalid) {
-  int Status = 0;
   char *Demangled = nullptr;
 
   // Invalid prefix.
-  Demangled = llvm::rustDemangle("_ABCDEF", nullptr, nullptr, &Status);
-  EXPECT_EQ(Status, llvm::demangle_invalid_mangled_name);
+  Demangled = llvm::rustDemangle("_ABCDEF");
   EXPECT_EQ(Demangled, nullptr);
 
   // Correct prefix but still invalid.
-  Demangled = llvm::rustDemangle("_RRR", nullptr, nullptr, &Status);
-  EXPECT_EQ(Status, llvm::demangle_invalid_mangled_name);
+  Demangled = llvm::rustDemangle("_RRR");
   EXPECT_EQ(Demangled, nullptr);
 }
-
-TEST(RustDemangle, OutputBufferWithoutLength) {
-  char *Buffer = static_cast<char *>(std::malloc(1024));
-  ASSERT_NE(Buffer, nullptr);
-
-  int Status = 0;
-  char *Demangled =
-      llvm::rustDemangle("_RNvC1a4main", Buffer, nullptr, &Status);
-
-  EXPECT_EQ(Status, llvm::demangle_invalid_args);
-  EXPECT_EQ(Demangled, nullptr);
-  std::free(Buffer);
-}
-
-TEST(RustDemangle, OutputBuffer) {
-  size_t N = 1024;
-  char *Buffer = static_cast<char *>(std::malloc(N));
-  ASSERT_NE(Buffer, nullptr);
-
-  int Status = 0;
-  char *Demangled = llvm::rustDemangle("_RNvC1a4main", Buffer, &N, &Status);
-
-  EXPECT_EQ(Status, llvm::demangle_success);
-  EXPECT_EQ(Demangled, Buffer);
-  EXPECT_STREQ(Demangled, "a::main");
-  std::free(Demangled);
-}
-
-TEST(RustDemangle, SmallOutputBuffer) {
-  size_t N = 1;
-  char *Buffer = static_cast<char *>(std::malloc(N));
-  ASSERT_NE(Buffer, nullptr);
-
-  int Status = 0;
-  char *Demangled = llvm::rustDemangle("_RNvC1a4main", Buffer, &N, &Status);
-
-  EXPECT_EQ(Status, llvm::demangle_success);
-  EXPECT_STREQ(Demangled, "a::main");
-  std::free(Demangled);
-}