-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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.
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
[ItaniumDemangle] Set InConstraintExpr
to true
when demangling a constraint expression
#107385
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -5737,6 +5737,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseTemplateParamDecl( | |||
} | |||
|
|||
if (consumeIf("Tk")) { | |||
ScopedOverride<bool> SaveInConstraintExpr(InConstraintExpr, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test that triggers this behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sure, added.
@@ -5737,6 +5737,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseTemplateParamDecl( | |||
} | |||
|
|||
if (consumeIf("Tk")) { | |||
ScopedOverride<bool> SaveInConstraintExpr(InConstraintExpr, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, renamed.
@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. |
@@ -5737,6 +5737,7 @@ Node *AbstractManglingParser<Derived, Alloc>::parseTemplateParamDecl( | |||
} | |||
|
|||
if (consumeIf("Tk")) { | |||
ScopedOverride<bool> SaveInConstraintExpr(InConstraintExpr, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, there's already a comment explaining this variable, I can add a similar one here.
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) |
@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:
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
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Thanks, added a test case and made the changes to libcxxabi demangler (synced to llvm with cp-to-llvm.sh), |
InConstraintExpr
to true
when demangling a c…InConstraintExpr
to true
when demangling a c…
@@ -0,0 +1,3 @@ | |||
RUN: llvm-cxxfilt -n _ZN3FooIiE6methodITk4TrueIT_EiEEvS3_ | FileCheck %s | |||
|
|||
CHECK: void Foo<int>::method<int>(T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we'll need to add a test-case to libcxxabi/test/test_demangle.pass.cpp
InConstraintExpr
to true
when demangling a c…InConstraintExpr
to true
when demangling a constraint expression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
if (demang) | ||
buf = demang; | ||
void test() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that was an accidental change, I have removed it. I don't intend to reformat the file.
…traint expression
Moved the testcase to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
This prevents demangler failures until the TODO in the demangler is implemented.
This is a temporary fix for #89914.