[clang] Improve nested name specifier AST representation#147835
[clang] Improve nested name specifier AST representation#147835
Conversation
|
This feels like a regression, unless you think the bug is elsewhere and the assert just catches it now: #156458 (comment) |
|
"And on that day, every maintainer of a nontrivial clang fork cried out to the sky in unison." |
|
@mizvekov there is one more instance of a clang crash that we are encountering during our testing at google. Here is the reproducer https://godbolt.org/z/jfeW9d9Ts. Please take a look. |
|
For convenience copying the code and clang stdout here: |
|
And a bit cleaned up (https://godbolt.org/z/q9fGPj54d): |
|
FWIW that crash is pre-existing, this PR just adds another way to reproduce it. Here is a reproducer for clang-21, using Typedefs instead of RecordTypes to induce the problem: https://compiler-explorer.com/z/E89P31brT class A;
using B = A;
using T = void(__attribute__((swift_async_context)) B *);
T R;
using B = A;
void fa(__attribute__((swift_async_context)) B *);
bool f() {
return R == fa;
} |
|
@joanahalili @alexfh that's fixed in #157925 |
|
On a positive note, we just updated Chromium's toolchain and saw a 5% reduction in build time, which I'm assuming is due to this change :-) |
|
Hello, did these changes modify the semantics of |
I think the documentation is right and matches what's happening. Sounds like you want to get the beginning of the range as something like: if (auto TL = NNSL->getAsTypeLoc())
return TL.getNonPrefixBeginLoc();
else
return NNSL->getLocalBeginLoc();I didn't add a helper for this because there was just a single user in the llvm codebase using this pattern. |
Just my two cents. I think the term "local" is ambiguous, and my mental model has always been that it refers to the last specifier in a qualified name. This seems to have been the behavior, and the public API doc implies it. This recent change seems to break that model and introduces behavior that feels inconsistent:
This difference is confusing. While I understand the reasoning behind the internal implementation, API users aren't likely to be aware of it, and the "last piece" model seems more intuitive. If this new behavior is intended, could we update the comments for |
|
Yeah, all except one user of I propose adding a note about this in the documentation, add a new helper which does what |
Thanks, this workaround works for me. It's just that such subtle changes of the API (the function interface wasn't changed, its documentation wasn't changed and changelog wasn't clear on this backwards-incompatible change) are very hard to pinpoint&address in downstream projects. |
|
After another look at this, we can remove that single potential user and that's a simplification over the status quo. See #163206 Also removes TypeLoc::getNonPrefixBeginLoc, which becomes unused after this change. @emaxx-google see if your problem can be solved with a change similar to the above patch, that would be better than the solution I posted earlier. I added the note on this patch, but also see that the example in that documentation already reflected that change. |
|
I just posted #163271 which will rename getOriginalDecl back to getDecl, as I think enough time has passed and there has been a big enough window to facilitate any rebases. Let me know if anyone needs more time. Ideally we would like to get this in before the release, so that upstream users that need to support multiple clang versions don't need a bunch of extra |
This is a major change on how we represent nested name qualifications in the AST.
This patch offers a great performance benefit.
It greatly improves compilation time for stdexec. For one datapoint, for
test_on2.cppin that project, which is the slowest compiling test, this patch improves-ccompilation time by about 7.2%, with the-fsyntax-onlyimprovement being at ~12%.This has great results on compile-time-tracker as well:
This patch also further enables other optimziations in the future, and will reduce the performance impact of template specialization resugaring when that lands.
It has some other miscelaneous drive-by fixes.
About the review: Yes the patch is huge, sorry about that. Part of the reason is that I started by the nested name specifier part, before the ElaboratedType part, but that had a huge performance downside, as ElaboratedType is a big performance hog. I didn't have the steam to go back and change the patch after the fact.
There is also a lot of internal API changes, and it made sense to remove ElaboratedType in one go, versus removing it from one type at a time, as that would present much more churn to the users. Also, the nested name specifier having a different API avoids missing changes related to how prefixes work now, which could make existing code compile but not work.
How to review: The important changes are all in
clang/include/clang/ASTandclang/lib/AST, with also important changes inclang/lib/Sema/TreeTransform.h.The rest and bulk of the changes are mostly consequences of the changes in API.
PS: TagType::getDecl is renamed to
getOriginalDeclin this patch, just for easier to rebasing. I plan to rename it back after this lands.Fixes #136624
Fixes #43179
Fixes #68670
Fixes #92757