Regression(6bad76c7ae93a94f): `llvm-undname --warn-trailing` is broken

[nico]

Expected:

% out/gn/bin/llvm-undname --warn-trailing '.?AVNet@@'
.?AVNet@@
class Net `RTTI Type Descriptor Name'

Actual:

% out/gn/bin/llvm-undname --warn-trailing '.?AVNet@@'
.?AVNet@@
class Net `RTTI Type Descriptor Name'
warning: trailing characters: .?AVNet@@

Bisected to 6bad76c

@nickdesaulniers

[nico]

At least over here, this fixes it:

@@ -2404,7 +2404,7 @@ char *llvm::microsoftDemangle(std::string_view MangledName, size_t *NMangled,
   std::string_view Name{MangledName};
   SymbolNode *AST = D.parse(Name);
   if (!D.Error && NMangled)
-    *NMangled = Name.empty() ? 0 : &*Name.begin() - &*MangledName.begin();
+    *NMangled = Name.data() - MangledName.data();

That's on macOS though, and the commit was about Microsoft's STL.

What are the semantics of derefing begin() for an empty string_view? I could see that tripping debug modes, so calling data() might be better?

There's a few other instances of this questionable pattern; this fixes all the ones I've seen (in MicrosoftDemangle.cpp; maybe ItaniumDemangle.cpp grew a bunch of these too?):

% git diff
diff --git a/llvm/lib/Demangle/MicrosoftDemangle.cpp b/llvm/lib/Demangle/MicrosoftDemangle.cpp
index b38b8a1e479d..aa17d9bd6fef 100644
--- a/llvm/lib/Demangle/MicrosoftDemangle.cpp
+++ b/llvm/lib/Demangle/MicrosoftDemangle.cpp
@@ -268,7 +268,7 @@ std::string_view Demangler::copyString(std::string_view Borrowed) {
   // 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());
+    std::memcpy(Stable, Borrowed.data(), Borrowed.size());
 
   return {Stable, Borrowed.size()};
 }
@@ -792,7 +792,7 @@ SymbolNode *Demangler::demangleMD5Name(std::string_view &MangledName) {
     Error = true;
     return nullptr;
   }
-  const char *Start = &*MangledName.begin();
+  const char *Start = MangledName.data();
   const size_t StartSize = MangledName.size();
   MangledName.remove_prefix(MD5Last + 1);
 
@@ -2382,7 +2382,7 @@ void Demangler::dumpBackReferences() {
     T->output(OB, OF_Default);
 
     std::string_view B = OB;
-    std::printf("  [%d] - %.*s\n", (int)I, (int)B.size(), &*B.begin());
+    std::printf("  [%d] - %.*s\n", (int)I, (int)B.size(), B.data());
   }
   std::free(OB.getBuffer());
 
@@ -2404,7 +2404,7 @@ char *llvm::microsoftDemangle(std::string_view MangledName, size_t *NMangled,
   std::string_view Name{MangledName};
   SymbolNode *AST = D.parse(Name);
   if (!D.Error && NMangled)
-    *NMangled = Name.empty() ? 0 : &*Name.begin() - &*MangledName.begin();
+    *NMangled = Name.data() - MangledName.data();
 
   if (Flags & MSDF_DumpBackrefs)
     D.dumpBackReferences();

@nickdesaulniers Want to see if ItaniumDemangle.cpp needs a similar treatment and take it from here?

[nico]

(The lower-risk fix is to replace the 0 with MangledName.size(). Maybe we should land that first and then do the &*begin() cleanup in a follow-up, since that part's supposed to be behavior-preserving.)

[nickdesaulniers]

@nickdesaulniers Want to see if ItaniumDemangle.cpp needs a similar treatment and take it from here?

Sure, I'm just getting back online today after a week offline, so I probably can't get to this today. Thanks for the report though.

[nickdesaulniers]

https://reviews.llvm.org/D154875 should fix this.

https://reviews.llvm.org/D154876 is the orthogonal cleanup suggested in this thread.