Page MenuHomePhabricator

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

Authored by Enna1 on Aug 13 2021, 12:28 AM.

Details

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

Diff Detail

Event Timeline

Enna1 requested review of this revision.Aug 13 2021, 12:28 AM
Enna1 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2021, 12:28 AM
Enna1 updated this revision to Diff 366205.Aug 13 2021, 12:29 AM
MTC added a subscriber: MTC.Aug 13 2021, 1:14 AM
Enna1 updated this revision to Diff 366215.Aug 13 2021, 1:28 AM

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 updated this revision to Diff 366389.Aug 13 2021, 7:12 PM
Enna1 edited the summary of this revision. (Show Details)
Enna1 added a comment.Aug 13 2021, 8:11 PM

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

smeenai accepted this revision.Aug 15 2021, 4:28 PM

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

This revision is now accepted and ready to land.Aug 15 2021, 4:28 PM
Enna1 updated this revision to Diff 366540.Aug 15 2021, 6:10 PM

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

Enna1 added a comment.Aug 15 2021, 6:51 PM

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

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

What name and email should I commit this under?

Enna1 added a comment.Aug 15 2021, 7:15 PM

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: xumingjie.enna1@bytedance.com

Thanks again!

MaskRay requested changes to this revision.Aug 15 2021, 7:27 PM

The convention is --no-symbolize-demangle

This revision now requires changes to proceed.Aug 15 2021, 7:27 PM
MaskRay added a comment.EditedAug 15 2021, 7:28 PM

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

MaskRay added inline comments.Aug 15 2021, 7:31 PM
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 added a comment.Aug 15 2021, 7:46 PM

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 added inline comments.Aug 15 2021, 7:56 PM
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 added a comment.Aug 15 2021, 8:08 PM

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 added a comment.EditedAug 15 2021, 8:31 PM

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 updated this revision to Diff 366545.Aug 15 2021, 8:52 PM

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

MTC added a reviewer: MTC.Aug 15 2021, 10:45 PM
Enna1 updated this revision to Diff 366554.Aug 15 2021, 11:22 PM
Enna1 retitled this revision from [NFC][llvm-xray] add a llvm-xray convert option `symbolize-no-demangle` to [NFC][llvm-xray] add a llvm-xray convert option `no-symbolize-demangle`.
Enna1 edited the summary of this revision. (Show Details)

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

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

Enna1 updated this revision to Diff 367085.Aug 17 2021, 6:55 PM
Enna1 retitled this revision from [NFC][llvm-xray] add a llvm-xray convert option `no-symbolize-demangle` to [NFC][llvm-xray] add a llvm-xray convert option `no-demangle`.
Enna1 edited the summary of this revision. (Show Details)

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 :)

Enna1 updated this revision to Diff 367096.Aug 17 2021, 7:36 PM
Enna1 marked an inline comment as done.
MaskRay accepted this revision.Aug 17 2021, 7:48 PM
This revision is now accepted and ready to land.Aug 17 2021, 7:48 PM
This revision was landed with ongoing or failed builds.Aug 17 2021, 9:22 PM
This revision was automatically updated to reflect the committed changes.