This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi][Demangle] Don't drop ctor/dtor name for abi-tagged structures
ClosedPublic

Authored by Michael137 on Mar 7 2023, 5:28 AM.

Details

Summary

Before this patch we would demangle abi-tagged structures as follows:

$ c++filt -n _ZN1SB5OuterC2Ev
S[abi:Outer]:()

$ c++filt -n _ZN1SB5OuterD2Ev
S[abi:Outer]::~()

This is because Node::getBaseName was unimplemented for the
AbiTagAttr node, which meant that when we tried printing CtorDtorName
where its Basename Node was an AbiTagAttr, we'd drop the
name.

Addresses https://github.com/llvm/llvm-project/issues/61213

Diff Detail

Event Timeline

Michael137 created this revision.Mar 7 2023, 5:28 AM
Herald added a project: Restricted Project. · View Herald Transcript
Michael137 requested review of this revision.Mar 7 2023, 5:28 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 7 2023, 5:28 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Michael137 updated this revision to Diff 503003.Mar 7 2023, 5:30 AM
  • Update commit message
compnerd accepted this revision.Mar 7 2023, 7:22 AM
This revision is now accepted and ready to land.Mar 7 2023, 7:22 AM
jhenderson requested changes to this revision.Mar 8 2023, 1:00 AM

Your test is failing the pre-merge checks.

llvm/test/tools/llvm-cxxfilt/abitag.test
1

This test is testing llvm-cxxfilt, but llvm-cxxfilt doesn't use the libcxxabi code. It uses the demangler code in libDemangle, so it's not surprising it fails.

This revision now requires changes to proceed.Mar 8 2023, 1:00 AM

Your test is failing the pre-merge checks.

Yup, I split the llvm changes to here: https://reviews.llvm.org/D145493

Want me to merge the two commits into a single revision/commit? Or move the test to the LLVM diff?

Thanks for the catch, this makes sense to me!

Want me to merge the two commits into a single revision/commit? Or move the test to the LLVM diff?

FWIW, no strong opinion here but I think it would make sense to apply the changes to both directories in the same patch. You may want to sync it once before your patch if it's out-of-date right now as a NFC, just so that this patch stays consistent.

llvm/test/tools/llvm-cxxfilt/abitag.test
7

Would it make sense to add a test to libcxxabi/test/test_demangle.pass.cpp as well? I think that's where those are usually added.

Michael137 added inline comments.Mar 8 2023, 4:18 PM
llvm/test/tools/llvm-cxxfilt/abitag.test
7

Good idea, will do

Michael137 updated this revision to Diff 503562.Mar 8 2023, 4:31 PM
  • Sync changes to llvm
  • Add test case to libcxx test-suite
Michael137 updated this revision to Diff 503567.Mar 8 2023, 4:46 PM
jhenderson accepted this revision.Mar 9 2023, 12:45 AM

Approving to clear my objection, but probably this could do with a pair of eyes more familiar with ABI tagging (or at least the mangling parts in relation to it).

llvm/include/llvm/Demangle/ItaniumDemangle.h
35–41 ↗(On Diff #503567)

Are these changes unrelated and simply because the two haven't been synced properly? If so, I'd do an NFC patch before this one to sync up the changes first.

This revision is now accepted and ready to land.Mar 9 2023, 12:45 AM
Michael137 marked an inline comment as done.
Michael137 added inline comments.
llvm/include/llvm/Demangle/ItaniumDemangle.h
35–41 ↗(On Diff #503567)

Makes sense, done

Michael137 updated this revision to Diff 503777.Mar 9 2023, 8:13 AM
Michael137 marked an inline comment as done.
This revision was landed with ongoing or failed builds.Mar 19 2023, 4:07 AM
This revision was automatically updated to reflect the committed changes.