[llvm-cxxfilt] Support Microsoft demangling format

[thieta]

Summary

This refactors the shared demangle() api to handle formats
and then expose that directly in llvm-cxxfilt.

New code should probably use the demangle() function that specifies
what format should be used.

[dblaikie]

New code should probably use the demangle() function that specifies what format should be used.

Not sure I agree with this direction - I'd think that callers that know which format they want to use can/should use APIs that already implement only that format, rather than passing an enum and then switching over the enum to get back to the statically known kind?

So I'd have figured llvm-cxxfilt could have the switch-over-mangling-kind, but the lower level APIs in libDemangle wouldn't have an enum-based API, instead having dedicated functions for different demanglings. (though, arguably, the presence of demangleNonWindows goes against that design/direction)

Depends if most callers are likely to know their mangling or not? Got a sense of that, or what your plans are for this new API, beyond llvm-cxxfilt?

llvm/include/llvm/Demangle/Demangle.h

36

This seems weird to have an entry in this list that is a grouping of other entries in the list?

[thieta]

In D138595#3948378, @dblaikie wrote:

New code should probably use the demangle() function that specifies what format should be used.

Not sure I agree with this direction - I'd think that callers that know which format they want to use can/should use APIs that already implement only that format, rather than passing an enum and then switching over the enum to get back to the statically known kind?
So I'd have figured llvm-cxxfilt could have the switch-over-mangling-kind, but the lower level APIs in libDemangle wouldn't have an enum-based API, instead having dedicated functions for different demanglings. (though, arguably, the presence of demangleNonWindows goes against that design/direction)

I guess I can see your point. I think the problem I was running into when trying to get cxxfilt to demangle Microsoft symbols was that it was most of the API's where behaving differently and I was not able to use the common function since I was not able to limit the demanglers used if you wanted to specify the format. My instinct was to make the shared API easier and more flexible, but maybe this code should just live in cxxfilt tool instead.

Depends if most callers are likely to know their mangling or not? Got a sense of that, or what your plans are for this new API, beyond llvm-cxxfilt?

I looked around at other uses for demangle() and the specific demangle functions:

• llvm::demangle() is used in CodeGenAction, MarkupFilter, ASAN and llvm-cxxfilt, where only cxxfilt seems to care about the demangling format.
• itaniumDemangle() is used all over the source code
• dlang and rust demangle functions are only called by cxxfilt and lldb.
• microsoftDemangle() is used in cxxfilt and undname (which is cxxfilt for windows).
• nonMicrosoftDemangle() is used in cxxfilt, llvm-nm and symbolize

With this information - I think I am in your camp here, better to move the format enum switching to cxxfilt. We can always revisit and rework the common API if more people have a use for it later.

[thieta]

Move enum switching logic to llvm-cxxfilt after review comments.

[thieta]

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

80

I am not super thrilled about copying the isXXXEncoding functions from Demangle.cpp - but they where static in there and not sure we want to expose those API's since they don't seem 100% complete. But let me know what you think.

[jhenderson]

llvm/test/tools/llvm-cxxfilt/microsoft-format.test

2–4

Nit: these should all end in full stops like code comments.

llvm/tools/llvm-cxxfilt/Opts.td

8

Seems like there's a lot of unrelated noise in this file?

22–28

Docs should be updated with the new --format values.

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

53

enum class maybe to avoid polluting the global namespace?

54–59

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly (Itanium, Rust etc).

80

I'm not sure why not being 100% complete translates into "don't expose them, duplicate them". Surely that means when the remaining parts are added, one or other of these locations will likely get missed and we'll end up with a mismatch?

92

93

96–97

It would probably make more sense for MangledName to be const char * as the input parameter.

126–131

I'm concerned that if we add other encodings, this list will easily rot, so that auto doesn't detect it. What was wrong with the nonMicrosoftDemangle from before?

132

non_microsoft is out of date?

(Also missing trailing full stop).

139–141

[dblaikie]

rather than duplicating the is* functions, what if the demangle functions returned some failure that indicated "this isn't formatted for this mangling scheme" (sort of "pass, I can't demangle this, but I'm not saying it's incorrect, it's just not suitable for this scheme/algorithm/function")?

[thieta]

Several issues addressed in review.

[thieta]

llvm/tools/llvm-cxxfilt/Opts.td

8

Yes - I ran clang-format on it. But I should have done it as a NFC instead.

Reverted the clang-format stuff.

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

80

Exposed them for now.

126–131

Fair point. I moved this function from Demangle.cpp when it we didn't want to change the API there so I guess it just followed along. I changed it to use nonMicrosoftDemangle now.

