[ItaniumDemangle] Set `InConstraintExpr` to `true` when demangling a constraint expression #107385

VitaNuo · 2024-09-05T11:39:11Z

This prevents demangler failures until the TODO in the demangler is implemented.

This is a temporary fix for #89914.

ilya-biryukov

My main request is to add a test for the cases that used to fail. Otherwise LG, and I hope this is temporary and we would follow up with addressing the actual underlying problem.

Also, see a few suggestions on naming and wording.

Suggestion: a much more common tag for the commit title is [Demangle] or [ItaniumDemangle]. Could we use one of those two for consistency?

Could you also mention #89914 in the commit message so that people can access the relevant discussions right away?

ilya-biryukov · 2024-09-06T11:31:51Z

llvm/include/llvm/Demangle/ItaniumDemangle.h

@@ -5737,6 +5737,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseTemplateParamDecl(
  }

  if (consumeIf("Tk")) {
+    ScopedOverride<bool> SaveInConstraintExpr(InConstraintExpr, true);


Could you add a test that triggers this behavior?

Yes, sure, added.

ilya-biryukov · 2024-09-06T11:50:14Z

llvm/include/llvm/Demangle/ItaniumDemangle.h

@@ -5737,6 +5737,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseTemplateParamDecl(
  }

  if (consumeIf("Tk")) {
+    ScopedOverride<bool> SaveInConstraintExpr(InConstraintExpr, true);


Could we rename InConstraintExpr to something that matches what we're trying to do better?
The main problem seems to be that it is not just <constraint-expr> that may have problems resolving the names of template parameters due to lack of mapping, but also the template arguments that preceded it.

<template-args> ::= I <template-arg>* [Q <requires-clause expr>] E

Something like HasIncompleteTemplateParameterTracking would make the code more readable.

Thanks, renamed.

ilya-biryukov · 2024-09-06T11:55:18Z

@hokein do you have any suggestions for reviewers from the Demangler side that we could loop in?

hokein · 2024-09-06T14:33:19Z

@hokein do you have any suggestions for reviewers from the Demangler side that we could loop in?

I think @Michael137 may have relevant context, so I'm looping him in as well.

hokein · 2024-09-06T14:30:27Z

llvm/include/llvm/Demangle/ItaniumDemangle.h

@@ -5737,6 +5737,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseTemplateParamDecl(
  }

  if (consumeIf("Tk")) {
+    ScopedOverride<bool> SaveInConstraintExpr(InConstraintExpr, true);


Since this is a temporary fix, it would be helpful to add comments that explain both what the newly-added line of code is doing and why it’s necessary for now.

Sure, there's already a comment explaining this variable, I can add a similar one here.

Michael137 · 2024-09-06T15:25:13Z

I'll have to page back in my context from when I last looked, but would be nice if we could just fix this properly. Agreed that a test case is necessary

Also, the changes will have to be made to the libcxxabi demangler (and then synced to llvm with cp-to-llvm.sh)

llvmbot · 2024-09-10T07:43:10Z

@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (VitaNuo)

Changes

…onstraint expression.

This prevents demangler failures until the TODO in the demangler is implemented.

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

2 Files Affected:

(modified) llvm/include/llvm/Demangle/ItaniumDemangle.h (+1)
(added) llvm/test/tools/llvm-cxxfilt/demangle-constrained-template.test (+3)

diff --git a/llvm/include/llvm/Demangle/ItaniumDemangle.h b/llvm/include/llvm/Demangle/ItaniumDemangle.h
index fb2060fa07f724..5c044def74e881 100644
--- a/llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ b/llvm/include/llvm/Demangle/ItaniumDemangle.h
@@ -5737,6 +5737,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseTemplateParamDecl(
   }
 
   if (consumeIf("Tk")) {
+    ScopedOverride<bool> SaveInConstraintExpr(InConstraintExpr, true);
     Node *Constraint = getDerived().parseName();
     if (!Constraint)
       return nullptr;
diff --git a/llvm/test/tools/llvm-cxxfilt/demangle-constrained-template.test b/llvm/test/tools/llvm-cxxfilt/demangle-constrained-template.test
new file mode 100644
index 00000000000000..7a038ebb77a06d
--- /dev/null
+++ b/llvm/test/tools/llvm-cxxfilt/demangle-constrained-template.test
@@ -0,0 +1,3 @@
+RUN: llvm-cxxfilt -n _ZN3FooIiE6methodITk4TrueIT_EiEEvS3_ | FileCheck %s
+
+CHECK: void Foo<int>::method<int>(T)
\ No newline at end of file

github-actions · 2024-09-10T08:11:38Z

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

VitaNuo · 2024-09-10T08:57:54Z

I'll have to page back in my context from when I last looked, but would be nice if we could just fix this properly. Agreed that a test case is necessary

Also, the changes will have to be made to the libcxxabi demangler (and then synced to llvm with cp-to-llvm.sh)

Thanks, added a test case and made the changes to libcxxabi demangler (synced to llvm with cp-to-llvm.sh),

Michael137 · 2024-09-10T10:58:45Z

llvm/test/tools/llvm-cxxfilt/demangle-constrained-template.test

@@ -0,0 +1,3 @@
+RUN: llvm-cxxfilt -n _ZN3FooIiE6methodITk4TrueIT_EiEEvS3_ | FileCheck %s
+
+CHECK: void Foo<int>::method<int>(T)


This looks weird to me actually. I think a user would be confused as to where this T here came from. I don't think we ever print out generic template parameter names. Probably what happened here is that the S3_ substitution picks up the fake constraint argument name that we deduced because of HasIncompleteTemplateParameterTracking.

I think what we really want to do is track the parameter levels properly, so that constraint expressions have access to template parameters in enclosing scopes. Currently parseTemplateArgs creates a fresh template parameter level because (from what I can tell) the demangler assumes we never reference template parameters from outer levels. But that's not the case anymore with constraints it looks like. Maybe just avoiding clearing the level if we're parsing a Tk is enough? I'm not sure atm, but maybe @zygoloid @urnathan have some better ideas here

I don't think we ever print out generic template parameter names

I think @VitaNuo showed me an example where we do already print those names (in constraints, not in lambda parameters). Hence, this workaround made sense until the proper fix lands.
@VitaNuo do you have those examples?

Maybe just avoiding clearing the level if we're parsing a Tk is enough?

It's probably more complicated than this. Apart from Tk, we should also handle constraints (Q IIRC). And the actual parameters we track do need to be reset at certain points when the new "top-level" name is being parsed. To put it another way, the parameter numbering is not "global" to the mangled name, instead the same numbering can mean different types at different points of the mangled name and we are not sure what are the points at which the template numbering should be reset or backtracked. @VitaNuo is in the process of figuring that out.

Also wanted to share https://gcc.godbolt.org/z/dqT877q6K and https://gcc.godbolt.org/z/bcfWqGdGz that we came up with yesterday that use TL<n> numbering referencing different types at different points in the mangled name.

PS Maybe the solution you have in mind already covers that, but I wasn't sure, so wanted to bring this up.

I think @VitaNuo showed me an example where we do already print those names (in constraints, not in lambda parameters). Hence, this workaround made sense until the proper fix lands.
@VitaNuo do you have those examples?

Possibly these

{"_ZZN5test71fIiEEvvENKUlTyQaa1CIT_E1CITL0__ET0_E_clIiiEEDaS3_Q1CIDtfp_EE", "auto void test7::f<int>()::'lambda'<typename $T> requires C<T> && C<TL0_> (auto)::operator()<int, int>(auto) const requires C<decltype(fp)>"}, {"_ZZN5test71fIiEEvvENKUlTyQaa1CIT_E1CITL0__ET0_E0_clIiiEEDaS3_Qaa1CIDtfp_EELb1E", "auto void test7::f<int>()::'lambda0'<typename $T> requires C<T> && C<TL0_> (auto)::operator()<int, int>(auto) const requires C<decltype(fp)> && true"}, {"_ZZN5test71fIiEEvvENKUlTyQaa1CIT_E1CITL0__ET0_E1_clIiiEEDaS3_Q1CIDtfp_EE", "auto void test7::f<int>()::'lambda1'<typename $T> requires C<T> && C<TL0_> (auto)::operator()<int, int>(auto) const requires C<decltype(fp)>"},

from libcxxabi/test/test_demangle.pass.cpp. These don't demangle without setting InConstraintExpr.

But I don't remember showing you any such examples apart from the one in this PR, to be honest.

yeah, this seems to be the case where we do show the constraints, hence can also show the "raw" parameter names.

@Michael137 given that we do show "raw" T and TL parameters in constraint expressions already, I think it's reasonable to land this workaround. Right now, the demangling completely gives up when it hits a constrained parameter, but shows raw template numbering when demangling a requires clause.

Not failing seems like a strictly better outcome, because users can at least see the other bits of demangled name (we're chasing this because some of our widely-used functions can't be demangled).

I would vouch for landing this workaround while @VitaNuo is working on a proper fix. WDYT?

It's probably more complicated than this. Apart from Tk, we should also handle constraints (Q IIRC). And the actual parameters we track do need to be reset at certain points when the new "top-level" name is being parsed. To put it another way, the parameter numbering is not "global" to the mangled name, instead the same numbering can mean different types at different points of the mangled name and we are not sure what are the points at which the template numbering should be reset or backtracked.

Yea I imagined this to be more complicated than I made it out to be. Happy to land it as is since it's better than failing to demangle at all. And there's precedent for printing the template numbering with constraint expressions.

Michael137

Also, we'll need to add a test-case to libcxxabi/test/test_demangle.pass.cpp

Michael137

Seems reasonable to me, since we do the same for constraint expressions already.

Nit: A more reasonable place for the test is libcxxabi/test/test_demangle.pass

Please give some time for the other reviewers to chime in, in case they have concerns/suggestions

Michael137 · 2024-09-11T16:04:38Z

libcxxabi/test/test_demangle.pass.cpp

-        }
-        if (demang)
-          buf = demang;
+void test() {


Minor nit: can we clang-format this file separately? And also put a // clang-format off around the invalid_cases (like we do for the cases array). It's easier to copy-paste out the test-cases when they're not split across multiple lines

Sorry, that was an accidental change, I have removed it. I don't intend to reformat the file.

…traint expression

VitaNuo · 2024-09-11T16:28:20Z

Also, we'll need to add a test-case to libcxxabi/test/test_demangle.pass.cpp

Moved the testcase to libcxxabi/test/test_demangle.pass.cpp, should be good to go.

mizvekov

I agree the temporary fix is better than leaving as is, so LGTM.

I am shocked to learn the source is duplicated between llvm and libcxx.

VitaNuo · 2024-09-12T06:58:48Z

Thanks for the review. As all the reviewers seem to have responded, I will proceed to merge this.

This follows what the LLVM demangler does in llvm/llvm-project#107385.

VitaNuo requested a review from ilya-biryukov September 5, 2024 11:40

VitaNuo self-assigned this Sep 5, 2024

VitaNuo linked an issue Sep 5, 2024 that may be closed by this pull request

[abi] mangled name for constrained template is not demangled by llvm-cxxfilt #89914

Open

ilya-biryukov requested changes Sep 6, 2024

View reviewed changes

hokein reviewed Sep 6, 2024

View reviewed changes

llvmbot added the llvm:binary-utilities label Sep 10, 2024

VitaNuo requested a review from a team as a code owner September 10, 2024 07:53

llvmbot added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Sep 10, 2024

VitaNuo changed the title ~~[LLVM Demangler] Set InConstraintExpr to true when demangling a c…~~ [ItaniumDemangle] Set InConstraintExpr to true when demangling a c… Sep 10, 2024

cor3ntin requested a review from mizvekov September 10, 2024 09:00

VitaNuo force-pushed the main branch from 586fd56 to 36aabbe Compare September 10, 2024 09:27

VitaNuo closed this Sep 10, 2024

VitaNuo force-pushed the main branch from 36aabbe to 07bef02 Compare September 10, 2024 09:39

VitaNuo reopened this Sep 10, 2024

VitaNuo requested a review from ilya-biryukov September 10, 2024 09:47

VitaNuo force-pushed the main branch from a7ff757 to cfc467d Compare September 10, 2024 09:55

Michael137 reviewed Sep 10, 2024

View reviewed changes

ldionne changed the title ~~[ItaniumDemangle] Set InConstraintExpr to true when demangling a c…~~ [ItaniumDemangle] Set InConstraintExpr to true when demangling a constraint expression Sep 10, 2024

Michael137 approved these changes Sep 10, 2024

View reviewed changes

Michael137 reviewed Sep 11, 2024

View reviewed changes

[ItaniumDemangle] Set InConstraintExpr to true when demangling a cons…

522ed38

…traint expression

VitaNuo force-pushed the main branch from 6402b1d to 522ed38 Compare September 11, 2024 16:14

mizvekov approved these changes Sep 11, 2024

View reviewed changes

VitaNuo merged commit c22b68c into llvm:main Sep 12, 2024
66 checks passed

ianlancetaylor added a commit to ianlancetaylor/demangle that referenced this pull request Sep 12, 2024

demangle: skip template parameter substitutions in constraints

0a2b629

This follows what the LLVM demangler does in llvm/llvm-project#107385.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ItaniumDemangle] Set `InConstraintExpr` to `true` when demangling a constraint expression #107385

[ItaniumDemangle] Set `InConstraintExpr` to `true` when demangling a constraint expression #107385

VitaNuo commented Sep 5, 2024 •

edited by ldionne

Loading

ilya-biryukov left a comment

ilya-biryukov Sep 6, 2024

VitaNuo Sep 10, 2024

ilya-biryukov Sep 6, 2024

VitaNuo Sep 10, 2024

ilya-biryukov commented Sep 6, 2024

hokein commented Sep 6, 2024

hokein Sep 6, 2024

VitaNuo Sep 10, 2024 •

edited

Loading

Michael137 commented Sep 6, 2024 •

edited by VitaNuo

Loading

llvmbot commented Sep 10, 2024 •

edited

Loading

github-actions bot commented Sep 10, 2024 •

edited

Loading

VitaNuo commented Sep 10, 2024

Michael137 Sep 10, 2024 •

edited

Loading

ilya-biryukov Sep 10, 2024

VitaNuo Sep 10, 2024

ilya-biryukov Sep 10, 2024

Michael137 Sep 10, 2024

Michael137 left a comment

Michael137 left a comment •

edited

Loading

Michael137 Sep 11, 2024

VitaNuo Sep 11, 2024

VitaNuo commented Sep 11, 2024

mizvekov left a comment

VitaNuo commented Sep 12, 2024

		@@ -0,0 +1,3 @@
		RUN: llvm-cxxfilt -n _ZN3FooIiE6methodITk4TrueIT_EiEEvS3_ \| FileCheck %s

		CHECK: void Foo<int>::method<int>(T)

[ItaniumDemangle] Set InConstraintExpr to true when demangling a constraint expression #107385

[ItaniumDemangle] Set InConstraintExpr to true when demangling a constraint expression #107385

Conversation

VitaNuo commented Sep 5, 2024 • edited by ldionne Loading

ilya-biryukov 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

ilya-biryukov commented Sep 6, 2024

hokein commented Sep 6, 2024

Choose a reason for hiding this comment

VitaNuo Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Michael137 commented Sep 6, 2024 • edited by VitaNuo Loading

llvmbot commented Sep 10, 2024 • edited Loading

github-actions bot commented Sep 10, 2024 • edited Loading

VitaNuo commented Sep 10, 2024

Michael137 Sep 10, 2024 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Michael137 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VitaNuo commented Sep 11, 2024

mizvekov left a comment

Choose a reason for hiding this comment

VitaNuo commented Sep 12, 2024

[ItaniumDemangle] Set `InConstraintExpr` to `true` when demangling a constraint expression #107385

[ItaniumDemangle] Set `InConstraintExpr` to `true` when demangling a constraint expression #107385

VitaNuo commented Sep 5, 2024 •

edited by ldionne

Loading

VitaNuo Sep 10, 2024 •

edited

Loading

Michael137 commented Sep 6, 2024 •

edited by VitaNuo

Loading

llvmbot commented Sep 10, 2024 •

edited

Loading

github-actions bot commented Sep 10, 2024 •

edited

Loading

Michael137 Sep 10, 2024 •

edited

Loading

Michael137 left a comment •

edited

Loading