This is an archive of the discontinued LLVM Phabricator instance.

[Demangle] replace use of llvm::StringView w/ std::string_view
ClosedPublic

Authored by nickdesaulniers on Apr 14 2023, 3:13 PM.

Details

Summary

This refactoring was waiting on converting LLVM to C++17.

Leave StringView.h and cleanup around for subsequent cleanup.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2023, 3:13 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
nickdesaulniers requested review of this revision.Apr 14 2023, 3:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2023, 3:13 PM
MaskRay accepted this revision.Apr 14 2023, 3:24 PM

LLVMDemangle is used by bolt, clang, lld, lldb. You may want to build all of them to ensure all in-tree users have been migrated.

This revision is now accepted and ready to land.Apr 14 2023, 3:24 PM
nickdesaulniers planned changes to this revision.Apr 14 2023, 3:37 PM

LLVMDemangle is used by bolt, clang, lld, lldb. You may want to build all of them to ensure all in-tree users have been migrated.

Oops! Wasn't buildnig bolt or lldb; thanks for pointing that out!

This revision is now accepted and ready to land.Apr 14 2023, 3:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2023, 3:41 PM
This revision was landed with ongoing or failed builds.Apr 14 2023, 3:50 PM
This revision was automatically updated to reflect the committed changes.
nickdesaulniers added a comment.EditedApr 14 2023, 3:57 PM

These windows buildbot failures are tough to make out: https://lab.llvm.org/buildbot/#/builders/127/builds/46749/steps/4/logs/stdio

[2/1778] Building CXX object lib\Demangle\CMakeFiles\LLVMDemangle.dir\DLangDemangle.cpp.obj
FAILED: lib/Demangle/CMakeFiles/LLVMDemangle.dir/DLangDemangle.cpp.obj 
C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_LIBCPP_ENABLE_ASSERTIONS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\Demangle -IC:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle -Iinclude -IC:\b\slave\sanitizer-windows\llvm-project\llvm\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Zi /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++17 /showIncludes /Folib\Demangle\CMakeFiles\LLVMDemangle.dir\DLangDemangle.cpp.obj /Fdlib\Demangle\CMakeFiles\LLVMDemangle.dir\LLVMDemangle.pdb /FS -c C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\DLangDemangle.cpp
C:\b\slave\sanitizer-windows\llvm-project\llvm\include\llvm/Demangle/Utility.h(111): error C2664: 'void *memcpy(void *,const void *,size_t)': cannot convert argument 2 from 'std::_String_view_iterator<_Traits>' to 'const void *'
        with
        [
            _Traits=std::char_traits<char>
        ]
C:\b\slave\sanitizer-windows\llvm-project\llvm\include\llvm/Demangle/Utility.h(111): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\vcruntime_string.h(43): note: see declaration of 'memcpy'

So something crazy is going on with std::string_view::iterator and implicit conversion to void*? Is this an issue in Windows C++17 support?

EDIT: also:

C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\MicrosoftDemangle.cpp(2403): error C2678: binary '-': no operator found which takes a left-hand operand of type 'std::_String_view_iterator<_Traits>' (or there is no acceptable conversion)

C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\MicrosoftDemangle.cpp(2380): warning C4477: 'printf' : format string '%.*s' requires an argument of type 'char *', but variadic argument 3 has type 'std::_String_view_iterator<_Traits>'

C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\MicrosoftDemangle.cpp(2388): warning C4477: 'printf' : format string '%.*s' requires an argument of type 'char *', but variadic argument 3 has type 'std::_String_view_iterator<_Traits>'

C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\ItaniumDemangle.cpp(82): warning C4477: 'fprintf' : format string '%.*s' requires an argument of type 'char *', but variadic argument 2 has type 'std::_String_view_iterator<_Traits>'

These windows buildbot failures are tough to make out: https://lab.llvm.org/buildbot/#/builders/127/builds/46749/steps/4/logs/stdio

[2/1778] Building CXX object lib\Demangle\CMakeFiles\LLVMDemangle.dir\DLangDemangle.cpp.obj
FAILED: lib/Demangle/CMakeFiles/LLVMDemangle.dir/DLangDemangle.cpp.obj 
C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_LIBCPP_ENABLE_ASSERTIONS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\Demangle -IC:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle -Iinclude -IC:\b\slave\sanitizer-windows\llvm-project\llvm\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Zi /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++17 /showIncludes /Folib\Demangle\CMakeFiles\LLVMDemangle.dir\DLangDemangle.cpp.obj /Fdlib\Demangle\CMakeFiles\LLVMDemangle.dir\LLVMDemangle.pdb /FS -c C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\DLangDemangle.cpp
C:\b\slave\sanitizer-windows\llvm-project\llvm\include\llvm/Demangle/Utility.h(111): error C2664: 'void *memcpy(void *,const void *,size_t)': cannot convert argument 2 from 'std::_String_view_iterator<_Traits>' to 'const void *'
        with
        [
            _Traits=std::char_traits<char>
        ]
C:\b\slave\sanitizer-windows\llvm-project\llvm\include\llvm/Demangle/Utility.h(111): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\vcruntime_string.h(43): note: see declaration of 'memcpy'

So something crazy is going on with std::string_view::iterator and implicit conversion to void*? Is this an issue in Windows C++17 support?

EDIT: also:

C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\MicrosoftDemangle.cpp(2403): error C2678: binary '-': no operator found which takes a left-hand operand of type 'std::_String_view_iterator<_Traits>' (or there is no acceptable conversion)

C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\MicrosoftDemangle.cpp(2380): warning C4477: 'printf' : format string '%.*s' requires an argument of type 'char *', but variadic argument 3 has type 'std::_String_view_iterator<_Traits>'

