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

[Michael137]

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

[Michael137]

• Update commit message

[jhenderson]

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.

[Michael137]

In D145492#4177222, @jhenderson wrote:

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?

[ldionne]

Thanks for the catch, this makes sense to me!

In D145492#4177344, @Michael137 wrote:

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]

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

7

Good idea, will do

[Michael137]

• Sync changes to llvm
• Add test case to libcxx test-suite

[jhenderson]

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.

[Michael137]

llvm/include/llvm/Demangle/ItaniumDemangle.h

35–41 ↗

(On Diff #503567)

Makes sense, done

libcxxabi/src/demangle/ItaniumDemangle.h

struct AbiTagAttr : Node {
 
   AbiTagAttr(Node* Base_, StringView Tag_)
       : Node(KAbiTagAttr, Base_->RHSComponentCache,
              Base_->ArrayCache, Base_->FunctionCache),
         Base(Base_), Tag(Tag_) {}
 
   template<typename Fn> void match(Fn F) const { F(Base, Tag); }
 
+  StringView getBaseName() const override { return Base->getBaseName(); }
+
   void printLeft(OutputBuffer &OB) const override {
     Base->printLeft(OB);
     OB += "[abi:";
     OB += Tag;
     OB += "]";
   }
 };
 

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

-RUN: llvm-cxxfilt -n _Z14returns_stringB5cxx11v _Z6globalB5cxx11 _Z6globalB12a_longer_tag | FileCheck %s
+RUN: llvm-cxxfilt -n _Z14returns_stringB5cxx11v _Z6globalB5cxx11 _Z6globalB12a_longer_tag _ZN6globalB3TagC2Ev _ZN6globalB3TagD2Ev | FileCheck %s
 
 CHECK: returns_string[abi:cxx11]()
 CHECK-NEXT: global[abi:cxx11]
 CHECK-NEXT: global[abi:a_longer_tag]
-
+CHECK-NEXT: global[abi:Tag]::global()
+CHECK-NEXT: global[abi:Tag]::~global()