Page MenuHomePhabricator

[demangler] Fix undocumented Local encoding
ClosedPublic

Authored by urnathan on Apr 5 2022, 9:35 AM.

Details

Reviewers
iains
Group Reviewers
Restricted Project
Commits
rGdf4522feb790: [demangler] Fix undocumented Local encoding
Summary

GCC emits [some] static symbols with an 'L' mangling, which we attempt to demangle. But the module mangling changes have exposed that we were doing so at the wrong level. AFAICT this isn't documented in the ABI. This adjusts the demangler along the same lines as the existing gcc demangler (which is not yet module-aware). 'L' is part of an unqualified name. As before we merely parse the 'L', and then ignore it.

@iains discovered this problem when playing with gcc's module implementation.

[I have GCC demangler patches, but GCC is currently stabilizing for release.]

Diff Detail

Event Timeline

urnathan created this revision.Apr 5 2022, 9:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 9:35 AM
urnathan requested review of this revision.Apr 5 2022, 9:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 9:35 AM
iains added a comment.Apr 6 2022, 2:54 AM

just to clarify the intent for:

export module A;

static int addone (int y) { return y + 1; }
static int x = 5;

export int addsix (int z) { return x + addone (z); }

GCC mangles addone => _ZW1AL6addonei
and x as _ZW1AL1x

clang mangles them as _ZL6addonei and _ZL1x respectively.

Whilst this makes no practical difference (since the symbols are TU-local in either case) - I was expecting that decls in the purview of A would be attached to it (and thus mangled as such). I guess I missed some subtlety.

just to clarify the intent for:

export module A;

static int addone (int y) { return y + 1; }
static int x = 5;

export int addsix (int z) { return x + addone (z); }

GCC mangles addone => _ZW1AL6addonei
and x as _ZW1AL1x

clang mangles them as _ZL6addonei and _ZL1x respectively.

Whilst this makes no practical difference (since the symbols are TU-local in either case) - I was expecting that decls in the purview of A would be attached to it (and thus mangled as such). I guess I missed some subtlety.

The ABI just specifies the mangling for cross-TU-visible symbols. Different compilers may choose different manglings for internal-linkage symbols.

That said, it seems I made different choices in GCC and Clang manglers, which isn't the best ($urnathan[GCC] != $urnathan[Clang]). I guess if the elf-linkage on a template instantiation over some internal-linkage entity gets the wrong linkage, the GCC approach is better?

iains added a comment.Apr 6 2022, 4:58 AM

just to clarify the intent for:

export module A;

static int addone (int y) { return y + 1; }
static int x = 5;

export int addsix (int z) { return x + addone (z); }

GCC mangles addone => _ZW1AL6addonei
and x as _ZW1AL1x

clang mangles them as _ZL6addonei and _ZL1x respectively.

Whilst this makes no practical difference (since the symbols are TU-local in either case) - I was expecting that decls in the purview of A would be attached to it (and thus mangled as such). I guess I missed some subtlety.

The ABI just specifies the mangling for cross-TU-visible symbols. Different compilers may choose different manglings for internal-linkage symbols.

That said, it seems I made different choices in GCC and Clang manglers, which isn't the best ($urnathan[GCC] != $urnathan[Clang]). I guess if the elf-linkage on a template instantiation over some internal-linkage entity gets the wrong linkage, the GCC approach is better?

That would be my thinking too (also a little more helpful to poor souls who sometimes need to look at .ll and .s files).

urnathan added a comment.EditedApr 6 2022, 7:33 AM

thinking further, adding module attachment makes the symbols longer (probably not much, but still). I also discovered in GCC it was intentionally mangling module attachment to anonymous namespaces. I will change GCC to match Clang's behaviour here -- post GCC12. Meanwhile there's D123220 to verify the attachment is not being added.

ETA: I tested Clang to add this mangling, and it generated the same as GCC, so that's more evidence that this demangler change is correct.

iains accepted this revision.Apr 6 2022, 9:22 AM

this LGTM, at least it demangles the symbols from GCC (presumably those will still be emitted for GCC11 and 12 even if the change is made to 13+ to match clang)

... hopefully the CI test fails are spurious?

This revision is now accepted and ready to land.Apr 6 2022, 9:22 AM
This revision was landed with ongoing or failed builds.Apr 6 2022, 10:13 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 10:13 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript