[LLDB] Use the llvm microsoft demangler instead of the windows dbghelp api

[labath]

yay

[thakis]

We can add flags for omitting access specifiers etc if it's critical for lldb. Or maybe we can just change the test that caused the revert.

[mstorsjo]

In D68134#1686970, @thakis wrote:

We can add flags for omitting access specifiers etc if it's critical for lldb. Or maybe we can just change the test that caused the revert.

Yeah I doubt it's critical to maintain the exact same form as before, but I need to get the tests running in my cross compile setup to verify exactly how to update them.

[labath]

In D68134#1687031, @mstorsjo wrote:

In D68134#1686970, @thakis wrote:

We can add flags for omitting access specifiers etc if it's critical for lldb. Or maybe we can just change the test that caused the revert.

Yeah I doubt it's critical to maintain the exact same form as before, but I need to get the tests running in my cross compile setup to verify exactly how to update them.

I'm not sure what failed here exactly, but there are some places in lldb that parse the demangled names. These might get confused by additional things appearing in the name. Though it's possible to also fix that, so the main question might be: what is the name we want to display to the users? I guess it would be the best if this matched what is displayed by other tools ?

As for tests, you should at least be able to run the tests in the regular "host" setup, right ?

[labath]

(See CPlusPlusLanguage::MethodName for one such parsing instance.)

[mstorsjo]

I tried to update the testcases based on the log output (http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9306/steps/test/logs/stdio).

Even after fudging the tests to somehow run on windows, both of the tests that failed seem to require the DIA SDK which I don't have readily available (for reading PDB files; there's some native support for PDB files but I'm not sure what the state of it is and running those tests with it).

[mstorsjo]

In D68134#1687500, @labath wrote:

In D68134#1687031, @mstorsjo wrote:

In D68134#1686970, @thakis wrote:

We can add flags for omitting access specifiers etc if it's critical for lldb. Or maybe we can just change the test that caused the revert.

Yeah I doubt it's critical to maintain the exact same form as before, but I need to get the tests running in my cross compile setup to verify exactly how to update them.

I'm not sure what failed here exactly, but there are some places in lldb that parse the demangled names. These might get confused by additional things appearing in the name. Though it's possible to also fix that, so the main question might be: what is the name we want to display to the users? I guess it would be the best if this matched what is displayed by other tools ?

In this case, the output becomes the same as what has been chosen to be used by the common llvm demangler, which probably should be a good choice in general. In this case, a few more identifiers are added. Or I guess it can be discussed if that one should be changed (or add options to it, as @thakis said was possible).

As for tests, you should at least be able to run the tests in the regular "host" setup, right ?

In principle, but I normally don't run windows except in a very underpowered VM for testing things, and building there is no fun. I can spin up some more powerful VMs for some better testing though.

In this case, much of the functionality of these tests require having the MS DIA SDK available (which also implies building in MSVC/clang-cl mode, not mingw mode), for reading PDB files. Probably just another step to do, but I don't have it set up right now.

[labath]

In D68134#1687518, @mstorsjo wrote:

In D68134#1687500, @labath wrote:

I'm not sure what failed here exactly, but there are some places in lldb that parse the demangled names. These might get confused by additional things appearing in the name. Though it's possible to also fix that, so the main question might be: what is the name we want to display to the users? I guess it would be the best if this matched what is displayed by other tools ?

In this case, the output becomes the same as what has been chosen to be used by the common llvm demangler, which probably should be a good choice in general.

That makes sense to me, but I am not really a windows person. Maybe wait a while to see if any of the real "windows people" have any thoughts.

As for tests, you should at least be able to run the tests in the regular "host" setup, right ?

In principle, but I normally don't run windows except in a very underpowered VM for testing things, and building there is no fun. I can spin up some more powerful VMs for some better testing though.

In this case, much of the functionality of these tests require having the MS DIA SDK available (which also implies building in MSVC/clang-cl mode, not mingw mode), for reading PDB files. Probably just another step to do, but I don't have it set up right now.

So, you're developing windows arm64 support, without even a real windows x86 around? That's brave. :)

[mstorsjo]

In D68134#1687865, @labath wrote:

In D68134#1687518, @mstorsjo wrote:

In D68134#1687500, @labath wrote:

I'm not sure what failed here exactly, but there are some places in lldb that parse the demangled names. These might get confused by additional things appearing in the name. Though it's possible to also fix that, so the main question might be: what is the name we want to display to the users? I guess it would be the best if this matched what is displayed by other tools ?

In this case, the output becomes the same as what has been chosen to be used by the common llvm demangler, which probably should be a good choice in general.

That makes sense to me, but I am not really a windows person. Maybe wait a while to see if any of the real "windows people" have any thoughts.

@thakis - Do you happen to have an opinion here on what's the best course forward? I guess you're the one currently most familiar with the MS demangler.

As for tests, you should at least be able to run the tests in the regular "host" setup, right ?

In principle, but I normally don't run windows except in a very underpowered VM for testing things, and building there is no fun. I can spin up some more powerful VMs for some better testing though.

