[libcxxabi][ItaniumDemangle] Demangle explicitly named object parameters #72881

Michael137 · 2023-11-20T14:58:15Z

The mangling for an explicitly named object was introduced in https://reviews.llvm.org/D140828

See following discussion for why a new mangling had to be introduced: itanium-cxx-abi/cxx-abi#148

Since clang started emitting names with the new mangling, this patch implements support for demangling such names.

The approach this patch takes is to add a new ExplicitObjectParameter node that will print the first parameter of a function declaration with a this prefix, to reflect what was spelled out in source.

Example:

void MyClass::func(this MyClass const& self); // _ZNH7MyClass4funcERKS_

With this patch, the above demangles to:

_ZNH7MyClass4funcERKS_ -> MyClass::func(this MyClass const&)

Note that func is not marked as const &, since the function-qualifiers are now encoded as part of the explicit this. C++ doesn't allow specifying the function-qualifiers in the presence of an explicit object parameter, so this demangling is consistent with the source spelling.

llvmbot · 2023-11-20T14:58:48Z

@llvm/pr-subscribers-libcxxabi

Author: Michael Buch (Michael137)

Changes

The mangling for an explicitly named object was introduced in https://reviews.llvm.org/D140828

See following discussion for why a new mangling had to be introduced: itanium-cxx-abi/cxx-abi#148

Since clang started emitting names with the new mangling, this patch implements support for demangling such names.

The approach this patch takes is to simply skip the explicit this parameter when constructing a FunctionEncoding. Thus following two functions demangle to the same:

struct Foo {
  void func(this Foo self);
  void func();
};

Full diff: https://github.com/llvm/llvm-project/pull/72881.diff

2 Files Affected:

(modified) libcxxabi/src/demangle/ItaniumDemangle.h (+22-8)
(modified) libcxxabi/test/test_demangle.pass.cpp (+10)

