Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[ItaniumDemangle] Add Named flag to "pm" operator #136862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
May 12, 2025
Merged

[ItaniumDemangle] Add Named flag to "pm" operator #136862

merged 4 commits into from
May 12, 2025
+7 −3

Conversation

Michael137
Copy link
Member

Compilers can generate mangled names such as _ZN1CpmEi which we currently fail to demangle. The OperatorInfo table only marked the pt operator as being "named", which prevented the others from demangling properly. Removing this logic for the other kinds of member operators isn't causing any tests to fail.

@Michael137 Michael137 requested a review from urnathan April 23, 2025 13:24
@Michael137 Michael137 requested a review from a team as a code owner April 23, 2025 13:24
@llvmbot llvmbot added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Apr 23, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 23, 2025

@llvm/pr-subscribers-libcxxabi

Author: Michael Buch (Michael137)

Changes

Compilers can generate mangled names such as _ZN1CpmEi which we currently fail to demangle. The OperatorInfo table only marked the pt operator as being "named", which prevented the others from demangling properly. Removing this logic for the other kinds of member operators isn't causing any tests to fail.

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

3 Files Affected:

  • (modified) libcxxabi/src/demangle/ItaniumDemangle.h (+4-7)
  • (modified) libcxxabi/test/test_demangle.pass.cpp (+4)
  • (modified) llvm/include/llvm/Demangle/ItaniumDemangle.h (+4-7)
