[PATCH] D138595: [llvm-cxxfilt] Support Microsoft demangling format

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-cxxfilt/microsoft-format.test:1-3
+## Test microsoft demangle format
+## This should only demangle microsofts format if format=microsoft or format=auto
+## And fail if we set format=gnu
----------------
Nit: these should all end in full stops like code comments.


================
Comment at: llvm/tools/llvm-cxxfilt/Opts.td:7
 multiclass BB<string name, string help1, string help2> {
-  def NAME: Flag<["--"], name>, HelpText<help1>;
-  def no_ # NAME: Flag<["--"], "no-" # name>, HelpText<help2>;
+  def NAME : Flag<["--"], name>, HelpText<help1>;
+  def no_ #NAME : Flag<["--"], "no-" #name>, HelpText<help2>;
----------------
Seems like there's a lot of unrelated noise in this file?


================
Comment at: llvm/tools/llvm-cxxfilt/Opts.td:23
 
-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">,
----------------
Docs should be updated with the new `--format` values.


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:53
+/// The different Demangle formats
+enum DemangleFormatTy {
+  itanium = 0,
----------------
`enum class` maybe to avoid polluting the global namespace?


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:54-59
+  itanium = 0,
+  dlang,
+  rust,
+  gnu, // gnu means itaninum, dlang and rust
+  microsoft,
+  autodetect
----------------
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly (`Itanium`, `Rust` etc).


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:80
 
+static bool isItaniumEncoding(const char *S) {
+  // Itanium encoding requires 1 or 3 leading underscores, followed by 'Z'.
----------------
thieta wrote:
> 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.
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?


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:92
+
+// Demangle MangledName with specific DemangleFormat
+// Return a llvm::Optional that's false if it couldn't
----------------



================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:93
+// Demangle MangledName with specific DemangleFormat
+// Return a llvm::Optional that's false if it couldn't
+// demangle the string.
----------------



================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:96-97
+static llvm::Optional<std::string>
+demangleFormat(const std::string &MangledName, DemangleFormatTy Format) {
+  const char *S = MangledName.c_str();
+  llvm::Optional<std::string> Result;
----------------
It would probably make more sense for `MangledName` to be `const char *` as the input parameter.


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:126-131
+    if (auto D = demangleFormat(MangledName, itanium))
+      return D;
+    if (auto D = demangleFormat(MangledName, rust))
+      return D;
+    if (auto D = demangleFormat(MangledName, dlang))
+      return D;
----------------
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?


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:132
+      return D;
+    // If the flag is non_microsoft we don't want to run the code below
+    if (Format == autodetect)
----------------
`non_microsoft` is out of date?

(Also missing trailing full stop).


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:153-155
+  if (auto Result = demangleFormat(DecoratedStr, DemangleFormat)) {
+    return Result.value();
+  }
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138595/new/

https://reviews.llvm.org/D138595