[NFC][llvm-xray] add a llvm-xray convert option `no-demangle`

[Enna1]

Summary

When option --symbolize is true, llvm-xray convert will demangle function name on default.

This patch adds a llvm-xray convert option no-demangle to determine whether to demangle function name when symbolizing function ids from the input log

[smeenai]

Looks straightforward enough, but is it possible to add a test?

You should fix the clang-format issue. If you install clang-format locally (either one you build yourself or a prebuilt downloaded one), arc lint should fix formatting for you automatically.

[Enna1]

Thanks @smeenai !
I fixed the clang-format issue, and added a test for the option symbolize-no-demangle.

[smeenai]

LGTM. Do you have commit access or do you need this committed for you?

llvm/tools/llvm-xray/xray-converter.cpp

63

Minor grammar nit

[smeenai]

Looks good, let me know if you need this to be committed for you.

[Enna1]

Thanks! I dot not have commit access, please commit this for me.

[smeenai]

In D108019#2945945, @Enna1 wrote:

Thanks! I dot not have commit access, please commit this for me.

What name and email should I commit this under?

[Enna1]

In D108019#2945946, @smeenai wrote:

In D108019#2945945, @Enna1 wrote:

Thanks! I dot not have commit access, please commit this for me.

What name and email should I commit this under?

Name: Xu Mingjie
Email: [e-mail]

Thanks again!

[MaskRay]

The convention is --no-symbolize-demangle

[MaskRay]

Perhaps switch to no-demangle by default and add --symbolize-demangle?

[MaskRay]

llvm/test/tools/llvm-xray/X86/convert-with-debug-syms-no-demangle.txt

1 ↗

(On Diff #366540)

This can be added to X86/extract-instrmap-symbolize.ll rather than add a new file.

[Enna1]

In D108019#2945953, @MaskRay wrote:

Perhaps switch to no-demangle by default and add --symbolize-demangle?

We should provide a consistent naming with llvm-xray extract ? llvm-xray extract use an option no-demangle and set no-demangle to false on default.

llvm/test/tools/llvm-xray/X86/convert-with-debug-syms-no-demangle.txt

1 ↗

(On Diff #366540)

This can be added to X86/extract-instrmap-symbolize.ll rather than add a new file.

llvm xray convert and llvm xray extract are different subcommand, to avoid adding a new file, perhaps add this to X86/convert-with-debug-syms.txt ?

[MaskRay]

llvm/test/tools/llvm-xray/X86/convert-with-debug-syms-no-demangle.txt

1 ↗

(On Diff #366540)

SG.

Please also avoid options like --foo=true.
It works just because llvm CommandLine.cpp has such behaviors. Such options are not guaranteed to work in the future for user-facing utilities.

[MTC]

In D108019#2945953, @MaskRay wrote:

Perhaps switch to no-demangle by default and add --symbolize-demangle?

Changing the default behavior may break current usages. It's not a good choice. Perhaps we can leave this choice to the future.

[MaskRay]

In D108019#2945958, @Enna1 wrote:

In D108019#2945953, @MaskRay wrote:

Perhaps switch to no-demangle by default and add --symbolize-demangle?

We should provide a consistent naming with llvm-xray extract ? llvm-xray extract use an option no-demangle and set no-demangle to false on default.

Now I have taken a closer look. --no-demangle from extract can be shared with convert. --symbolize is similarly used by both commands.

Changing the default behavior may break current usages. It's not a good choice. Perhaps we can leave this choice to the future.

function was a late addition. --no-demangle was added to extract. Now there is a request for convert.
This made we wonder whether demangle is a good default choice.
If not, surely we should be able to change it.
Changing the default should not be taken lightly, but I'd not say such change is blocked because some users can be reluctant to change.

I'm going to add --demangle first.

[Enna1]

add test to X86/convert-with-debug-syms.txt instead of adding a new file.

[Enna1]

rename --symbolize-no-demangle to --no-symbolize-demangle

[MaskRay]

Sorry for the back and forth, but later I suggested --no-demangle

[Enna1]

Sorry for the back and forth, but later I suggested --no-demangle

OK, renamed to --no-demangle, this will be consistent with https://reviews.llvm.org/D108100 :)

llvm/tools/llvm-xray/xray-converter.cpp

static cl::alias ConvertOutput2("o", cl::aliasopt(ConvertOutput),
                                 cl::desc("Alias for -output"));
 
 static cl::opt<bool>
     ConvertSymbolize("symbolize",
                      cl::desc("symbolize function ids from the input log"),
                      cl::init(false), cl::sub(Convert));
 static cl::alias ConvertSymbolize2("y", cl::aliasopt(ConvertSymbolize),
                                    cl::desc("Alias for -symbolize"));
+static cl::opt<bool>
+    ConvertSymbolizeNoDemangle("symbolize-no-demangle",
+                     cl::desc("determines whether to demangle function name "
+                              "when symbolize function ids from the input log"),
+                     cl::init(false), cl::sub(Convert));
 
 static cl::opt<std::string>
     ConvertInstrMap("instr_map",
                     cl::desc("binary with the instrumentation map, or "
                              "a separate instrumentation map"),
                     cl::value_desc("binary with xray_instr_map"),
                     cl::sub(Convert), cl::init(""));
 static cl::alias ConvertInstrMap2("m", cl::aliasopt(ConvertInstrMap),
if (!InstrumentationMapOrError)
                             Twine("Cannot open instrumentation map '") +
                                 ConvertInstrMap + "'",
                             std::make_error_code(std::errc::invalid_argument)),
                         InstrumentationMapOrError.takeError());
     Map = std::move(*InstrumentationMapOrError);
   }
 
   const auto &FunctionAddresses = Map.getFunctionAddresses();
-  symbolize::LLVMSymbolizer Symbolizer;
+  symbolize::LLVMSymbolizer::Options SymbolizerOpts;
+  if (ConvertSymbolizeNoDemangle)
+    SymbolizerOpts.Demangle = false;
+  symbolize::LLVMSymbolizer Symbolizer(SymbolizerOpts);
   llvm::xray::FuncIdConversionHelper FuncIdHelper(ConvertInstrMap, Symbolizer,
                                                   FunctionAddresses);
   llvm::xray::TraceConverter TC(FuncIdHelper, ConvertSymbolize);
   std::error_code EC;
   raw_fd_ostream OS(ConvertOutput, EC,
                     ConvertOutputFormat == ConvertFormats::BINARY
                         ? sys::fs::OpenFlags::OF_None
                         : sys::fs::OpenFlags::OF_TextWithCRLF);