C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\MicrosoftDemangle.cpp(2388): warning C4477: 'printf' : format string '%.*s' requires an argument of type 'char *', but variadic argument 3 has type 'std::_String_view_iterator<_Traits>'

C:\b\slave\sanitizer-windows\llvm-project\llvm\lib\Demangle\ItaniumDemangle.cpp(82): warning C4477: 'fprintf' : format string '%.*s' requires an argument of type 'char *', but variadic argument 2 has type 'std::_String_view_iterator<_Traits>'

Perhaps:

diff --git a/llvm/lib/Demangle/ItaniumDemangle.cpp b/llvm/lib/Demangle/ItaniumDemangle.cpp
index 4bc00c5144a4..7a7bcd36fc68 100644
--- a/llvm/lib/Demangle/ItaniumDemangle.cpp
+++ b/llvm/lib/Demangle/ItaniumDemangle.cpp
@@ -79,7 +79,7 @@ struct DumpVisitor {
 
   void printStr(const char *S) { fprintf(stderr, "%s", S); }
   void print(std::string_view SV) {
-    fprintf(stderr, "\"%.*s\"", (int)SV.size(), SV.begin());
+    fprintf(stderr, "\"%.*s\"", (int)SV.size(), &*SV.begin());
   }
   void print(const Node *N) {
     if (N)
diff --git a/llvm/lib/Demangle/MicrosoftDemangle.cpp b/llvm/lib/Demangle/MicrosoftDemangle.cpp
index 15cb0e7d8864..5204c24164d6 100644
--- a/llvm/lib/Demangle/MicrosoftDemangle.cpp
+++ b/llvm/lib/Demangle/MicrosoftDemangle.cpp
@@ -2377,7 +2377,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.begin());
   }
   std::free(OB.getBuffer());
 
@@ -2386,7 +2386,7 @@ void Demangler::dumpBackReferences() {
   std::printf("%d name backreferences\n", (int)Backrefs.NamesCount);
   for (size_t I = 0; I < Backrefs.NamesCount; ++I) {
     std::printf("  [%d] - %.*s\n", (int)I, (int)Backrefs.Names[I]->Name.size(),
-                Backrefs.Names[I]->Name.begin());
+                &*Backrefs.Names[I]->Name.begin());
   }
   if (Backrefs.NamesCount > 0)
     std::printf("\n");
@@ -2400,7 +2400,7 @@ char *llvm::microsoftDemangle(const char *MangledName, size_t *NMangled,
   std::string_view Name{MangledName};
   SymbolNode *AST = D.parse(Name);
   if (!D.Error && NMangled)
-    *NMangled = Name.begin() - MangledName;
+    *NMangled = Name.begin() - &*MangledName;
 
   if (Flags & MSDF_DumpBackrefs)
     D.dumpBackReferences();

?

chapuni added inline comments.
llvm/include/llvm/Demangle/ItaniumDemangle.h
19

Odd layering...

MaskRay added inline comments.Apr 14 2023, 5:12 PM
llvm/include/llvm/Demangle/ItaniumDemangle.h
19

Good catch. LLVMDemangle cannot depend on other LLVM libraries.

It looks like this breaks the build: https://lab.llvm.org/buildbot#builders/119/builds/12865 https://lab.llvm.org/buildbot#builders/123/builds/18388 https://lab.llvm.org/buildbot#builders/60/builds/11628

I'm pretty new with LLVM so I'll leave reverting the change to someone else, but I figured I should give you a heads up.

It looks like this breaks the build: https://lab.llvm.org/buildbot#builders/119/builds/12865 https://lab.llvm.org/buildbot#builders/123/builds/18388 https://lab.llvm.org/buildbot#builders/60/builds/11628

I'm pretty new with LLVM so I'll leave reverting the change to someone else, but I figured I should give you a heads up.

I am unsure about the Windows issue and find it difficult to repair... I'm testing a revert commit to not leave issues to the weekend.

Ah, I see what happened. Those 3 builds had my follow up commit https://reviews.llvm.org/D148392, BUT the initial broken builds stopped producing diagnostics after the initial set of diagnostics that I fixed; kind of like how clang stops emitting diagnostics after the first 20. You can fix the 20, but there might be more to fix. That's what happened here. Specifically, I see 3 new diagnostics that weren't addressed in https://reviews.llvm.org/D148392:

  1. llvm\lib\Demangle\MicrosoftDemangle.cpp(809)
  2. llvm\include\llvm/Demangle/ItaniumDemangle.h(2334)
  3. llvm\include\llvm/Demangle/ItaniumDemangle.h(2335)

I'll reopen this with https://reviews.llvm.org/D148392 squashed, plus the above three issues fixed, then get a colleague with a windows machine to help verify.

I am unsure about the Windows issue and find it difficult to repair... I'm testing a revert commit to not leave issues to the weekend.

That was the right thing to do, thank you.

nickdesaulniers reopened this revision.Apr 17 2023, 10:16 AM
This revision is now accepted and ready to land.Apr 17 2023, 10:16 AM
nickdesaulniers planned changes to this revision.Apr 17 2023, 10:16 AM
llvm/include/llvm/Demangle/ItaniumDemangle.h
19

If these are inline function definitions, then there is no linkage between the libraries?

  • reopen with D148392 folded in, and 3 additional fixes for windows as reported by buildbots
This revision is now accepted and ready to land.Apr 17 2023, 10:41 AM

Creating https://reviews.llvm.org/D148546 instead; arcanist really doesn't like how I've reopened this review.

MaskRay reopened this revision.Apr 17 2023, 10:54 AM
This revision is now accepted and ready to land.Apr 17 2023, 10:54 AM

Let's use D148546 as the reland (sorry, couldn't get arcanist to work to reuse this instance of code review reopened).