[Demangle] convert itaniumDemangle and nonMicrosoftDemangle to use std::string_view

[nickdesaulniers]

Summary

D149104 converted llvm::demangle to use std::string_view. Enabling
"expensive checks" (via -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON) causes
lld/test/wasm/why-extract.s to fail. The reason for this is obscure:

Reason #10007 why std::string_view is dangerous:
Consider the following pattern:

std::string_view s = ...;
const char *c = s.data();
std::strlen(c);

Is c a NUL-terminated C style string? It depends; but if it's not then
it's not safe to call std::strlen on the std::string_view::data().
std::string_view::length() should be used instead.

Fixing this fixes the one lone test that caught this.

microsoftDemangle, rustDemangle, and dlangDemangle should get this same
treatment, too. I will do that next.

[efriedma]

This doesn't make any sense. If the argument "name" is live on entry to the function, it'll remain alive until the function exit. Converting it to a string_view should maintain the same lifetime. Converting it to an std::string actually makes the lifetime shorter.

[MaskRay]

In D149675#4313265, @efriedma wrote:

This doesn't make any sense. If the argument "name" is live on entry to the function, it'll remain alive until the function exit. Converting it to a string_view should maintain the same lifetime. Converting it to an std::string actually makes the lifetime shorter.

I agree with the analysis. It's more likely a caller of maybeDemangleSymbol passes a temporary whose lifetime has ended.

Switching to string_view may expose (due to expensive checks setting _GLIBCXX_DEBUG) latent bugs.

[nickdesaulniers]

Looks like the problematic call site is writeWhyExtract() in lld/wasm/Driver.cpp. Trying to figure out why we're not demangling the symbol reference though still...

[nickdesaulniers]

The call to std::strlen(MangledName) in llvm::itaniumDemangle looks suspicious and I'll bet we're having trouble demangling non-null terminated string_views.

[nickdesaulniers]

In D149675#4313532, @nickdesaulniers wrote:

The call to std::strlen(MangledName) in llvm::itaniumDemangle looks suspicious and I'll bet we're having trouble demangling non-null terminated string_views.

Yeah, something funny going on where the MangledName.length() != std::strlen(MangledName.data()) for this test case. Digging...

[nickdesaulniers]

Testing a more complete fix...

[nickdesaulniers]

sorry, the point I sync'ed to doesn't test cleanly with expensive checks enabled, so it's hard to know if my change is regressing anything new...(and this takes FOREVER to build and run tests with expensive checks enabled)...

[MaskRay]

In D149675#4313833, @nickdesaulniers wrote:

sorry, the point I sync'ed to doesn't test cleanly with expensive checks enabled, so it's hard to know if my change is regressing anything new...(and this takes FOREVER to build and run tests with expensive checks enabled)...

You may just run ninja check-lld-wasm. The change does not affect other testsuites..

[nickdesaulniers]

• redo patch completely

[nickdesaulniers]

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp

91–94

This needs a bounds check; otherwise a few lllvm-cxxfilt tests fail an assertion.

[nickdesaulniers]

• use starts_with for bounds check, rewrite part of commit message

[efriedma]

Maybe it makes sense to revert D149104 while you're fixing the various demanglers to correctly respect the input length?

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp

91–94

DecoratedStr.substr(6) == "__imp_" looks wrong. Can we not just use DecoratedStr.starts_with("__imp_")?

[nickdesaulniers]

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp

94

Oh, look, another interface where nullptr is passed excessively by EVERY call site. Smells like another cleanup similar to D148940. Will do so in a follow up patch.

[MaskRay]

Is c_str2 NUL-terminated? Good question. It depends on whether c_str was NUL-terminated. When s in the above example is a std::string, then it doesn't matter whether c_str is NUL-terminated or not, c_str2 ALWAYS will be.

I am still confused... Is the bug in lld/wasm or llvm/lib/Demangle?

If the former it seems that we need a patch fixing just that part. Changing the signature of llvm::itaniumDemangle is useful but can be a separate patch.

If the latter, which const char * variable points a non-NUL-terminated string that leads to the lld/test/wasm failure?

[nickdesaulniers]

In D149675#4313933, @efriedma wrote:

Maybe it makes sense to revert D149104 while you're fixing the various demanglers to correctly respect the input length?

This should fix-forwards the test. Sorry it took me so long to root cause.

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp

91–94

mid-air collision on your comment, PTAL. also phab is being slow for me today.

[nickdesaulniers]

In D149675#4313939, @MaskRay wrote:

Is c_str2 NUL-terminated? Good question. It depends on whether c_str was NUL-terminated. When s in the above example is a std::string, then it doesn't matter whether c_str is NUL-terminated or not, c_str2 ALWAYS will be.

