[llvm-cxxfilt] Add --quote option to quote demangled function names #111871

keryell · 2024-10-10T16:59:36Z

This is useful when looking at LLVM/MLIR assembly produced from C++ sources. For example
cir.call @_ZN3aie4tileILi1ELi4EE7programIZ4mainE3$0EEvOT(%2, %7) :
will be translated to
cir.call @"void aie::tile<1, 4>::programmain::$_0(main::$_0&&)"(%2, %7) : which can be parsed as valid MLIR by the right mlir-lsp-server.

If a symbol is already quoted, do not quote it more.

This is useful when looking at LLVM/MLIR assembly produced from C++ sources. For example cir.call @_ZN3aie4tileILi1ELi4EE7programIZ4mainE3$_0EEvOT_(%2, %7) : will be translated to cir.call @"void aie::tile<1, 4>::program<main::$_0>(main::$_0&&)"(%2, %7) : which can be parsed as valid MLIR by the right mlir-lsp-server. If a symbol is already quoted, do not quote it more.

llvmbot · 2024-10-10T17:00:12Z

@llvm/pr-subscribers-llvm-binary-utilities

Author: Ronan Keryell (keryell)

Changes

This is useful when looking at LLVM/MLIR assembly produced from C++ sources. For example
cir.call @_ZN3aie4tileILi1ELi4EE7programIZ4mainE3$0EEvOT(%2, %7) :
will be translated to
cir.call @"void aie::tile<1, 4>::program<main::$_0>(main::$_0&&)"(%2, %7) : which can be parsed as valid MLIR by the right mlir-lsp-server.

If a symbol is already quoted, do not quote it more.

Full diff: https://github.com/llvm/llvm-project/pull/111871.diff

4 Files Affected:

(modified) llvm/docs/CommandGuide/llvm-cxxfilt.rst (+5)
(added) llvm/test/tools/llvm-cxxfilt/quote.test (+12)
(modified) llvm/tools/llvm-cxxfilt/Opts.td (+2)
(modified) llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp (+19-5)

diff --git a/llvm/docs/CommandGuide/llvm-cxxfilt.rst b/llvm/docs/CommandGuide/llvm-cxxfilt.rst
index 0933f0b5bed878..ea90a05bcd4338 100644
--- a/llvm/docs/CommandGuide/llvm-cxxfilt.rst
+++ b/llvm/docs/CommandGuide/llvm-cxxfilt.rst
@@ -52,6 +52,11 @@ OPTIONS
 
   Do not demangle function parameters or return types.
 
+.. option:: --quote, -q
+
+  Add `"` `"` around demangled function symbols, typically to keep LLVM/MLIR
+  assembly files with `@<symbols>` valid. Do not quote already quoted symbols.
+
 .. option:: --no-strip-underscore, -n
 
   Do not strip a leading underscore. This is the default for all platforms
