[Damangle] convert dlangDemangle to use std::string_view

[nickdesaulniers]

Summary

I was doing this API conversion to use std::string_view top-down in
D149104, but this exposed issues in individual demanglers that needed to
get fixed first. There's no issue with the conversion for the D language
demangler, so convert it.

I have a more aggressive refactoring of the entire D language demangler
to use std::string_view more extensively, but the interface with
llvm::nonMicrosoftDemangle is the more interesting one.

[nickdesaulniers]

needs rebasing for presubmit tests

[nickdesaulniers]

• rebase

[MaskRay]

llvm/lib/Demangle/Demangle.cpp

49

Why is this change? The original contract is that MangledName must be non-null.

[nickdesaulniers]

llvm/lib/Demangle/Demangle.cpp

49

https://fosstodon.org/@llvm/110397650834821908

It's insidious; implicitly constructing a std::string_view from a char* which is nullptr invokes a call to strlen upon construction, which segfaults on my system. Therefor, all of the nullptr checks need to be hoisted into the callers or stated another way exist at the boundary of char* to std::string_view API boundaries (such as right here).

[nickdesaulniers]

llvm/lib/Demangle/Demangle.cpp

49

This is also why the nullptr input test case must be removed from llvm/unittests/Demangle/DLangDemangleTest.cpp below.

[nickdesaulniers]

• rebase for presubmits

[MaskRay]

llvm/lib/Demangle/Demangle.cpp

49

It's usually code smell if a function taking a C string argument additionally accepts nullptr.
Previously, passing nullptr to nonMicrosoftDemangle will crash as MangledName[0] is accessed. We should retain this strictness.

ninja check-llvm-demangle passes if I remove the 2 lines.

If future refactoring will possibly pass nullptr to nonMicrosoftDemangle, I think we should fix those call sites not to do that...

[MaskRay]

If nonMicrosoftDemangle is going to be changed to take a string_view argument, adding if (!MangledName) return false; should be fine.
If possible, it's best for the change to be deferred to a later patch on the stack, but I won't push strongly on this. You did the work...

[nickdesaulniers]

llvm/lib/Demangle/Demangle.cpp

49

We should retain this strictness.
If future refactoring will possibly pass nullptr to nonMicrosoftDemangle, I think we should fix those call sites not to do that...

Ok, I will drop it then, and add nullptr checks to callers.

ninja check-llvm-demangle passes if I remove the 2 lines.

Then check-llvm-demangle must not be running llvm/unittests/Demangle/DLangDemangleTest.cpp, or you tested with my change to llvm/unittests/Demangle/DLangDemangleTest.cpp from below applied.

[nickdesaulniers]

• rebase on D149675, remove nullptr check

No, I don't want no nullptr-check
A nullptr-check is a guard that can't get no love from me
Hangin' out the passenger side
Of their best friend's ride
Trying to holla at me

[efriedma]

llvm/lib/Demangle/DLangDemangle.cpp

555–556

Isn't this still assuming MangledName is null-terminated?

[nickdesaulniers]

llvm/lib/Demangle/DLangDemangle.cpp

555–556

(I have a rewrite of the D language demangler to use std::string_view throughout, which I will post shortly; I believe that addresses your concern)

[nickdesaulniers]

llvm/lib/Demangle/DLangDemangle.cpp

555–556

D152177 PTAL

[nickdesaulniers]

[Damangle]

Damangle? dAmangle? *sigh*

lldb/source/Core/Mangled.cpp

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);
   }
 
   return demangled_cstr;
 }
 