I am still confused... Is the bug in lld/wasm or llvm/lib/Demangle?

llvm/lib/Demangle

If the former it seems that we need a patch fixing just that part. Changing the signature of llvm::itaniumDemangle is useful but can be a separate patch.

False. Passing a c_str that may not be null terminated without passing the length then calling std::strlen on it gives bonkers results. The interface of llvm::itaniumDemangle must be changed to pass the size, such as by passing a std::string_view.

If the latter, which const char * variable points a non-NUL-terminated string that leads to the lld/test/wasm failure?

The std::string_view's data() in llvm::demangle is passed to llvm::itaniumDemangle which calls std::strlen.

[efriedma]

LGTM

In D149675#4313944, @nickdesaulniers wrote:

In D149675#4313933, @efriedma wrote:

Maybe it makes sense to revert D149104 while you're fixing the various demanglers to correctly respect the input length?

This should fix-forwards the test. Sorry it took me so long to root cause.

If I understand the situation correctly, all the other demanglers read past the end of the buffer too; we just get lucky that none of the tests on the buildbots use the API in a way which exposes that. I'd prefer to revert to known working, then fix each demangler, then fix the generic demangle() API.

[nickdesaulniers]

In D149675#4313962, @efriedma wrote:

LGTM

In D149675#4313944, @nickdesaulniers wrote:

In D149675#4313933, @efriedma wrote:

Maybe it makes sense to revert D149104 while you're fixing the various demanglers to correctly respect the input length?

This should fix-forwards the test. Sorry it took me so long to root cause.

If I understand the situation correctly, all the other demanglers read past the end of the buffer too; we just get lucky that none of the tests on the buildbots use the API in a way which exposes that. I'd prefer to revert to known working, then fix each demangler, then fix the generic demangle() API.

Sure, I've backed out the offending commit in 3e3c6f24ff85ea52ed67d4c26f1d3d0eacd1ad1b.

[MaskRay]

In D149675#4313962, @efriedma wrote:

LGTM

In D149675#4313944, @nickdesaulniers wrote:

In D149675#4313933, @efriedma wrote:

Maybe it makes sense to revert D149104 while you're fixing the various demanglers to correctly respect the input length?

This should fix-forwards the test. Sorry it took me so long to root cause.

If I understand the situation correctly, all the other demanglers read past the end of the buffer too; we just get lucky that none of the tests on the buildbots use the API in a way which exposes that. I'd prefer to revert to known working, then fix each demangler, then fix the generic demangle() API.

Sounds good. So we need two commits

• Revert D149104. The description of this patch can be used by the revert commit.
• Combine D149104 and this patch

Both commits need very sophisticated testing (including check-lldb check-bolt)...

[nickdesaulniers]

• rebase on top of D149703, D149707, and the revert of D149104
• ninja check-llvm check-lldb check-bolt

[nickdesaulniers]

In D149675#4313980, @MaskRay wrote:

In D149675#4313962, @efriedma wrote:

LGTM

In D149675#4313944, @nickdesaulniers wrote:

In D149675#4313933, @efriedma wrote:

Maybe it makes sense to revert D149104 while you're fixing the various demanglers to correctly respect the input length?

This should fix-forwards the test. Sorry it took me so long to root cause.

If I understand the situation correctly, all the other demanglers read past the end of the buffer too; we just get lucky that none of the tests on the buildbots use the API in a way which exposes that. I'd prefer to revert to known working, then fix each demangler, then fix the generic demangle() API.

Sounds good. So we need two commits

• Revert D149104. The description of this patch can be used by the revert commit.

Done https://reviews.llvm.org/rG3e3c6f24ff85ea52ed67d4c26f1d3d0eacd1ad1b.

• Combine D149104 and this patch

I don't plan to combine them and would prefer to keep them distinct patches to minimize the amount of code that may get reverted (if at all). D149104 was a "top down" approach to the interface. The necessity of the revert shown it should be done bottom up instead.

This patch, as well as D149784 and D151003 are bottom up. I also need to write a patch for the M$ demangler. Then I will revisit the capstone D149104.

Both commits need very sophisticated testing (including check-lldb check-bolt)...

Done (for this patch).

[nickdesaulniers]

llvm/lib/Demangle/Demangle.cpp

31

I guess S is now unnecessary and can be removed.

[nickdesaulniers]

llvm/lib/Demangle/Demangle.cpp

31

Nvm/not yet. microsoftDemangle needs to be converted to use std::string_view first.

[nickdesaulniers]

• rebase for presubmits

[MaskRay]

llvm/lib/Demangle/Demangle.cpp

50

I think it's best for the if (!MangledName) return false; change to be made in this patch, not in the precursor patch.