I still think it would be nice if the API of Demangle.cpp was a bit more coherent, but that will be for another day.

[jhenderson]

No more comments from me, but please wait for @dblaikie to be happy.

llvm/docs/CommandGuide/llvm-cxxfilt.rst

45

Nit

[jhenderson]

Actually, don't you need tests to show that the new --format options work as expected?

[thieta]

In D138595#3957002, @jhenderson wrote:

Actually, don't you need tests to show that the new --format options work as expected?

You mean testing rust, dlang and itanium explicitly? I can definitely add that.

[jhenderson]

In D138595#3957034, @thieta wrote:

In D138595#3957002, @jhenderson wrote:

Actually, don't you need tests to show that the new --format options work as expected?

You mean testing rust, dlang and itanium explicitly? I can definitely add that.

Yep, that's what I meant (I think we have other testing that already tests that we can demangle these name types, so it should be fairly simple to ensure the option values are acceptable too).

[thieta]

Added format testing

[thieta]

Comment spelling and nit fix

[jhenderson]

Looks good. Please wait for @dblaikie.

[thieta]

Added release note.

[dblaikie]

In D138595#3951868, @dblaikie wrote:

rather than duplicating the is* functions, what if the demangle functions returned some failure that indicated "this isn't formatted for this mangling scheme" (sort of "pass, I can't demangle this, but I'm not saying it's incorrect, it's just not suitable for this scheme/algorithm/function")?

Ping on this ^?

[thieta]

In D138595#3957652, @dblaikie wrote:

Ping on this ^?

Ah sorry - I thought I replied to this, but I just replied in my head.

Yes, I like this idea, but it does feel like we are getting into the territory where we have to refactor the API to demangle since the demangle functions have different return values and handling.

I am happy to do this, but I am not sure it's part of this diff. IMHO it's probably better to land this and then refactor the demangle API in its own diff.

[dblaikie]

In D138595#3959825, @thieta wrote:

In D138595#3957652, @dblaikie wrote:

Ping on this ^?

Ah sorry - I thought I replied to this, but I just replied in my head.

Yes, I like this idea, but it does feel like we are getting into the territory where we have to refactor the API to demangle since the demangle functions have different return values and handling.

I am happy to do this, but I am not sure it's part of this diff. IMHO it's probably better to land this and then refactor the demangle API in its own diff.

I'd rather avoid the technical debt of duplicating the is* functions only to figure out how to remove them later - the refactoring could go in before this patch to make the APIs amenable to this sort of usage.

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

120

Adding recursion in here is a bit quirky (& calling this generic function only to ask for one particular mangling, when there's direct functions for specific manglings) - could this be another call to microsoftDemangle directly here?

[thieta]

In D138595#3961248, @dblaikie wrote:

In D138595#3959825, @thieta wrote:

In D138595#3957652, @dblaikie wrote:

Ping on this ^?

Ah sorry - I thought I replied to this, but I just replied in my head.

Yes, I like this idea, but it does feel like we are getting into the territory where we have to refactor the API to demangle since the demangle functions have different return values and handling.

I am happy to do this, but I am not sure it's part of this diff. IMHO it's probably better to land this and then refactor the demangle API in its own diff.

I'd rather avoid the technical debt of duplicating the is* functions only to figure out how to remove them later - the refactoring could go in before this patch to make the APIs amenable to this sort of usage.

Alright, I am fine with that. What approach would you prefer:

doing something in Demangle.cpp that wraps the current demangle functions.

Changing the different demangle functions to all use a similar api?

I am thinking we could make the functions return an optional and all take a const char* as input. Then we can return a false optional if it's not the right format or if it fails. Or we make it possible to return multiple errors so we know if it failed because of the format or something else (not sure this is that applicable since there can't be that many failure states). Any other thoughts?

[dblaikie]

I think the basic idea would be to maybe leave the existing xxxDemangle functions in place, as wrappers around more verbose functions with more precise error handling. (specifically for itaniumDemangle - it'd probably be good for the underlying function to not support fallback to type demangling, it'll be easier to fail out from there & the itaniumDemangle could rall in again and ask specifically for type demangling (there's at least one caller that's doing this already anyway))

So I guess, for instance, llvm::itaniumDemangle calls, say, llvm::itaniumDemangleWithError that returns Expected<char*> - then llvm::itaniumDemangle maps failures to nullptr (well, does the fallback to type name demangling if possible, then fails) as before, but llvm-cxxfilt can inspect the error and check if it's some particular custom Error introduced to communicate unrecognized name. Then fallback to the next one, and so on and so forth.

