Repository navigation
[Clang][RAV] Simplify TraverseTemplateArgumentLocsHelper - #199131
Conversation
We were checking the result of getTemplateArgsAsWritten() to skip over implicit instantiations, with an assert to ensure that it has the desired effect, before checking getTemplateSpecializationKind() == TSK_ExplicitSpecialization which would skip over implicit instantiations anyway. As the included tests show, the invariant that we were relying on did not hold, but we no longer have any need to rely on that, we can now just check the result of getTemplateSpecializationKind() directly. Fixes: llvm#198903 Fixes: llvm#169302
llvmorg-github-actions
Bot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Harald van Dijk (hvdijk) ChangesWe were checking the result of getTemplateArgsAsWritten() to skip over implicit instantiations, with an assert to ensure that it has the desired effect, before checking getTemplateSpecializationKind() == TSK_ExplicitSpecialization which would skip over implicit instantiations anyway. As the included tests show, the invariant that we were relying on did not hold, but we no longer have any need to rely on that, we can now just check the result of getTemplateSpecializationKind() directly. Fixes: #198903 Full diff: https://github.com/llvm/llvm-project/pull/199131.diff 3 Files Affected:
diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h b/clang/include/clang/AST/RecursiveASTVisitor.h
index febdf715698d9..2efdde5450f3c 100644
--- a/clang/include/clang/AST/RecursiveASTVisitor.h
+++ b/clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2222,25 +2222,20 @@ bool RecursiveASTVisitor<Derived>::TraverseTemplateArgumentLocsHelper(
handles traversal of template args and qualifier. \
For explicit specializations ("template<> set<int> {...};"), \
we traverse template args here since there is no EID. */ \
- if (const auto *ArgsWritten = D->getTemplateArgsAsWritten()) { \
- assert(D->getTemplateSpecializationKind() != TSK_ImplicitInstantiation); \
- if (D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) { \
- TRY_TO(TraverseTemplateArgumentLocsHelper( \
- ArgsWritten->getTemplateArgs(), ArgsWritten->NumTemplateArgs)); \
- } \
- } \
- \
- if (getDerived().shouldVisitTemplateInstantiations() || \
- D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) { \
- /* Traverse base definition for explicit specializations */ \
- TRY_TO(Traverse##DECLKIND##Helper(D)); \
- } else { \
+ if (D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) { \
+ const auto *ArgsWritten = D->getTemplateArgsAsWritten(); \
+ TRY_TO(TraverseTemplateArgumentLocsHelper( \
+ ArgsWritten->getTemplateArgs(), ArgsWritten->NumTemplateArgs)); \
+ } else if (!getDerived().shouldVisitTemplateInstantiations()) { \
/* Returning from here skips traversing the \
declaration context of the *TemplateSpecializationDecl \
(embedded in the DEF_TRAVERSE_DECL() macro) \
which contains the instantiated members of the template. */ \
return true; \
} \
+ \
+ /* Traverse base definition for explicit specializations */ \
+ TRY_TO(Traverse##DECLKIND##Helper(D)); \
})
DEF_TRAVERSE_TMPL_SPEC_DECL(Class, CXXRecord)
diff --git a/clang/test/AST/pr198903.cpp b/clang/test/AST/pr198903.cpp
new file mode 100644
index 0000000000000..1f0f68f92b7e4
--- /dev/null
+++ b/clang/test/AST/pr198903.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -ast-list %s | FileCheck -strict-whitespace %s
+
+template <typename>
+struct Tpl {
+ template <typename>
+ static int var;
+};
+// CHECK: Tpl
+// CHECK-NEXT: Tpl::(anonymous)
+// CHECK-NEXT: Tpl
+// CHECK-NEXT: Tpl::var
+// CHECK-NEXT: Tpl::(anonymous)
+// CHECK-NEXT: Tpl::var
+
+template <typename T>
+template <typename>
+int Tpl<T>::var;
+// CHECK-NEXT: Tpl::var
+// CHECK-NEXT: Tpl::(anonymous)
+// CHECK-NEXT: Tpl::var
+// CHECK-NEXT: T
+
+int i = Tpl<int>::var<int>;
+// CHECK-NEXT: i
+// CHECK-NEXT: Tpl<int>::var
diff --git a/clang/test/Analysis/pr169302.cpp b/clang/test/Analysis/pr169302.cpp
new file mode 100644
index 0000000000000..9aa594627708a
--- /dev/null
+++ b/clang/test/Analysis/pr169302.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core -verify %s
+
+// expected-no-diagnostics
+
+template <typename> struct S;
+
+class Sp {
+public:
+ template <bool> void M() {}
+ template <unsigned> struct I {
+ static void IM();
+ };
+};
+
+template <> struct S<Sp> {
+ using F = void (Sp::*)();
+ template <bool P> static constexpr F SpM = &Sp::template M<P>;
+};
+
+template <bool> constexpr S<Sp>::F S<Sp>::SpM;
+
+template <unsigned X> void Sp::I<X>::IM() {
+ using Spec = S<Sp>;
+ typename Spec::F E = Spec::template SpM<true>;
+}
|
|
cc @16bit-ykiko, I can't add you as a reviewer, but this is based on your PR #191658 and if you have comments on this, I would welcome them. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
|
As I recall, there is a strange asymmetry in how implicit instantiations are represented in the AST: template <typename T> T var = T{};
template <typename T> struct S {};
template <typename T> T func() { return T{}; }
void test() {
int x = var<int>;
S<int> s;
int y = func<int>();
}https://godbolt.org/z/njdM1b51G This is caused by |
|
So I think your PR fix is reasonable for this specific problem, but it may only be addressing the surface-level cause. There might be deeper bugs here that need investigation. Also, I feel that Clang's AST traversal has always lacked thorough testing. There are recurring cases where certain nodes get unexpectedly visited twice. We probably need a better way to test traversal correctness. cc @erichkeane @mizvekov @zyn0217 any thoughts on this? |
Oh, absolutely. We've had a mismatch between what we assume and what we actually do for a long time, and I noted in #198903 I had actually wanted to yank earlier changes because it kept being a problem. This PR is an attempt to sidestep that, we still don't do what we want to do, we just remove the assumption that we do. |
| we traverse template args here since there is no EID. */ \ | ||
| if (const auto *ArgsWritten = D->getTemplateArgsAsWritten()) { \ | ||
| assert(D->getTemplateSpecializationKind() != TSK_ImplicitInstantiation); \ | ||
| if (D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) { \ |
There was a problem hiding this comment.
It is astonishing how these two adjacent lines were overlooked by several people (including me).
No, I think it is anyway worth fixing because even if var template implicit specializations weren't traversed as top-level decls, they could be traversed as child nodes of the corresponding llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h Lines 2081 to 2083 in c53f299 |
Yes, I agree with this fix. My point is just that it's a bit strange — why does only variable template trigger this issue, while class template and function template don't? There must be some deeper root cause here that we should investigate and fix. This could be a good opportunity, since it seems like people haven't really recognized that variable template handling has quite a few issues. Of course, that doesn't need to be addressed in this PR. |
|
FYI explicit instantiations are really quite similar to implicit ones, except they have different linkage, and they have some extra baggage, like some properties which really ought to appertain to the I think with @16bit-ykiko 's work on ExplicitInstantiationDecl, we could simplify this and remove both explicit instantiation specialization kinds, and treat explicit instantiations just like implicit instantiations almost everywhere, except that we can use the |
|
It's annoying and disrespectful that people involved this PR would then submit a later conflicting PR, fixing the bugs that this was meant to fix, without commenting here, and without including the tests to ensure that this remains fixed. I've updated this PR to resolve the conflicts and keep the simplification and tests. Please let me now just actually merge it. |
|
Updated again based on that conflicting PR being reverted, the revert naturally reintroducing the same conflict with this PR. |
|
ping |
We were checking the result of getTemplateArgsAsWritten() to skip over implicit instantiations, with an assert to ensure that it has the desired effect, before checking getTemplateSpecializationKind() == TSK_ExplicitSpecialization which would skip over implicit instantiations anyway. As the included tests show, the invariant that we were relying on did not hold, but we no longer have any need to rely on that, we can now just check the result of getTemplateSpecializationKind() directly. Fixes: llvm#198903 Fixes: llvm#169302
…csHelper Cherry-pick of upstream faaec33 ("[Clang][RAV] Simplify TraverseTemplateArgumentLocsHelper", llvm#199131, fixes llvm#198903 and llvm#169302), unchanged, plus one regression test of our own. This fixes a CRASH on ordinary C++ that this branch is far more exposed to than main. `DEF_TRAVERSE_TMPL_SPEC_DECL` used `getTemplateArgsAsWritten() != nullptr` as a proxy for "is not an implicit instantiation", with an assert added in d2ac21d to catch regressions of that invariant. The invariant does not hold: for an out-of-line static data member template, `Sema::InstantiateVariableDefinition` calls `setTemplateArgsAsWritten` unconditionally, and that setter always allocates -- so an implicit instantiation ends up reporting a non-null, empty list and the assert fires. Upstream's fix drops the proxy and tests `getTemplateSpecializationKind() == TSK_ExplicitSpecialization` directly. Three lines were enough to crash an asserts build at every standard >= C++14: struct B { template<typename T> static T v; }; template<typename T> T B::v = T(); float fsvar = B::v<float>; Main only reaches this through its one instantiation-visiting sweep, gated on -fsafe-buffer-usage-suggestions. We have five, and the four lifetime-safety TU sweeps in AnalysisBasedWarnings::IssueWarnings run for every C++ TU with no flag required, so any TU containing this shape crashed. Fixes 7 previously-crashing lit tests -- AST/ast-dump-decl.cpp, CodeGenCXX/{cxx1y-variable-template,dllexport-members,visibility}.cpp, Modules/initializers.cpp, SemaCXX/cxx98-compat.cpp, SemaTemplate/instantiate-partial-spec.cpp -- and breaks none: clang/test goes from 48 failures to 41, with the failure lists differing only by those 7. The remaining 41 are pre-existing and unrelated. Lifetime side unaffected: 80/80 unit tests, 363/363 soundness corpus, asteroids clean. Preferred over patching Sema: the AST behaviour is unchanged, which is what upstream chose too after finding the same broken invariant.
5 participants
We were checking the result of getTemplateArgsAsWritten() to skip over implicit instantiations, with an assert to ensure that it has the desired effect, before checking getTemplateSpecializationKind() == TSK_ExplicitSpecialization which would skip over implicit instantiations anyway. As the included tests show, the invariant that we were relying on did not hold, but we no longer have any need to rely on that, we can now just check the result of getTemplateSpecializationKind() directly.
Fixes: #198903
Fixes: #169302