isItaniumEncoding/etc still take const char * arguments and it makes sense for them to keep strict by disallowing nullptr arguments.

[nickdesaulniers]

llvm/lib/Demangle/Demangle.cpp

50

I suspect you are referring to https://reviews.llvm.org/D151003#4391457.

In that case, I will drop the nullptr check from D151003. I will land this (D149675) as is.

[nickdesaulniers]

llvm/lib/Demangle/Demangle.cpp

50

isItaniumEncoding/etc still take const char * arguments and it makes sense for them to keep strict by disallowing nullptr arguments.

I thought I had fixed that already. Ah! Yeah, but it was backed out as part of:

https://reviews.llvm.org/D149104#change-M6oofoKhLOim

I'll break that off into a separate patch since it's a good change to make separate from landing the top down refactoring.

llvm/include/llvm/Demangle/Demangle.h

enum : int {
   demangle_invalid_mangled_name = -2,
   demangle_memory_alloc_failure = -1,
   demangle_success = 0,
 };
 
 /// Returns a non-NULL pointer to a NUL-terminated C style string
 /// that should be explicitly freed, if successful. Otherwise, may return
 /// nullptr if mangled_name is not a valid mangling or is nullptr.
-char *itaniumDemangle(const char *mangled_name);
+char *itaniumDemangle(std::string_view mangled_name);
 
 enum MSDemangleFlags {
   MSDF_None = 0,
   MSDF_DumpBackrefs = 1 << 0,
   MSDF_NoAccessSpecifier = 1 << 1,
   MSDF_NoCallingConvention = 1 << 2,
   MSDF_NoReturnType = 1 << 3,
   MSDF_NoMemberType = 1 << 4,

 
 /// 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);
 
-bool nonMicrosoftDemangle(const char *MangledName, std::string &Result);
+bool nonMicrosoftDemangle(std::string_view MangledName, std::string &Result);
 
 /// "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);

llvm/lib/DebugInfo/Symbolize/Symbolize.cpp

 }
 
 } // end anonymous namespace
 
 std::string
 LLVMSymbolizer::DemangleName(const std::string &Name,
                              const SymbolizableModule *DbiModuleDescriptor) {
   std::string Result;
-  if (nonMicrosoftDemangle(Name.c_str(), Result))
+  if (nonMicrosoftDemangle(Name, Result))
     return Result;
 
   if (!Name.empty() && Name.front() == '?') {
     // Only do MSVC C++ demangling on symbols starting with '?'.
     int status = 0;
     char *DemangledName = microsoftDemangle(
         Name.c_str(), nullptr, &status,
         MSDemangleFlags(MSDF_NoAccessSpecifier | MSDF_NoCallingConvention |
                         MSDF_NoMemberType | MSDF_NoReturnType));
     if (status != 0)
       return Name;
     Result = DemangledName;
     free(DemangledName);
     return Result;
   }
 
   if (DbiModuleDescriptor && DbiModuleDescriptor->isWin32Module()) {
     std::string DemangledCName(demanglePE32ExternCFunc(Name));
     // On i386 Windows, the C name mangling for different calling conventions
     // may also be applied on top of the Itanium or Rust name mangling.
-    if (nonMicrosoftDemangle(DemangledCName.c_str(), Result))
+    if (nonMicrosoftDemangle(DemangledCName, Result))
       return Result;
     return DemangledCName;
   }
   return Name;
 }
 
 void LLVMSymbolizer::recordAccess(CachedBinary &Bin) {
   if (Bin->getBinary())

llvm/lib/Demangle/Demangle.cpp

 
 static bool isDLangEncoding(const std::string &MangledName) {
   return MangledName.size() >= 2 && MangledName[0] == '_' &&
          MangledName[1] == 'D';
 }
 
 std::string llvm::demangle(const std::string &MangledName) {
   std::string Result;
   const char *S = MangledName.c_str();
 
-  if (nonMicrosoftDemangle(S, Result))
+  if (nonMicrosoftDemangle(MangledName, Result))
     return Result;
 
-  if (S[0] == '_' && nonMicrosoftDemangle(S + 1, Result))
+  if (S[0] == '_' && nonMicrosoftDemangle(MangledName.substr(1), Result))
     return Result;
 
   if (char *Demangled = microsoftDemangle(S, nullptr, nullptr)) {
     Result = Demangled;
     std::free(Demangled);
     return Result;
   }
 
   return MangledName;
 }
 
-bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) {
+bool llvm::nonMicrosoftDemangle(std::string_view MangledName,
+                                std::string &Result) {
   char *Demangled = nullptr;
-  if (isItaniumEncoding(MangledName))
+  if (isItaniumEncoding(MangledName.data()))
     Demangled = itaniumDemangle(MangledName);
-  else if (isRustEncoding(MangledName))
-    Demangled = rustDemangle(MangledName);
-  else if (isDLangEncoding(MangledName))
-    Demangled = dlangDemangle(MangledName);
+  else if (isRustEncoding(MangledName.data()))
+    Demangled = rustDemangle(MangledName.data());
+  else if (isDLangEncoding(MangledName.data()))
+    Demangled = dlangDemangle(MangledName.data());
 
   if (!Demangled)
     return false;
 
   Result = Demangled;
   std::free(Demangled);
   return true;
 }

llvm/lib/Demangle/ItaniumDemangle.cpp

 }  // unnamed namespace
 
 //===----------------------------------------------------------------------===//
 // Code beyond this point should not be synchronized with libc++abi.
 //===----------------------------------------------------------------------===//
 
 using Demangler = itanium_demangle::ManglingParser<DefaultAllocator>;
 