-static char *GetDLangDemangledStr(const char *M) {
+static char *GetDLangDemangledStr(std::string_view M) {
   char *demangled_cstr = llvm::dlangDemangle(M);
 
   if (Log *log = GetLog(LLDBLog::Demangle)) {
     if (demangled_cstr && demangled_cstr[0])
       LLDB_LOG(log, "demangled dlang: {0} -> \"{1}\"", M, demangled_cstr);
     else
       LLDB_LOG(log, "demangled dlang: {0} -> error: failed to demangle", M);
   }
if (mangling_scheme != eManglingSchemeNone &&
       case eManglingSchemeItanium: {
         demangled_name = GetItaniumDemangledStr(mangled_name);
         break;
       }
       case eManglingSchemeRustV0:
         demangled_name = GetRustV0DemangledStr(m_mangled);
         break;
       case eManglingSchemeD:
-        demangled_name = GetDLangDemangledStr(mangled_name);
+        demangled_name = GetDLangDemangledStr(m_mangled);
         break;
       case eManglingSchemeNone:
         llvm_unreachable("eManglingSchemeNone was handled already");
       }
       if (demangled_name) {
         m_demangled.SetStringWithMangledCounterpart(
             llvm::StringRef(demangled_name), m_mangled);
         free(demangled_name);

llvm/include/llvm/Demangle/Demangle.h

 /// Flags controls various details of the demangled representation.
 char *microsoftDemangle(const char *mangled_name, size_t *n_read, int *status,
                         MSDemangleFlags Flags = MSDF_None);
 
 // Demangles a Rust v0 mangled symbol.
 char *rustDemangle(std::string_view MangledName);
 
 // Demangles a D mangled symbol.
-char *dlangDemangle(const char *MangledName);
+char *dlangDemangle(std::string_view 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
 /// demangling occurred.
 std::string demangle(const std::string &MangledName);
 

llvm/lib/Demangle/DLangDemangle.cpp

 //===--- DLangDemangle.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
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file defines a demangler for the D programming language as specified
 /// in the ABI specification, available at:
 /// https://dlang.org/spec/abi.html#name_mangling
 ///
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Demangle/Demangle.h"
+#include "llvm/Demangle/StringViewExtras.h"
 #include "llvm/Demangle/Utility.h"
 
 #include <cctype>
 #include <cstring>
 #include <limits>
 
 using namespace llvm;
 using llvm::itanium_demangle::OutputBuffer;
+using llvm::itanium_demangle::starts_with;
 
 namespace {
 
 /// Demangle information structure.
 struct Demangler {
   /// Initialize the information structure we use to pass around information.
   ///
   /// \param Mangled String to demangle.

 
 Demangler::Demangler(const char *Mangled)
     : Str(Mangled), LastBackref(strlen(Mangled)) {}
 
 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)
+char *llvm::dlangDemangle(std::string_view MangledName) {
+  if (MangledName.empty() || !starts_with(MangledName, "_D"))
     return nullptr;
 
   OutputBuffer Demangled;
-  if (strcmp(MangledName, "_Dmain") == 0) {
+  if (MangledName == "_Dmain") {
     Demangled << "D main";
   } else {
 
-    Demangler D = Demangler(MangledName);
-    MangledName = D.parseMangle(&Demangled);
+    Demangler D(MangledName.data());
+    const char *M = D.parseMangle(&Demangled);
 
     // Check that the entire symbol was successfully demangled.
-    if (MangledName == nullptr || *MangledName != '\0') {
+    if (M == nullptr || *M != '\0') {
       std::free(Demangled.getBuffer());
       return nullptr;
     }
   }
 
   // OutputBuffer's internal buffer is not null terminated and therefore we need
   // to add it to comply with C null terminated strings.
   if (Demangled.getCurrentPosition() > 0) {
     Demangled << '\0';
     Demangled.setCurrentPosition(Demangled.getCurrentPosition() - 1);
     return Demangled.getBuffer();
   }
 
   std::free(Demangled.getBuffer());
   return nullptr;
 }

llvm/lib/Demangle/Demangle.cpp

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

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

 //===--- llvm-dlang-demangle-fuzzer.cpp - Fuzzer for the DLang 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);
-  char *Demangled = llvm::dlangDemangle(NullTerminatedString.c_str());
+  char *Demangled = llvm::dlangDemangle(NullTerminatedString);
   std::free(Demangled);
   return 0;
 }

llvm/unittests/Demangle/DLangDemangleTest.cpp

 //===------------------ DLangDemangleTest.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>
+#include <string_view>
 #include <utility>
 
 struct DLangDemangleTestFixture
-    : public testing::TestWithParam<std::pair<const char *, const char *>> {
+    : public testing::TestWithParam<std::pair<std::string_view, const char *>> {
   char *Demangled;
 
   void SetUp() override { Demangled = llvm::dlangDemangle(GetParam().first); }
 
   void TearDown() override { std::free(Demangled); }
 };
 
 TEST_P(DLangDemangleTestFixture, DLangDemangleTest) {
   EXPECT_STREQ(Demangled, GetParam().second);
 }
 
 INSTANTIATE_TEST_SUITE_P(
     DLangDemangleTest, DLangDemangleTestFixture,
     testing::Values(
-        std::make_pair("_Dmain", "D main"), std::make_pair(nullptr, nullptr),
-        std::make_pair("_Z", nullptr), std::make_pair("_DDD", nullptr),
-        std::make_pair("_D88", nullptr),
+        std::make_pair("_Dmain", "D main"), std::make_pair("_Z", nullptr),
+        std::make_pair("_DDD", nullptr), std::make_pair("_D88", nullptr),
         std::make_pair("_D8demangleZ", "demangle"),
         std::make_pair("_D8demangle4testZ", "demangle.test"),
         std::make_pair("_D8demangle4test5test2Z", "demangle.test.test2"),
         std::make_pair("_D8demangle4test0Z", "demangle.test"),
         std::make_pair("_D8demangle4test03fooZ", "demangle.test.foo"),
         std::make_pair("_D8demangle4test6__initZ",
                        "initializer for demangle.test"),
         std::make_pair("_D8demangle4test6__vtblZ", "vtable for demangle.test"),