diff --git a/libcxxabi/src/demangle/ItaniumDemangle.h b/libcxxabi/src/demangle/ItaniumDemangle.h
index 2336b84da293bcc..f9a0c059e86e8bd 100644
--- a/libcxxabi/src/demangle/ItaniumDemangle.h
+++ b/libcxxabi/src/demangle/ItaniumDemangle.h
@@ -2780,6 +2780,7 @@ template <typename Derived, typename Alloc> struct AbstractManglingParser {
     Qualifiers CVQualifiers = QualNone;
     FunctionRefQual ReferenceQualifier = FrefQualNone;
     size_t ForwardTemplateRefsBegin;
+    bool HasExplicitObjectParameter = false;
 
     NameState(AbstractManglingParser *Enclosing)
         : ForwardTemplateRefsBegin(Enclosing->ForwardTemplateRefs.size()) {}
@@ -3438,15 +3439,25 @@ AbstractManglingParser<Derived, Alloc>::parseNestedName(NameState *State) {
   if (!consumeIf('N'))
     return nullptr;
 
-  Qualifiers CVTmp = parseCVQualifiers();
-  if (State) State->CVQualifiers = CVTmp;
+  // 'H' specifies that the encoding that follows
+  // has an explicit object parameter.
+  if (!consumeIf('H')) {
+    Qualifiers CVTmp = parseCVQualifiers();
+    if (State)
+      State->CVQualifiers = CVTmp;
 
-  if (consumeIf('O')) {
-    if (State) State->ReferenceQualifier = FrefQualRValue;
-  } else if (consumeIf('R')) {
-    if (State) State->ReferenceQualifier = FrefQualLValue;
-  } else {
-    if (State) State->ReferenceQualifier = FrefQualNone;
+    if (consumeIf('O')) {
+      if (State)
+        State->ReferenceQualifier = FrefQualRValue;
+    } else if (consumeIf('R')) {
+      if (State)
+        State->ReferenceQualifier = FrefQualLValue;
+    } else {
+      if (State)
+        State->ReferenceQualifier = FrefQualNone;
+    }
+  } else if (State) {
+    State->HasExplicitObjectParameter = true;
   }
 
   Node *SoFar = nullptr;
@@ -5424,6 +5435,9 @@ Node *AbstractManglingParser<Derived, Alloc>::parseEncoding() {
         return nullptr;
       Names.push_back(Ty);
     } while (!IsEndOfEncoding() && look() != 'Q');
+    // Ignore the explicit 'this' parameter.
+    if (NameInfo.HasExplicitObjectParameter)
+      ++ParamsBegin;
     Params = popTrailingNodeArray(ParamsBegin);
   }
 
diff --git a/libcxxabi/test/test_demangle.pass.cpp b/libcxxabi/test/test_demangle.pass.cpp
index 77741a952850ab9..a6b6d45a3783bb1 100644
--- a/libcxxabi/test/test_demangle.pass.cpp
+++ b/libcxxabi/test/test_demangle.pass.cpp
@@ -30174,6 +30174,16 @@ const char* cases[][2] =
     {"_Z2f5IKiEvu14__remove_constIT_E", "void f5<int const>(__remove_const(int const))"},
     {"_Z2f5IPiEvu16__remove_pointerIT_E", "void f5<int*>(__remove_pointer(int*))"},
     {"_Z2f5IiEvu14__remove_cvrefIT_E", "void f5<int>(__remove_cvref(int))"},
+
+    // C++23 explicit object parameter
+    {"_ZNH3Foo3fooES_i", "Foo::foo(int)"},
+    {"_ZNH3Foo3fooERKS_i", "Foo::foo(int)"},
+    {"_ZNH1X3fooIRS_EEvOT_i", "void X::foo<X&>(int)"},
+    {"_ZZNH2ns3Foo3fooES0_iENH4Foo24foo2EOKS1_", "ns::Foo::foo(int)::Foo2::foo2()" },
+    {"_ZNH2ns3FooB7Foo_tag3fooB7foo_tagERKS0_i", "ns::Foo[abi:Foo_tag]::foo[abi:foo_tag](int)" },
+    {"_ZZN3Foo3fooEiENH4Foo24foo2EOKS0_", "Foo::foo(int)::Foo2::foo2()"},
+    {"_ZZNH3Foo3fooES_iENK4Foo24foo2Ev", "Foo::foo(int)::Foo2::foo2() const" },
+    {"_ZNH3FooclERKS_", "Foo::operator()()"},
 };
 
 const unsigned N = sizeof(cases) / sizeof(cases[0]);

Michael137 · 2023-11-20T14:59:35Z

Once people are happy with the approach I'll sync the changes to llvm in a separate patch

Michael137 · 2023-11-20T15:24:00Z

clang-format fails for the tests, which I don't think we want to actually format

ldionne

My review shouldn't be considered authoritative cause I don't know the demangler very well, but this seems reasonable to me.

epilk · 2023-11-22T22:38:08Z

libcxxabi/src/demangle/ItaniumDemangle.h

@@ -5424,6 +5435,9 @@ Node *AbstractManglingParser<Derived, Alloc>::parseEncoding() {
        return nullptr;
      Names.push_back(Ty);
    } while (!IsEndOfEncoding() && look() != 'Q');
+    // Ignore the explicit 'this' parameter.
+    if (NameInfo.HasExplicitObjectParameter)


Any reason to avoid printing out the this parameter? I think it could be useful to include it, especially if a user is overloading using it.

No particular reason apart from implementation simplicity. Thinking about it some more though, we would be losing the ref-qualifiers on the functions here because in the explicit-object-paramter case that information is encoded into the parameter itself, which we discard.

Shouldn't be too difficult to amend this patch to explicitly name the parameter. Probably with a this prefix.

Had a stab at this in the latest commit

epilk

Can you run the cp-to-llvm.sh script before merging this too? Thanks!

epilk · 2023-11-24T18:56:57Z

libcxxabi/src/demangle/ItaniumDemangle.h

@@ -5422,6 +5456,14 @@ Node *AbstractManglingParser<Derived, Alloc>::parseEncoding() {
      Node *Ty = getDerived().parseType();
      if (Ty == nullptr)
        return nullptr;
+
+      const bool IsFirstParam = (Names.size() - ParamsBegin) == 0;


nit: ParamsBegin == Names.size() seems a bit more direct.

epilk · 2023-11-24T18:59:33Z

libcxxabi/src/demangle/ItaniumDemangle.h

+
+  template <typename Fn> void match(Fn F) const { F(Base); }
+
+  void printLeft(OutputBuffer &OB) const override { OB += "this "; }


I think you don't need to overload printRight, you should just be able to do: OB += "this"; Base->print(OB); here in printLeft (in which case you can remove the Cache::Yes just above).

epilk · 2023-11-24T19:03:24Z

libcxxabi/src/demangle/ItaniumDemangle.h

+public:
+  ExplicitObjectParameter(Node *Base_)
+      : Node(KExplicitObjectParameter, Cache::Yes), Base(Base_) {
+    assert(Base != nullptr);


The bots are complaining about assert here, looks like we're meant to use DEMANGLE_ASSERT now.

Yea I'm a little confused about this one, since we're using plain asserts throughout the file. Any idea why specifically this one is failing?

Maybe the bots are rebasing your changes on top of c4779ea ?

ah good catch!

Michael137 · 2023-11-25T09:28:27Z

The buildkite job for this PR is failing on Windows/Linux with:

.../llvm-project/github-pull-requests/llvm/include/llvm/Demangle/ItaniumDemangle.h:905:5: error: use of undeclared identifier 'DEMANGLE_ASSERT'
    DEMANGLE_ASSERT(
    ^

Pretty sure it isn't a problem with this patch. Is there some misconfiguration going on? (@ldionne)

Michael137 · 2023-11-28T07:51:53Z

This builds fine for me on my Linux machine. @epilk any idea what the buildkite bot could be doing differently?

epilk · 2023-11-28T14:24:47Z

This builds fine for me on my Linux machine. @epilk any idea what the buildkite bot could be doing differently?

Looks like an unrelated failure on windows. Maybe try rebasing to see if it's still failing, and if so ping @ldionne?

Michael137 · 2023-11-28T15:46:42Z

This builds fine for me on my Linux machine. @epilk any idea what the buildkite bot could be doing differently?

Looks like an unrelated failure on windows. Maybe try rebasing to see if it's still failing, and if so ping @ldionne?

Ah seems like after rebase the build failure was fixed. Going to merge since the test failures look unrelated

Michael137 requested a review from ldionne November 20, 2023 14:58

Michael137 requested a review from a team as a code owner November 20, 2023 14:58

llvmbot added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Nov 20, 2023

Michael137 requested a review from urnathan November 20, 2023 14:59

ldionne approved these changes Nov 20, 2023

View reviewed changes

epilk reviewed Nov 22, 2023

View reviewed changes

epilk approved these changes Nov 24, 2023

View reviewed changes

Michael137 force-pushed the bugfix/demangle-explicit-object-parameter branch from 9197ebd to b2c4195 Compare November 27, 2023 11:26

Michael137 added 9 commits November 28, 2023 15:53

[libcxxabi][ItaniumDemangle] Demangle explicitly named object parameters

e0c8b4d

fixup! print the explicit parameter

513e470

fixup! assert nullness in ExplicitObjectParamter constructor

81bb5da

fixup! remove redundant printRight; simplify IsFirstParam check

e6daf0b

fixup! sync libcxxabi into llvm

997207b

fixup! remove printRight

831665e

fixup! re-sync libcxxabi to llvm

7fca693

fixup! assert -> DEMANGLE_ASSERT

750f65c

fixup! remove breaking header

033fd85

Michael137 force-pushed the bugfix/demangle-explicit-object-parameter branch from b2c4195 to 033fd85 Compare November 28, 2023 15:54

Michael137 merged commit 4d5079c into llvm:main Nov 28, 2023
35 of 38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libcxxabi][ItaniumDemangle] Demangle explicitly named object parameters #72881

[libcxxabi][ItaniumDemangle] Demangle explicitly named object parameters #72881

Michael137 commented Nov 20, 2023 •

edited

llvmbot commented Nov 20, 2023

Michael137 commented Nov 20, 2023

Michael137 commented Nov 20, 2023

ldionne left a comment

epilk Nov 22, 2023

Michael137 Nov 23, 2023 •

edited

Michael137 Nov 24, 2023

epilk left a comment

epilk Nov 24, 2023

epilk Nov 24, 2023

epilk Nov 24, 2023

Michael137 Nov 24, 2023

epilk Nov 25, 2023

Michael137 Nov 25, 2023

Michael137 commented Nov 25, 2023

Michael137 commented Nov 28, 2023

epilk commented Nov 28, 2023

Michael137 commented Nov 28, 2023


		template <typename Fn> void match(Fn F) const { F(Base); }

		void printLeft(OutputBuffer &OB) const override { OB += "this "; }

[libcxxabi][ItaniumDemangle] Demangle explicitly named object parameters #72881

[libcxxabi][ItaniumDemangle] Demangle explicitly named object parameters #72881

Conversation

Michael137 commented Nov 20, 2023 • edited

llvmbot commented Nov 20, 2023

Michael137 commented Nov 20, 2023

Michael137 commented Nov 20, 2023

ldionne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael137 Nov 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael137 commented Nov 25, 2023

Michael137 commented Nov 28, 2023

epilk commented Nov 28, 2023

Michael137 commented Nov 28, 2023

Michael137 commented Nov 20, 2023 •

edited

Michael137 Nov 23, 2023 •

edited