-char *llvm::itaniumDemangle(const char *MangledName) {
-  if (!MangledName)
+char *llvm::itaniumDemangle(std::string_view MangledName) {
+  if (MangledName.empty())
     return nullptr;
 
-  Demangler Parser(MangledName, MangledName + std::strlen(MangledName));
+  Demangler Parser(MangledName.data(),
+                   MangledName.data() + MangledName.length());
   Node *AST = Parser.parse();
   if (!AST)
     return nullptr;
 
   OutputBuffer OB;
   assert(Parser.ForwardTemplateRefs.empty());
   AST->print(OB);
   OB += '\0';

llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp

 //===-- llvm-c++filt.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/ADT/StringExtras.h"
 #include "llvm/Demangle/Demangle.h"
+#include "llvm/Demangle/StringViewExtras.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/LLVMDriver.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"

 static StringRef ToolName;
 
 static void error(const Twine &Message) {
   WithColor::error(errs(), ToolName) << Message << '\n';
   exit(1);
 }
 
 static std::string demangle(const std::string &Mangled) {
-  const char *DecoratedStr = Mangled.c_str();
+  using llvm::itanium_demangle::starts_with;
+  std::string_view DecoratedStr = Mangled;
   if (StripUnderscore)
     if (DecoratedStr[0] == '_')
-      ++DecoratedStr;
+      DecoratedStr.remove_prefix(1);
 
   std::string Result;
   if (nonMicrosoftDemangle(DecoratedStr, Result))
     return Result;
 
   std::string Prefix;
   char *Undecorated = nullptr;
 
   if (Types)
     Undecorated = itaniumDemangle(DecoratedStr);
 
-  if (!Undecorated && strncmp(DecoratedStr, "__imp_", 6) == 0) {
+  if (!Undecorated && starts_with(DecoratedStr, "__imp_")) {
     Prefix = "import thunk for ";
-    Undecorated = itaniumDemangle(DecoratedStr + 6);
+    Undecorated = itaniumDemangle(DecoratedStr.substr(6));
   }
 
   Result = Undecorated ? Prefix + Undecorated : Mangled;
   free(Undecorated);
   return Result;
 }
 
 // Split 'Source' on any character that fails to pass 'IsLegalChar'.  The
 // returned vector consists of pairs where 'first' is the delimited word, and

llvm/tools/llvm-nm/llvm-nm.cpp

static void darwinPrintStab(MachOObjectFile *MachO, const NMSymbol &S) {
   if (const char *stabString = getDarwinStabString(NType))
     outs() << format("%5.5s", stabString);
   else
     outs() << format("   %02x", NType);
 }
 
 static std::optional<std::string> demangle(StringRef Name) {
   std::string Demangled;
-  if (nonMicrosoftDemangle(Name.str().c_str(), Demangled))
+  if (nonMicrosoftDemangle(Name, Demangled))
     return Demangled;
   return std::nullopt;
 }
 
 static std::optional<std::string> demangleXCOFF(StringRef Name) {
   if (Name.empty() || Name[0] != '.')
     return demangle(Name);
 

llvm/tools/llvm-opt-report/OptReport.cpp

for (line_iterator LI(*Buf.get(), false); LI != line_iterator(); ++LI) {
           for (const auto &FuncName : FuncNameSet) {
             if (FirstFunc)
               FirstFunc = false;
             else
               OS << ", ";
 
             bool Printed = false;
             if (!NoDemangle) {
-              if (char *Demangled = itaniumDemangle(FuncName.c_str())) {
+              if (char *Demangled = itaniumDemangle(FuncName)) {
                 OS << Demangled;
                 Printed = true;
                 std::free(Demangled);
               }
             }
 
             if (!Printed)
               OS << FuncName;