This is an archive of the discontinued LLVM Phabricator instance.

[demangler] Add support for C++20 modules
ClosedPublic

Authored by urnathan on Feb 16 2022, 4:46 AM.

Details

Summary

Now all the prerequisite demangler patches are in, here's the patch for module demangling. As with the mangler patch (D118352) this is the updated scheme described at: https://drive.google.com/file/d/1quPYyByXqOpCyAPlxYt1JleA70TSFMrp/view?usp=sharing

We have two new demangler nodes -- ModuleName and ModuleEntity. The former represents a module name in a hierarchical fashion. The latter is the combination of a (name) node and a module name. Because module names and entity identities use the same substitution encoding, we have to adjust the flow of how substitutions are handled, and examine the substituted node to know how to deal with it.

There is one difference to that doc, the global initializer _ZGI <module-name> lacks the 'v' suffix. I don't think that necessary -- my original thought was of describing something more like a regular void function mangling. I'll be updating the google doc with this alteration and some other clarifications that don't affect the demangler.

Diff Detail

Event Timeline

urnathan created this revision.Feb 16 2022, 4:46 AM
urnathan requested review of this revision.Feb 16 2022, 4:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2022, 4:46 AM

Let's add D118352 as a related revision?

Let's add D118352 as a related revision?

It's related. but it is neither a child nor parent. And we've now noted it here :)

urnathan updated this revision to Diff 409928.Feb 18 2022, 7:12 AM
urnathan edited the summary of this revision. (Show Details)

rebase, & fix formats

urnathan updated this revision to Diff 412711.Mar 3 2022, 7:14 AM

fix CI formatting

Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 7:14 AM
urnathan updated this revision to Diff 412713.Mar 3 2022, 7:23 AM
urnathan updated this revision to Diff 412717.Mar 3 2022, 7:31 AM
urnathan updated this revision to Diff 412739.Mar 3 2022, 8:46 AM

rebase again

urnathan updated this revision to Diff 412792.Mar 3 2022, 11:43 AM

remove trailing whitespace

urnathan updated this revision to Diff 414345.Mar 10 2022, 4:45 AM
urnathan removed a reviewer: ChuanqiXu.

rebased, the clang-format CI fail is due to testcase formatting that I have not touched.

ping? AFAICT the CI build failures are not consistent and not caused by this patch

urnathan edited reviewers, added: aaron.ballman, dblaikie; removed: rsmith.Mar 17 2022, 11:33 AM
dblaikie accepted this revision.Mar 21 2022, 3:13 PM

I don't have much context for the mangler, but this generally looks OK to me. (one minor question, probably just a comment update)

llvm/include/llvm/Demangle/ItaniumDemangle.h
2829–2835

Is the comment about S out of date, and should say W instead?

This revision is now accepted and ready to land.Mar 21 2022, 3:13 PM
urnathan marked an inline comment as done.Mar 22 2022, 7:44 AM
urnathan added inline comments.
llvm/include/llvm/Demangle/ItaniumDemangle.h
2829–2835

oops, I'd think'od that, it is indeed 'W'

This revision was landed with ongoing or failed builds.Mar 22 2022, 9:43 AM
This revision was automatically updated to reflect the committed changes.
urnathan marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 9:43 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Seems that abi::__cxa_demangle("W2.c", nullptr, nullptr, &ignored); crashes on libcxxabi/src/demangle/ItaniumDemangle.h:355 printLeft(OB); (virtual function call).

At the call site libcxxabi/src/cxa_demangle.cpp:358, Node *AST is parsed as a type which does not seem to have the left child.

MaskRay added inline comments.Mar 30 2022, 10:14 AM
libcxxabi/src/demangle/ItaniumDemangle.h
1033

Name can be nullptr here when the input is "W2.c" (would be parsed as a type). See my main comment.

Made a fix here: 6f5ecd089f77e0da9af44bb7211ad1b11ea582d4 - could have a _Z* test case too as I think it'd come up there too.