Page MenuHomePhabricator

Disable Unique Internal Linkage Names for internal global vars
ClosedPublic

Authored by tmsriram on Mar 10 2021, 9:34 PM.

Details

Summary

Demanglers do not recognize clone suffixes for global variables, so disable unique linkage suffixes until demanglers can be fixed.

D96109 added support for unique internal linkage names for both internal linkage functions and global variables. There was a lot of discussion on how to get the demangling right for functions but I completely missed the point that demanglers do not support suffixes for global vars. For example:

$ c++filt _ZL3foo
foo
$ c++filt _ZL3foo.uniq.123
_ZL3foo.
uniq.123

The demangling for functions works as expected.

I am not sure of the impact of this. I don't understand how debuggers and other tools depend on the correctness of global variable demangling so I am pre-emptively disabling it until we can get the demangling support added.

Importantly, uniquefying global variables is not needed right now as we do not do profile attribution to global vars based on sampling. It was added for completeness and so this feature is not exactly missed.

Sorry about missing this in the previous change, it was a blind spot.

Diff Detail

Event Timeline

tmsriram requested review of this revision.Mar 10 2021, 9:34 PM
tmsriram created this revision.
hoy accepted this revision.Mar 11 2021, 1:13 PM

Thanks for the fix. This should have no impact on AutoFDO.

This revision is now accepted and ready to land.Mar 11 2021, 1:13 PM
dblaikie added inline comments.Mar 11 2021, 2:17 PM
clang/lib/AST/ItaniumMangle.cpp
642–649

/might/ be marginally better using an early exit

clang/lib/CodeGen/CodeGenModule.cpp
1182–1184

Perhaps this would be simpler to return the expression.

Or could be split out into a series of early exit:

if (empty module hash)
  return false;
if (!isa function)
  return false;
if (!= internal linkage)
  return false;
return true;
tmsriram updated this revision to Diff 330130.Mar 11 2021, 7:31 PM
tmsriram marked 2 inline comments as done.

Address reviewer comments and refactor code as mentioned.

dblaikie accepted this revision.Mar 11 2021, 7:52 PM

Looks good

This revision was landed with ongoing or failed builds.Mar 11 2021, 8:59 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 9:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript