Page MenuHomePhabricator

Don't use $ as suffix for symbol names in ThinLTOBitcodeWriter and other places
ClosedPublic

Authored by hans on Feb 25 2021, 9:15 AM.

Details

Summary

Using $ breaks demangling of the symbols. For example,

$ c++filt _Z3foov\$123
_Z3foov$123

This causes problems for developers who would like to see nice stack traces etc. but also for automatic crash tracking systems which try to organize crashes based on the stack traces.

Instead, use the period as suffix separator, since Itanium demanglers normally ignore such suffixes:

$ c++filt _Z3foov.123
foo() [clone .123]

This is already done in some places; create a utility function to do it, and try to use that everywhere.

Diff Detail

Event Timeline

hans created this revision.Feb 25 2021, 9:15 AM
hans requested review of this revision.Feb 25 2021, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 9:15 AM

Why isn't the solution is to fix the uses to prevent variable substitution in them?
Because of course $1 is substituted with a value of variable 1 in shell, and elsewhere.

$ c++filt _Z3foov\$123
_Z3foov$123
hans added a comment.Feb 25 2021, 9:24 AM

Why isn't the solution is to fix the uses to prevent variable substitution in them?
Because of course $1 is substituted with a value of variable 1 in shell, and elsewhere.

$ c++filt _Z3foov\$123
_Z3foov$123

Sorry, I should have used quotes. But your example shows that the name is still not getting demangled. Compare that with a period as separator:

$ c++filt _Z3foov.123
foo() [clone .123]

Now it demangles, and puts the suffix on the side.

Thanks for fixing this. Question below

llvm/lib/IR/Module.cpp
677 ↗(On Diff #326420)

Should getUniqueModuleId be changed to not put in the "$"? Looking at the other use sites, it is used for renaming comdats and types - can/should these get the same treatment via this function (use .llvm.moduleId instead of $moduleId)?

pcc added inline comments.Feb 25 2021, 10:47 AM
llvm/lib/IR/Module.cpp
677 ↗(On Diff #326420)

I agree, that function should be changed to use . instead of $.

Why isn't the solution is to fix the uses to prevent variable substitution in them?
Because of course $1 is substituted with a value of variable 1 in shell, and elsewhere.

$ c++filt _Z3foov\$123
_Z3foov$123

Sorry, I should have used quotes. But your example shows that the name is still not getting demangled. Compare that with a period as separator:

$ c++filt _Z3foov.123
foo() [clone .123]

Now it demangles, and puts the suffix on the side.

Thank you, please make that more obvious in the patch's description :)

hans edited the summary of this revision. (Show Details)Mar 16 2021, 5:48 AM
hans updated this revision to Diff 330963.Mar 16 2021, 6:25 AM

Sorry for the delay, I got distracted by other fires.

I've updated the patch to just use a period as separator in getUniqueModuleId() and tweak any other place I found that used $ as separator on their own.

Please take another look.

tejohnson accepted this revision.Thu, Mar 25, 11:01 AM

lgtm, thanks!

This revision is now accepted and ready to land.Thu, Mar 25, 11:01 AM
This revision was landed with ongoing or failed builds.Mon, Mar 29, 4:17 AM
This revision was automatically updated to reflect the committed changes.