In this case, much of the functionality of these tests require having the MS DIA SDK available (which also implies building in MSVC/clang-cl mode, not mingw mode), for reading PDB files. Probably just another step to do, but I don't have it set up right now.

So, you're developing windows arm64 support, without even a real windows x86 around? That's brave. :)

That's nothing; I brought up most of the windows arm64 support in llvm/clang/lld before I even had a real windows arm64 device to test on (and without access to MSVC targeting arm64 as well for most of the time), just testing in wine, which was capable of running a handful of binaries that were available :-)

[amccarth]

LGTM after one question.

lldb/lit/SymbolFile/PDB/udt-layout.test

1 ↗

(On Diff #222358)

Is system-windows still required after you've removed the dependency on dbghelp?

[mstorsjo]

In D68134#1700453, @amccarth wrote:

LGTM after one question.

Thanks! I'll hold off committing for a bit still, as I might try to add more options to the microsoft demangler, to match the previous output.

lldb/lit/SymbolFile/PDB/udt-layout.test

1 ↗

(On Diff #222358)

Yes, this test depends on the system-provided PDB support, so it needs to run on windows.

[mstorsjo]

Using newly added demangler options to (hopefully) avoid any changes to test outputs (at least the differences seen when applied earlier shouldn't be present any longer).

[mstorsjo]

Does anyone want to ack this one after updating it to not need changes to tests (as far as I know), by using new demangler options?

lldb/source/Core/Mangled.cpp

 //===-- Mangled.cpp ---------------------------------------------*- C++ -*-===//
 //
 // 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 "lldb/Core/Mangled.h"
 
-#if defined(_WIN32)
-#include "lldb/Host/windows/windows.h"
-
-#include <dbghelp.h>
-#pragma comment(lib, "dbghelp.lib")
-#endif
-
 #include "lldb/Core/RichManglingContext.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Logging.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
 #include "lldb/lldb-enumerations.h"
 
 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
 #include "Plugins/Language/ObjC/ObjCLanguage.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Demangle/Demangle.h"
 #include "llvm/Support/Compiler.h"
 
 #include <mutex>
 #include <string>
 #include <utility>
 
 #include <stdlib.h>
 #include <string.h>
 using namespace lldb_private;
 
-#if defined(_MSC_VER)
-static DWORD safeUndecorateName(const char *Mangled, char *Demangled,
-                                DWORD DemangledLength) {
-  static std::mutex M;
-  std::lock_guard<std::mutex> Lock(M);
-  return ::UnDecorateSymbolName(
-      Mangled, Demangled, DemangledLength,
-      UNDNAME_NO_ACCESS_SPECIFIERS |       // Strip public, private, protected
-                                           // keywords
-          UNDNAME_NO_ALLOCATION_LANGUAGE | // Strip __thiscall, __stdcall,
-                                           // etc keywords
-          UNDNAME_NO_THROW_SIGNATURES |    // Strip throw() specifications
-          UNDNAME_NO_MEMBER_TYPE |         // Strip virtual, static, etc
-                                           // specifiers
-          UNDNAME_NO_MS_KEYWORDS           // Strip all MS extension keywords
-      );
-}
-#endif
-
 static inline Mangled::ManglingScheme cstring_mangling_scheme(const char *s) {
   if (s) {
     if (s[0] == '?')
       return Mangled::eManglingSchemeMSVC;
     if (s[0] == '_' && s[1] == 'Z')
       return Mangled::eManglingSchemeItanium;
   }
   return Mangled::eManglingSchemeNone;
void Mangled::SetValue(ConstString name) {
   } else {
     m_demangled.Clear();
     m_mangled.Clear();
   }
 }
 
 // Local helpers for different demangling implementations.
 static char *GetMSVCDemangledStr(const char *M) {
-#if defined(_MSC_VER)
-  const size_t demangled_length = 2048;
-  char *demangled_cstr = static_cast<char *>(::malloc(demangled_length));
-  ::ZeroMemory(demangled_cstr, demangled_length);
-  DWORD result = safeUndecorateName(M, demangled_cstr, demangled_length);
+  char *demangled_cstr = llvm::microsoftDemangle(
+      M, nullptr, nullptr, nullptr,
+      llvm::MSDemangleFlags(llvm::MSDF_NoAccessSpecifier |
+                            llvm::MSDF_NoCallingConvention |
+                            llvm::MSDF_NoMemberType));
 
   if (Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_DEMANGLE)) {
     if (demangled_cstr && demangled_cstr[0])
       LLDB_LOGF(log, "demangled msvc: %s -> \"%s\"", M, demangled_cstr);
     else
-      LLDB_LOGF(log, "demangled msvc: %s -> error: 0x%lu", M, result);
+      LLDB_LOGF(log, "demangled msvc: %s -> error", M);
   }
 
-  if (result != 0) {
-    return demangled_cstr;
-  } else {
-    ::free(demangled_cstr);
-    return nullptr;
-  }
-#else
-  return nullptr;
-#endif
 }
 
 static char *GetItaniumDemangledStr(const char *M) {
   char *demangled_cstr = nullptr;
 
   llvm::ItaniumPartialDemangler ipd;
   bool err = ipd.partialDemangle(M);
   if (!err) {