Does that sound feasible? @jhenderson does that sound reasonable? (got other ideas about how this should all be factored?)

To a comment earlier about duplicating/exposing the is**Encoding:

not sure we want to expose those API's since they don't seem 100% complete.

What did you mean by that?

(I personally like the idea of not having separate is* functions and then do* functions when the do* has to do the same work anyway & could report is/isn't in the process)

[jhenderson]

In D138595#3965634, @dblaikie wrote:

Does that sound feasible? @jhenderson does that sound reasonable? (got other ideas about how this should all be factored?)

Yeah, that all sounds sensible to me. I think I recall it bugging me that there's no easy way of telling whether demangling failed because it was e.g. Itanium (based on the leading characters), but not a valid such encoding, so having the ability to report an Error to the caller to help with this would certainly be good. The only issue with this approach is that I recall at one point that the demangling library is I recall something to do with the layering of the demangler library is such that it is depended upon by the Support library, and not the other way around. Alternatively, it might have been an issue with the fact that (in theory) Demangle.h is a copy of somewhere else, IIRC (there's a comment to this effect near the top of the file). I don't know if that's still an issue though.

Worst case, we could return some sort of enum error code or provide a callback function of some kind instead in place of llvm::Error.

[dblaikie]

In D138595#3965948, @jhenderson wrote:

In D138595#3965634, @dblaikie wrote:

Does that sound feasible? @jhenderson does that sound reasonable? (got other ideas about how this should all be factored?)

Yeah, that all sounds sensible to me. I think I recall it bugging me that there's no easy way of telling whether demangling failed because it was e.g. Itanium (based on the leading characters), but not a valid such encoding, so having the ability to report an Error to the caller to help with this would certainly be good. The only issue with this approach is that I recall at one point that the demangling library is I recall something to do with the layering of the demangler library is such that it is depended upon by the Support library, and not the other way around. Alternatively, it might have been an issue with the fact that (in theory) Demangle.h is a copy of somewhere else, IIRC (there's a comment to this effect near the top of the file). I don't know if that's still an issue though.

Worst case, we could return some sort of enum error code or provide a callback function of some kind instead in place of llvm::Error.

Ooooh, right. Yeah, that sounds like it's the case to me. So... so, std::error_code? (I thought maybe we had an LLVM error code-ish thing too, but I don't recall, maybe it's been fully replaced by llvm::Error, if it did exist) Pick a blessed value to communicate "this input isn't handled here, maybe someone else knows what it is" as distinct from "this input is mine, but it's broken".

[thieta]

New API draft here: https://reviews.llvm.org/D139298

llvm/docs/CommandGuide/llvm-cxxfilt.rst

.. code-block:: console
   $ llvm-cxxfilt < input.txt
   | foo() *** bar(int) *** not_mangled |
 
 OPTIONS
 -------
 
 .. option:: --format=<value>, -s
 
-  Mangling scheme to assume. Valid values are ``auto`` (default, auto-detect the
-  style) and ``gnu`` (assume GNU/Itanium style).
+  Mangling scheme to assume. Valid values are ``gnu`` (default, auto-detect the
+  style, but omit the ``microsoft`` format.), ``auto`` (auto-detect the style,
+  including ``microsoft``), ``itanium`` (assume Itanium format), ``dlang`` (assume
+  DLang format), ``rust`` (assume Rust format) and ``microsoft`` (assume Microsoft
+  format).
 
 .. option:: --help, -h
 
   Print a summary of command line options.
 
 .. option:: --no-strip-underscore, -n
 
   Do not strip a leading underscore. This is the default for all platforms

llvm/include/llvm/Demangle/Demangle.h

enum : int {
   demangle_success = 0,
 };
 
 char *itaniumDemangle(const char *mangled_name, char *buf, size_t *n,
                       int *status);
 
 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,
   MSDF_NoVariableType = 1 << 5,
 };
 
+/// These functions will return True if they look like the matching
+/// demangling format.
+bool isItaniumEncoding(const char *S);
+bool isRustEncoding(const char *S);
+bool isDLangEncoding(const std::string &MangledName);
+
 /// Demangles the Microsoft symbol pointed at by mangled_name and returns it.
 /// Returns a pointer to the start of a null-terminated demangled string on
 /// success, or nullptr on error.
 /// If n_read is non-null and demangling was successful, it receives how many
 /// bytes of the input string were consumed.
 /// buf can point to a *n_buf bytes large buffer where the demangled name is
 /// stored. If the buffer is too small, it is grown with realloc(). If buf is
 /// nullptr, then this malloc()s memory for the result.