diff --git a/llvm/test/tools/llvm-cxxfilt/quote.test b/llvm/test/tools/llvm-cxxfilt/quote.test
new file mode 100644
index 00000000000000..1420c14efbbfbc
--- /dev/null
+++ b/llvm/test/tools/llvm-cxxfilt/quote.test
@@ -0,0 +1,12 @@
+// Show that llvm-cxxfilt --quote can emit quoted demangled symbols but not
+// without double-quoting in the case they are already quoted
+
+RUN: llvm-cxxfilt --quote < %s | FileCheck %s
+      cir.call @_ZN3aie4tileILi1ELi4EE7programIZ4mainE3$_0EEvOT_(%2, %7) : (!cir.ptr<!ty_aie3A3Atile3C12C_43E>, !cir.ptr<!ty_anon2E0_>) -> () loc(#loc74)
+  cir.func lambda internal private  @_ZZ4mainENK3$_1clEv(%arg0: !cir.ptr<!ty_anon2E1_> loc("example.cpp":31:26)) extra(#fn_attr) {
+module @"example.cpp" attributes {cir.global_annotations = #cir<global_annotations [["_ZN3aie6deviceILNS_3$_0E42EE4tileILi1ELi4EEENS_4tileIXT_EXT0_EEEv", #cir.annotation<name = "aie.device.tile", args = [1 : i32, 4 : i32, 42 : i8, 42 : i8]>]]}
+
+CHECK: cir.call @"void aie::tile<1, 4>::program<main::$_0>(main::$_0&&)"(%2, %7) 
+CHECK-NEXT: cir.func lambda internal private  @"main::$_1::operator()() const"(%arg0: !cir.ptr<!ty_anon2E1_>
+// \todo Is there a simpler way to escape these [[ leading to "error: invalid variable name" otherwise?
+CHECK-NEXT: module @"example.cpp" attributes {cir.global_annotations = #cir<global_annotations {{[[]}}["aie::tile<1, 4> aie::device<(aie::$_0)42>::tile<1, 4>()", #cir.annotation<name = "aie.device.tile", args = [1 : i32, 4 : i32, 42 : i8, 42 : i8]>]]}
diff --git a/llvm/tools/llvm-cxxfilt/Opts.td b/llvm/tools/llvm-cxxfilt/Opts.td
index 034cb267aab800..e69082ba05e3a4 100644
--- a/llvm/tools/llvm-cxxfilt/Opts.td
+++ b/llvm/tools/llvm-cxxfilt/Opts.td
@@ -15,6 +15,7 @@ multiclass Eq<string name, string help> {
 }
 
 def help : FF<"help", "Display this help">;
+def quote : FF<"quote", "Quote demangled function names with \" \" if not already quoted. Useful for symbols appearing after an @ in MLIR/LLVM assembly">;
 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 no_params : FF<"no-params", "Skip function parameters and return types">;
@@ -27,4 +28,5 @@ 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<"p", "Alias for --no-params">, Alias<no_params>;
+def : F<"q", "Alias for --quote">, Alias<quote>;
 def : F<"t", "Alias for --types">, Alias<types>;
diff --git a/llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp b/llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
index f90adb6cacb990..f3f11e488d2c3d 100644
--- a/llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
+++ b/llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp
@@ -20,6 +20,7 @@
 #include "llvm/TargetParser/Triple.h"
 #include <cstdlib>
 #include <iostream>
+#include <string>
 
 using namespace llvm;
 
@@ -54,6 +55,7 @@ class CxxfiltOptTable : public opt::GenericOptTable {
 } // namespace
 
 static bool ParseParams;
+static bool Quote;
 static bool StripUnderscore;
 static bool Types;
 
@@ -64,7 +66,15 @@ static void error(const Twine &Message) {
   exit(1);
 }
 
-static std::string demangle(const std::string &Mangled) {
+// Quote Undecorated with "" if asked for and not already followed by a '"'
+static std::string optionalQuote(std::string Undecorated,
+                                 StringRef Delimiters) {
+  if (Quote && (Delimiters.empty() || Delimiters[0] != '"'))
+    return '"' + Undecorated + '"';
+  return Undecorated;
+}
+
+static std::string demangle(const std::string &Mangled, StringRef Delimiters) {
   using llvm::itanium_demangle::starts_with;
   std::string_view DecoratedStr = Mangled;
   bool CanHaveLeadingDot = true;
@@ -76,7 +86,7 @@ static std::string demangle(const std::string &Mangled) {
   std::string Result;
   if (nonMicrosoftDemangle(DecoratedStr, Result, CanHaveLeadingDot,
                            ParseParams))
-    return Result;
+    return optionalQuote(Result, Delimiters);
 
   std::string Prefix;
   char *Undecorated = nullptr;
@@ -89,7 +99,8 @@ static std::string demangle(const std::string &Mangled) {
     Undecorated = itaniumDemangle(DecoratedStr.substr(6), ParseParams);
   }
 
-  Result = Undecorated ? Prefix + Undecorated : Mangled;
+  Result =
+      Undecorated ? Prefix + optionalQuote(Undecorated, Delimiters) : Mangled;
   free(Undecorated);
   return Result;
 }
@@ -137,9 +148,10 @@ static void demangleLine(llvm::raw_ostream &OS, StringRef Mangled, bool Split) {
     SmallVector<std::pair<StringRef, StringRef>, 16> Words;
     SplitStringDelims(Mangled, Words, IsLegalItaniumChar);
     for (const auto &Word : Words)
-      Result += ::demangle(std::string(Word.first)) + Word.second.str();
+      Result +=
+          ::demangle(std::string(Word.first), Word.second) + Word.second.str();
   } else
-    Result = ::demangle(std::string(Mangled));
+    Result = ::demangle(std::string(Mangled), "");
   OS << Result << '\n';
   OS.flush();
 }
@@ -172,6 +184,8 @@ int llvm_cxxfilt_main(int argc, char **argv, const llvm::ToolContext &) {
 
   Types = Args.hasArg(OPT_types);
 
+  Quote = Args.hasArg(OPT_quote);
+
   std::vector<std::string> Decorated = Args.getAllArgValues(OPT_INPUT);
   if (Decorated.empty())
     for (std::string Mangled; std::getline(std::cin, Mangled);)

keryell · 2024-10-10T17:03:48Z

This is a follow-up of llvm/clangir#824 suggested by @bcardosolopes.

jh7370

Thanks for this: the core of it looks fine. Just some testing could be simplified and improved, in my opinion, and there are a few other minor bits and pieces.

jh7370 · 2024-10-11T07:04:38Z

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

@@ -64,7 +66,15 @@ static void error(const Twine &Message) {
  exit(1);
 }

-static std::string demangle(const std::string &Mangled) {
+// Quote Undecorated with "" if asked for and not already followed by a '"'
+static std::string optionalQuote(std::string Undecorated,


Should Undecorated be a const std::string & or StringRef?

I did this to have the compiler able to move the string from the callee to the caller in the case it is not to be quoted, which is, I guess, the common case. But I might be wrong.

I think it's likely this function call will get optimized away, so it's somewhat moot, and we should just do what requires less mental logistics by readers of the code (which typically would not expect a string to be passed by value).

jh7370 · 2024-10-11T07:19:20Z

llvm/test/tools/llvm-cxxfilt/quote.test

+// Show that llvm-cxxfilt --quote can emit quoted demangled symbols but not
+// without double-quoting in the case they are already quoted


Suggested change

// Show that llvm-cxxfilt --quote can emit quoted demangled symbols but not

// without double-quoting in the case they are already quoted

// Show that llvm-cxxfilt --quote can emit quoted demangled symbols but not

// without double-quoting in the case they are already quoted.

Nit.

jh7370 · 2024-10-11T07:28:33Z

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

@@ -20,6 +20,7 @@
 #include "llvm/TargetParser/Triple.h"
 #include <cstdlib>
 #include <iostream>
+#include <string>


Nit: std::string is already used by this file, so adding this seems unnecessary by my reading of the include rules in the style guide.

This is added automatically by clangd LSP server using the project configuration, so I guess it is cleaner according to the official coding style but I can remove it.

jh7370 · 2024-10-11T07:36:10Z

llvm/test/tools/llvm-cxxfilt/quote.test

+// without double-quoting in the case they are already quoted
+
+RUN: llvm-cxxfilt --quote < %s | FileCheck %s
+      cir.call @_ZN3aie4tileILi1ELi4EE7programIZ4mainE3$_0EEvOT_(%2, %7) : (!cir.ptr<!ty_aie3A3Atile3C12C_43E>, !cir.ptr<!ty_anon2E0_>) -> () loc(#loc74)


It looks to me like this test is overly complicated for what actually needs testing. If I understand it correctly, an input of something like the following would be sufficient to test the case with and without a quote:

_Z3barv "_Z3barv"

which I believe would turn into:

"bar()" "bar()"

(or if there was a bug, it might become `"bar()" ""bar()"", for example)

I'd also add two additional test cases:

A case that doesn't result in demangling (to show that only successfully demangled names are quoted)

A case that shows the behaviour if --quote is specified with the mangled name as one of the command-line arguments (e.g. llvm-cxxfilt --quote _Z3barv). I don't think quotes can appear in mangled names, so I don't think we need to show the "no double-quoting" behaviour in this case.

A case that exercises the behaviour when an "import thunk for" prefix is added. Aside: I wonder if this case should be always quoted with --quote?

Finally, it may make the test cleaner to not use < %s and instead redirect the stdin cases from a separate file, possibly created via split-file. This would prevent things like the RUN and CHECK lines themselves from being fed into llvm-cxxfilt. This would also allow you to add --match-full-lines or similar, to make sure there's no unexpected quotes being added, since CHECK: bar() doesn't prevent quotes from existing.

--match-full-lines sounds like a good idea if the tested lines are simplified as you propose.

I don't think quotes can appear in mangled names, so I don't think we need to show the "no double-quoting" behaviour in this case.

I have added it anyway for completude.
I am unfamiliar with the "import thunk for". Is this something we expect to show up in LLVM or MLIR dialect assembly file?
The goal is to produce a clearer self-content LLVM/MLIR file but not something which can be linked into something sensible.

I am unfamiliar with the "import thunk for". Is this something we expect to show up in LLVM or MLIR dialec assembly filet?

I don't know, but possibly? I don't think it really matters where it can appear though, since its behaviour can be impacted by the new option (see the code around line 102 in llvm-cxxfilt.cpp in this PR, where you've modified it - in particular, I'm interested in testing the behaviour of Prefix in combination with the quoting).

You are right, this is more comprehensive to have this case to work too.

jh7370 · 2024-10-11T07:48:52Z

llvm/docs/CommandGuide/llvm-cxxfilt.rst

+  Add `"` `"` around demangled function symbols, typically to keep LLVM/MLIR
+  assembly files with `@<symbols>` valid. Do not quote already quoted symbols.


I can imagine other cases where the quoting behaviour might be useful, so I'd be inclined to drop the "typically to keep ..." part of this sentence.

I thought it would be more inclusive and compassionate to give a use case 😃 but I can remove it.

jh7370 · 2024-10-11T07:49:30Z

llvm/tools/llvm-cxxfilt/Opts.td

@@ -15,6 +15,7 @@ multiclass Eq<string name, string help> {
 }

 def help : FF<"help", "Display this help">;
+def quote : FF<"quote", "Quote demangled function names with \" \" if not already quoted. Useful for symbols appearing after an @ in MLIR/LLVM assembly">;


Similar comment to above, let's drop the "Useful for" bit of this.

jh7370 · 2024-10-11T07:51:21Z

llvm/tools/llvm-cxxfilt/Opts.td

@@ -27,4 +28,5 @@ 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<"p", "Alias for --no-params">, Alias<no_params>;
+def : F<"q", "Alias for --quote">, Alias<quote>;


Please don't add single-character aliases for options that don't exist in the GNU version of the tool (c++filt), as it becomes a problem if they add their own -q option (we aim for compatibility with the GNU binutils). That is unless you are proposing to the binutils list to add it too...

I did not know this.

@jh7370

Thanks @jh7370

keryell

Thanks for the detailed feedback!

keryell · 2024-10-12T00:43:25Z

llvm/docs/CommandGuide/llvm-cxxfilt.rst

+  Add `"` `"` around demangled function symbols, typically to keep LLVM/MLIR
+  assembly files with `@<symbols>` valid. Do not quote already quoted symbols.


I thought it would be more inclusive and compassionate to give a use case 😃 but I can remove it.

keryell · 2024-10-12T00:45:47Z

llvm/tools/llvm-cxxfilt/Opts.td

@@ -27,4 +28,5 @@ 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<"p", "Alias for --no-params">, Alias<no_params>;
+def : F<"q", "Alias for --quote">, Alias<quote>;


I did not know this.

keryell · 2024-10-12T00:56:26Z

llvm/test/tools/llvm-cxxfilt/quote.test

+// without double-quoting in the case they are already quoted
+
+RUN: llvm-cxxfilt --quote < %s | FileCheck %s
+      cir.call @_ZN3aie4tileILi1ELi4EE7programIZ4mainE3$_0EEvOT_(%2, %7) : (!cir.ptr<!ty_aie3A3Atile3C12C_43E>, !cir.ptr<!ty_anon2E0_>) -> () loc(#loc74)


--match-full-lines sounds like a good idea if the tested lines are simplified as you propose.

I don't think quotes can appear in mangled names, so I don't think we need to show the "no double-quoting" behaviour in this case.

I have added it anyway for completude.
I am unfamiliar with the "import thunk for". Is this something we expect to show up in LLVM or MLIR dialect assembly file?
The goal is to produce a clearer self-content LLVM/MLIR file but not something which can be linked into something sensible.

keryell · 2024-10-12T01:05:36Z

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

@@ -64,7 +66,15 @@ static void error(const Twine &Message) {
  exit(1);
 }

-static std::string demangle(const std::string &Mangled) {
+// Quote Undecorated with "" if asked for and not already followed by a '"'
+static std::string optionalQuote(std::string Undecorated,


I did this to have the compiler able to move the string from the callee to the caller in the case it is not to be quoted, which is, I guess, the common case. But I might be wrong.

jh7370

@keryell, for some reason your follow-up commit (keryell/clangir@d3553f3) isn't showing up in the PR, so I can't review it. Please push the commit to the branch used for this PR.

jh7370 · 2024-10-14T07:06:34Z

llvm/test/tools/llvm-cxxfilt/quote.test

+// without double-quoting in the case they are already quoted
+
+RUN: llvm-cxxfilt --quote < %s | FileCheck %s
+      cir.call @_ZN3aie4tileILi1ELi4EE7programIZ4mainE3$_0EEvOT_(%2, %7) : (!cir.ptr<!ty_aie3A3Atile3C12C_43E>, !cir.ptr<!ty_anon2E0_>) -> () loc(#loc74)


I am unfamiliar with the "import thunk for". Is this something we expect to show up in LLVM or MLIR dialec assembly filet?

I don't know, but possibly? I don't think it really matters where it can appear though, since its behaviour can be impacted by the new option (see the code around line 102 in llvm-cxxfilt.cpp in this PR, where you've modified it - in particular, I'm interested in testing the behaviour of Prefix in combination with the quoting).

jh7370 · 2024-10-14T07:11:39Z

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

@@ -64,7 +66,15 @@ static void error(const Twine &Message) {
  exit(1);
 }

-static std::string demangle(const std::string &Mangled) {
+// Quote Undecorated with "" if asked for and not already followed by a '"'
+static std::string optionalQuote(std::string Undecorated,


I think it's likely this function call will get optimized away, so it's somewhat moot, and we should just do what requires less mental logistics by readers of the code (which typically would not expect a string to be passed by value).

keryell · 2024-10-15T17:33:10Z

@keryell, for some reason your follow-up commit (keryell/clangir@d3553f3) isn't showing up in the PR, so I can't review it. Please push the commit to the branch used for this PR.

Sorry about that. I have pushed on the wrong remote I have on this worktree.

jh7370 · 2024-10-16T07:42:06Z

llvm/docs/CommandGuide/llvm-cxxfilt.rst

+  Add `"` `"` around demangled function symbols.
+  Do not quote already quoted symbols.


Nit: word wrapping seems premature.

llvm/test/tools/llvm-cxxfilt/quote.test

jh7370 · 2024-10-16T07:45:14Z

llvm/test/tools/llvm-cxxfilt/quote.test

+RUN: echo _Z3barv '"_Z3barv"' > %t
+RUN: echo '"_Z3barv"' _Z3barv >> %t
+// This is not mangled, thus it should not be quoted
+RUN: echo 'log()' >> %t
+// Check that an "import thunk for" prefix can be quoted along the demangled name
+RUN: echo __imp__ZSt6futureIvE >> %t


Rather than use echo, I'd recommend the more modern split-file approach. In essence, you add a marker later in the file, then write the intended contents of the input file below that marker. In place of the echo lines, you simply add RUN: split-file %s .... See various existing examples for correct usage.

Interesting!

jh7370 · 2024-10-16T07:46:17Z

llvm/test/tools/llvm-cxxfilt/quote.test

-// \todo Is there a simpler way to escape these [[ leading to "error: invalid variable name" otherwise?
-CHECK-NEXT: module @"example.cpp" attributes {cir.global_annotations = #cir<global_annotations {{[[]}}["aie::tile<1, 4> aie::device<(aie::$_0)42>::tile<1, 4>()", #cir.annotation<name = "aie.device.tile", args = [1 : i32, 4 : i32, 42 : i8, 42 : i8]>]]}
+// Check it works with cli symbols too. Since a quoted mangled name is not a
+// mangled name, it should be unchanged


Suggested change

// mangled name, it should be unchanged

// mangled name, it should be unchanged.

Nit.

Not addressed?

It looks like I messed up in conflict resolution when I rebased my commit on top of the one I made on GitHub UI with your suggestions.

jh7370 · 2024-10-16T07:46:43Z

llvm/test/tools/llvm-cxxfilt/quote.test

+CHECK-CLI: "_Z3barv"
+CHECK-CLI: saw()


-NEXT prefixes?

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

Co-authored-by: James Henderson <46713263+jh7370@users.noreply.github.com>

@jh7370

Apply reviews from @jh7370.

jh7370

Looks good, barring the remaining nits. I'll give it one more once-over once you've addressed those to make sure I haven't missed anything, before formally approving.

jh7370 · 2024-10-17T07:38:49Z

llvm/test/tools/llvm-cxxfilt/quote.test

-// Check that an "import thunk for" prefix can be quoted along the demangled name
-RUN: echo __imp__ZSt6futureIvE >> %t
-RUN: llvm-cxxfilt --quote < %t | FileCheck --match-full-lines --check-prefix=CHECK-FILE %s
+// Show that llvm-cxxfilt --quote adds quotes around demangled symbols, unless the symbol is already quoted.


Nit: sorry, thought I said this, but please reflow comments to approximately 80 characters, just like in regular code.

This is my usual coding style actually, so I love this.

jh7370 · 2024-10-17T07:39:28Z

llvm/test/tools/llvm-cxxfilt/quote.test

-// \todo Is there a simpler way to escape these [[ leading to "error: invalid variable name" otherwise?
-CHECK-NEXT: module @"example.cpp" attributes {cir.global_annotations = #cir<global_annotations {{[[]}}["aie::tile<1, 4> aie::device<(aie::$_0)42>::tile<1, 4>()", #cir.annotation<name = "aie.device.tile", args = [1 : i32, 4 : i32, 42 : i8, 42 : i8]>]]}
+// Check it works with cli symbols too. Since a quoted mangled name is not a
+// mangled name, it should be unchanged


Not addressed?

jh7370 · 2024-10-17T07:40:05Z

llvm/test/tools/llvm-cxxfilt/quote.test

+//--- symbols-in-file.test
+_Z3barv "_Z3barv"
+"_Z3barv" _Z3barv
+// This is not mangled, thus it should not be quoted


Also missing "." here and below - comments should always end with one, just like in regular code.

@jh7370

Apply reviews from @jh7370.

keryell

Thanks for all the review feedback!

keryell · 2024-10-17T18:17:28Z

llvm/test/tools/llvm-cxxfilt/quote.test

-// \todo Is there a simpler way to escape these [[ leading to "error: invalid variable name" otherwise?
-CHECK-NEXT: module @"example.cpp" attributes {cir.global_annotations = #cir<global_annotations {{[[]}}["aie::tile<1, 4> aie::device<(aie::$_0)42>::tile<1, 4>()", #cir.annotation<name = "aie.device.tile", args = [1 : i32, 4 : i32, 42 : i8, 42 : i8]>]]}
+// Check it works with cli symbols too. Since a quoted mangled name is not a
+// mangled name, it should be unchanged


It looks like I messed up in conflict resolution when I rebased my commit on top of the one I made on GitHub UI with your suggestions.

keryell · 2024-10-17T18:21:05Z

llvm/test/tools/llvm-cxxfilt/quote.test

-// Check that an "import thunk for" prefix can be quoted along the demangled name
-RUN: echo __imp__ZSt6futureIvE >> %t
-RUN: llvm-cxxfilt --quote < %t | FileCheck --match-full-lines --check-prefix=CHECK-FILE %s
+// Show that llvm-cxxfilt --quote adds quotes around demangled symbols, unless the symbol is already quoted.


This is my usual coding style actually, so I love this.

jh7370

LGTM, with one exception - please reorder the documentation entry to be in alphabetical order.

jh7370 · 2024-10-18T07:29:58Z

llvm/docs/CommandGuide/llvm-cxxfilt.rst

@@ -52,6 +52,11 @@ OPTIONS

  Do not demangle function parameters or return types.

+.. option:: --quote


Nit: This should appear after --no-strip-underscore to maintain alphabetical order.

That makes sense.

@jh7370

Apply reviews from @jh7370.

keryell

Please feel free to merge this as I do not have write access to the repository.

llvmbot added the llvm:binary-utilities label Oct 10, 2024

keryell mentioned this pull request Oct 10, 2024

Keep unmangled fully qualified function name in cir.func llvm/clangir#824

Open

jh7370 requested changes Oct 11, 2024

View reviewed changes

[llvm-cxxfilt] Apply PR review feedback

d3553f3

Thanks @jh7370

keryell commented Oct 12, 2024

View reviewed changes

jh7370 reviewed Oct 14, 2024

View reviewed changes

[llvm-cxxfilt] Make --quote to work with "import thunk for" prefix

0ca7195

jh7370 reviewed Oct 16, 2024

View reviewed changes

keryell and others added 2 commits October 16, 2024 11:58

Commit suggestion on llvm/test/tools/llvm-cxxfilt/quote.test

ecd1d77

Co-authored-by: James Henderson <46713263+jh7370@users.noreply.github.com>

[llvm-cxxfilt][NFC] Use split-file to avoid echo abuse

2e070db

Apply reviews from @jh7370.

jh7370 reviewed Oct 17, 2024

View reviewed changes

[llvm-cxxfilt][NFC] More comment cleaning

8867ce7

Apply reviews from @jh7370.

keryell commented Oct 17, 2024

View reviewed changes

jh7370 approved these changes Oct 18, 2024

View reviewed changes

[llvm-cxxfilt][NFC] Insert new option in a sorted way

f4f4799

Apply reviews from @jh7370.

keryell commented Oct 18, 2024

View reviewed changes

Merge branch 'main' into ronan/cxxfilt-quote

634979c

		// Show that llvm-cxxfilt --quote can emit quoted demangled symbols but not
		// without double-quoting in the case they are already quoted

		Add `"` `"` around demangled function symbols, typically to keep LLVM/MLIR
		assembly files with `@<symbols>` valid. Do not quote already quoted symbols.

		Add `"` `"` around demangled function symbols.
		Do not quote already quoted symbols.

	// mangled name, it should be unchanged
	// mangled name, it should be unchanged.

		@@ -52,6 +52,11 @@ OPTIONS

		Do not demangle function parameters or return types.

		.. option:: --quote

		CHECK-CLI: "_Z3barv"
		CHECK-CLI: saw()

[llvm-cxxfilt] Add --quote option to quote demangled function names #111871

Are you sure you want to change the base?

[llvm-cxxfilt] Add --quote option to quote demangled function names #111871

Conversation

keryell commented Oct 10, 2024

llvmbot commented Oct 10, 2024

keryell commented Oct 10, 2024

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell left a comment

Choose a reason for hiding this comment

keryell Oct 12, 2024 •

edited

Loading

keryell Oct 12, 2024 •

edited

Loading