diff --git a/libcxxabi/src/demangle/ItaniumDemangle.h b/libcxxabi/src/demangle/ItaniumDemangle.h
index 69932e63669bf..67cc4560f061d 100644
--- a/libcxxabi/src/demangle/ItaniumDemangle.h
+++ b/libcxxabi/src/demangle/ItaniumDemangle.h
@@ -3389,9 +3389,9 @@ const typename AbstractManglingParser<
     {"de", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator*"},
     {"dl", OperatorInfo::Del, /*Ary*/ false, Node::Prec::Unary,
      "operator delete"},
-    {"ds", OperatorInfo::Member, /*Named*/ false, Node::Prec::PtrMem,
+    {"ds", OperatorInfo::Member, false, Node::Prec::PtrMem,
      "operator.*"},
-    {"dt", OperatorInfo::Member, /*Named*/ false, Node::Prec::Postfix,
+    {"dt", OperatorInfo::Member, false, Node::Prec::Postfix,
      "operator."},
     {"dv", OperatorInfo::Binary, false, Node::Prec::Assign, "operator/"},
     {"eO", OperatorInfo::Binary, false, Node::Prec::Assign, "operator^="},
@@ -3421,11 +3421,11 @@ const typename AbstractManglingParser<
     {"or", OperatorInfo::Binary, false, Node::Prec::Ior, "operator|"},
     {"pL", OperatorInfo::Binary, false, Node::Prec::Assign, "operator+="},
     {"pl", OperatorInfo::Binary, false, Node::Prec::Additive, "operator+"},
-    {"pm", OperatorInfo::Member, /*Named*/ false, Node::Prec::PtrMem,
+    {"pm", OperatorInfo::Member, false, Node::Prec::PtrMem,
      "operator->*"},
     {"pp", OperatorInfo::Postfix, false, Node::Prec::Postfix, "operator++"},
     {"ps", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator+"},
-    {"pt", OperatorInfo::Member, /*Named*/ true, Node::Prec::Postfix,
+    {"pt", OperatorInfo::Member, false, Node::Prec::Postfix,
      "operator->"},
     {"qu", OperatorInfo::Conditional, false, Node::Prec::Conditional,
      "operator?"},
@@ -3499,9 +3499,6 @@ AbstractManglingParser<Derived, Alloc>::parseOperatorName(NameState *State) {
     if (Op->getKind() >= OperatorInfo::Unnameable)
       /* Not a nameable operator.  */
       return nullptr;
-    if (Op->getKind() == OperatorInfo::Member && !Op->getFlag())
-      /* Not a nameable MemberExpr */
-      return nullptr;
 
     return make<NameType>(Op->getName());
   }
diff --git a/libcxxabi/test/test_demangle.pass.cpp b/libcxxabi/test/test_demangle.pass.cpp
index 53da1bf6765e7..22b39fb06458d 100644
--- a/libcxxabi/test/test_demangle.pass.cpp
+++ b/libcxxabi/test/test_demangle.pass.cpp
@@ -30246,6 +30246,10 @@ const char* cases[][2] = {
 
     {"_Z3fooPU9__ptrauthILj3ELb1ELj234EEPi", "foo(int* __ptrauth<3u, true, 234u>*)"},
     {"_Z3fooIPU9__ptrauthILj1ELb0ELj64EEPiEvT_", "void foo<int* __ptrauth<1u, false, 64u>*>(int* __ptrauth<1u, false, 64u>*)"},
+
+    {"_ZN1CpmEi", "C::operator->*(int)"},
+    {"_ZN1CdtEi", "C::operator.(int)"},
+    {"_ZN1CdsEi", "C::operator.*(int)"},
     // clang-format on
 };
 
diff --git a/llvm/include/llvm/Demangle/ItaniumDemangle.h b/llvm/include/llvm/Demangle/ItaniumDemangle.h
index b4a4c72021fd1..f75e309b06e0a 100644
--- a/llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ b/llvm/include/llvm/Demangle/ItaniumDemangle.h
@@ -3389,9 +3389,9 @@ const typename AbstractManglingParser<
     {"de", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator*"},
     {"dl", OperatorInfo::Del, /*Ary*/ false, Node::Prec::Unary,
      "operator delete"},
-    {"ds", OperatorInfo::Member, /*Named*/ false, Node::Prec::PtrMem,
+    {"ds", OperatorInfo::Member, false, Node::Prec::PtrMem,
      "operator.*"},
-    {"dt", OperatorInfo::Member, /*Named*/ false, Node::Prec::Postfix,
+    {"dt", OperatorInfo::Member, false, Node::Prec::Postfix,
      "operator."},
     {"dv", OperatorInfo::Binary, false, Node::Prec::Assign, "operator/"},
     {"eO", OperatorInfo::Binary, false, Node::Prec::Assign, "operator^="},
@@ -3421,11 +3421,11 @@ const typename AbstractManglingParser<
     {"or", OperatorInfo::Binary, false, Node::Prec::Ior, "operator|"},
     {"pL", OperatorInfo::Binary, false, Node::Prec::Assign, "operator+="},
     {"pl", OperatorInfo::Binary, false, Node::Prec::Additive, "operator+"},
-    {"pm", OperatorInfo::Member, /*Named*/ false, Node::Prec::PtrMem,
+    {"pm", OperatorInfo::Member, false, Node::Prec::PtrMem,
      "operator->*"},
     {"pp", OperatorInfo::Postfix, false, Node::Prec::Postfix, "operator++"},
     {"ps", OperatorInfo::Prefix, false, Node::Prec::Unary, "operator+"},
-    {"pt", OperatorInfo::Member, /*Named*/ true, Node::Prec::Postfix,
+    {"pt", OperatorInfo::Member, false, Node::Prec::Postfix,
      "operator->"},
     {"qu", OperatorInfo::Conditional, false, Node::Prec::Conditional,
      "operator?"},
@@ -3499,9 +3499,6 @@ AbstractManglingParser<Derived, Alloc>::parseOperatorName(NameState *State) {
     if (Op->getKind() >= OperatorInfo::Unnameable)
       /* Not a nameable operator.  */
       return nullptr;
-    if (Op->getKind() == OperatorInfo::Member && !Op->getFlag())
-      /* Not a nameable MemberExpr */
-      return nullptr;
 
     return make<NameType>(Op->getName());
   }

@Michael137
Copy link
Member Author

@urnathan I couldn't really find an explanation for what "Not a nameable MemberExpr" are. Any idea if this is still needed? Test-suite runs cleanly without this

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 23, 2025 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

urnathan
urnathan reviewed Apr 23, 2025
View reviewed changes
Copy link
Contributor

@urnathan urnathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On vacation. But iirc this logic predates my rewrite. Ill try and remember to look more later

@Michael137
Copy link
Member Author

On vacation. But iirc this logic predates my rewrite. Ill try and remember to look more later

Thanks!

Seems to have been introduced by:

commit 7f89fa32e8e9251b0b780009c340bc0d8f4d2e88                                            
Author: Nathan Sidwell <nathan@acm.org>                                                    
Date:   Thu Jan 27 13:23:16 2022 -0800                                                     
                                                                                           
    [demangler][NFC] Tabularize operator name parsing                                      
                                                                                           
    We need to parse operator names in 3 places -- expressions, names &                    
    fold expressions.  Currently we have 3 separate pieces to do this, and a FIXME.        
                                                                                           
    The operator name and expression parsing are implemented as                            
    handwritten two-character nested switches, the fold expression is a                    
    sequence of string comparisons.                                                        
                                                                                           
    This adds a new OperatorInfo class to encode the operator info                         
    (encoding, kind, name), and has a table that it can binary search.                     
    From that each of the above 3 uses are altered to use the new scheme.                  
                                                                                           
    Existing tests cover parsing operator encodings.                                       
                                                                                           
    Reviewed By: ChuanqiXu                                                                 
                                                                                           
    Differential Revision: https://reviews.llvm.org/D119467

But couldn't quite tell from the diff why this condition was necessary.

@Michael137
Copy link
Member Author

Michael137 commented Apr 24, 2025 •
edited
Loading

Ah what was probably meant by this is that the operators are non-overloadable, and thus "not nameable". If we want to keep that check we should at least mark ->* as "Named", since it can be overloaded and something that Clang will generate. Though FWIW, the GCC demangler does demangle . and .* accesses just fine

@Michael137 Michael137 changed the title [ItaniumDemangle] Remove Named flag from OperatorInfo::Member entries [ItaniumDemangle] Add Named flag to "pm" operator Apr 25, 2025
@Michael137
Copy link
Member Author

Decided to put condition back and just make pm named. Where "Named" basically means "is it overloadable"

ldionne
ldionne approved these changes May 6, 2025
View reviewed changes
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please give a few days for @urnathan to chime in, but I'd say you can merge this at EOW if there's no further activity.

@ldionne ldionne merged commit 028f70d into llvm:main May 12, 2025
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urnathan urnathan urnathan left review comments

@ldionne ldionne ldionne approved these changes

Assignees
No one assigned
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Michael137 @llvmbot @ldionne @urnathan
Morty Proxy This is a proxified and sanitized view of the page, visit original site.