llvm/lib/Demangle/Demangle.cpp

 //===-- Demangle.cpp - Common demangling functions ------------------------===//
 //
 // 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 contains definitions of common demangling functions.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Demangle/Demangle.h"
 #include <cstdlib>
 #include <cstring>
 
-static bool isItaniumEncoding(const char *S) {
+bool llvm::isItaniumEncoding(const char *S) {
   // Itanium encoding requires 1 or 3 leading underscores, followed by 'Z'.
   return std::strncmp(S, "_Z", 2) == 0 || std::strncmp(S, "___Z", 4) == 0;
 }
 
-static bool isRustEncoding(const char *S) { return S[0] == '_' && S[1] == 'R'; }
+bool llvm::isRustEncoding(const char *S) { return S[0] == '_' && S[1] == 'R'; }
 
-static bool isDLangEncoding(const std::string &MangledName) {
+bool llvm::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();
 

llvm/test/tools/llvm-cxxfilt/microsoft-format.test

+## Test microsoft demangle format.
+## This should only demangle microsofts format if format=microsoft or format=auto.
+## And fail if we set format=gnu.
+
+RUN: llvm-cxxfilt --format=microsoft ?a@@YAHD@Z ?c@b@@AAGXM@Z | FileCheck %s
+RUN: llvm-cxxfilt --format=auto ?a@@YAHD@Z ?c@b@@AAGXM@Z | FileCheck %s
+RUN: llvm-cxxfilt --format=gnu ?a@@YAHD@Z ?c@b@@AAGXM@Z | FileCheck %s --check-prefix=GNU
+
+CHECK: int __cdecl a(char)
+CHECK-NEXT: private: void __stdcall b::c(float)
+
+GNU: ?a@@YAHD@Z
+GNU-NEXT: ?c@b@@AAGXM@Z

llvm/tools/llvm-cxxfilt/Opts.td

 include "llvm/Option/OptParser.td"
 
 class F<string letter, string help> : Flag<["-"], letter>, HelpText<help>;
 class FF<string name, string help> : Flag<["--"], name>, HelpText<help>;
 
 multiclass BB<string name, string help1, string help2> {
   def NAME: Flag<["--"], name>, HelpText<help1>;
   def no_ # NAME: Flag<["--"], "no-" # name>, HelpText<help2>;
 }
 
 multiclass Eq<string name, string help> {
   def NAME #_EQ : Joined<["--"], name #"=">,
                   HelpText<help>;
   def : Separate<["--"], name>, Alias<!cast<Joined>(NAME #_EQ)>;
 }
 
 def help : FF<"help", "Display this help">;
 defm strip_underscore : BB<"strip-underscore", "Strip the leading underscore", "Don't strip the leading underscore">;
 def types : FF<"types", "Attempt to demangle types as well as function names">;
 def version : FF<"version", "Display the version">;
 
-defm : Eq<"format", "Specify mangling format. Currently ignored because only 'gnu' is supported">;
-def : F<"s", "Alias for --format">;
+defm format : Eq<"format", "Specify mangling format: gnu (default), itanium, "
+                           "dlang, rust, microsoft or auto">,
+              MetaVarName<"<format>">;
+def : JoinedOrSeparate<["-"], "s">,
+      Alias<format_EQ>,
+      HelpText<"Alias for --format">,
+      MetaVarName<"<format>">;
 
 def : F<"_", "Alias for --strip-underscore">, Alias<strip_underscore>;
 def : F<"h", "Alias for --help">, Alias<help>;
 def : F<"n", "Alias for --no-strip-underscore">, Alias<no_strip_underscore>;
 def : F<"t", "Alias for --types">, Alias<types>;

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/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Demangle/Demangle.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Host.h"
#define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
       PREFIX,      NAME,      HELPTEXT,                                        \
       METAVAR,     OPT_##ID,  opt::Option::KIND##Class,                        \
       PARAM,       FLAGS,     OPT_##GROUP,                                     \
       OPT_##ALIAS, ALIASARGS, VALUES},
 #include "Opts.inc"
 #undef OPTION
 };
 
+/// The different Demangle formats
+enum class DemangleFormatTy {
+  Itanium = 0,
+  DLang,
+  Rust,
+  GNU, // gnu means itaninum, dlang and rust
+  Microsoft,
+  AutoDetect
+};
+
+static DemangleFormatTy DemangleFormat;
+
 class CxxfiltOptTable : public opt::OptTable {
 public:
   CxxfiltOptTable() : OptTable(InfoTable) { setGroupedShortOptions(true); }
 };
 } // namespace
 
 static bool StripUnderscore;
 static bool Types;
 
 static StringRef ToolName;
 
 static void error(const Twine &Message) {
   WithColor::error(errs(), ToolName) << Message << '\n';
   exit(1);
 }
 
+// Demangle MangledName with specific DemangleFormat.
+// Return an llvm::Optional that's false if it couldn't
+// demangle the string.
+static llvm::Optional<std::string> demangleFormat(const char *MangledName,
+                                                  DemangleFormatTy Format) {
+  llvm::Optional<std::string> Result;
+  char *Demangled = nullptr;
+  int Status = demangle_unknown_error;
+
+  switch (Format) {
+  case DemangleFormatTy::Itanium:
+    if (isItaniumEncoding(MangledName)) {
+      Demangled = itaniumDemangle(MangledName, nullptr, nullptr, nullptr);
+      Status = demangle_success;
+    }
+    break;
+  case DemangleFormatTy::DLang:
+    if (isDLangEncoding(MangledName)) {
+      Demangled = dlangDemangle(MangledName);
+      Status = demangle_success;
+    }
+    break;
+  case DemangleFormatTy::Rust:
+    if (isRustEncoding(MangledName)) {
+      Demangled = rustDemangle(MangledName);
+      Status = demangle_success;
+    }
+    break;
+  case DemangleFormatTy::Microsoft:
+    Demangled =
+        microsoftDemangle(MangledName, nullptr, nullptr, nullptr, &Status);
+    break;
+  case DemangleFormatTy::AutoDetect:
+  case DemangleFormatTy::GNU:
+    std::string DM;
+    if (nonMicrosoftDemangle(MangledName, DM))
+      return DM;
+    // If the flag is GNU we don't want to run the code below, if it's auto we
+    // run it as well.
+    if (Format == DemangleFormatTy::AutoDetect)
+      if (auto D = demangleFormat(MangledName, DemangleFormatTy::Microsoft))
+        return D;
+    return Result;
+  }
+
+  if (Status == demangle_success && Demangled) {
+    Result = Demangled;
+    std::free(Demangled);
+  }
+
+  return Result;
+}
+
 static std::string demangle(const std::string &Mangled) {
   const char *DecoratedStr = Mangled.c_str();
   if (StripUnderscore)
     if (DecoratedStr[0] == '_')
       ++DecoratedStr;
 
-  std::string Result;
-  if (nonMicrosoftDemangle(DecoratedStr, Result))
-    return Result;
+  if (auto Result = demangleFormat(DecoratedStr, DemangleFormat))
+    return Result.value();
 
   std::string Prefix;
+  std::string Result = DecoratedStr;
   char *Undecorated = nullptr;
 
   if (Types)
     Undecorated = itaniumDemangle(DecoratedStr, nullptr, nullptr, nullptr);
 
   if (!Undecorated && strncmp(DecoratedStr, "__imp_", 6) == 0) {
     Prefix = "import thunk for ";
     Undecorated = itaniumDemangle(DecoratedStr + 6, nullptr, nullptr, nullptr);
if (Args.hasArg(OPT_help)) {
     return 0;
   }
   if (Args.hasArg(OPT_version)) {
     outs() << ToolName << '\n';
     cl::PrintVersionMessage();
     return 0;
   }
 
+  StringRef V = Args.getLastArgValue(OPT_format_EQ, "gnu");
+  if (V == "gnu")
+    DemangleFormat = DemangleFormatTy::GNU;
+  else if (V == "microsoft")
+    DemangleFormat = DemangleFormatTy::Microsoft;
+  else if (V == "auto")
+    DemangleFormat = DemangleFormatTy::AutoDetect;
+  else if (V == "dlang")
+    DemangleFormat = DemangleFormatTy::DLang;
+  else if (V == "rust")
+    DemangleFormat = DemangleFormatTy::Rust;
+  else if (V == "itanium")
+    DemangleFormat = DemangleFormatTy::Itanium;
+  else
+    error("--format value should be one of: gnu (default), itanium, dlang, "
+          "rust, microsoft or auto");
+
   // The default value depends on the default triple. Mach-O has symbols
   // prefixed with "_", so strip by default.
   if (opt::Arg *A =
           Args.getLastArg(OPT_strip_underscore, OPT_no_strip_underscore))
     StripUnderscore = A->getOption().matches(OPT_strip_underscore);
   else
     StripUnderscore = Triple(sys::getProcessTriple()).isOSBinFormatMachO();