Fixes PR28101
(https://llvm.org/PR28101)
by setting identifier for invalid member variables with template parameters,
so that the invalid declarators would not crash clang.
Details
- Reviewers
aaron.ballman erichkeane cor3ntin - Commits
- rG355f1c75aa66: [Clang] Fix PR28101
Diff Detail
Event Timeline
Can you add some tests please? Additionally, we don't typically use LLVM_UNLIKELY like you did there.
FWIW, this appears to cause quite a few failures in precommit CI that should be addressed as well: https://buildkite.com/llvm-project/premerge-checks/builds/68803#45a9b858-fa66-4a4b-8c4a-20c2cef820e5
Failed Tests (11): Clang :: CXX/basic/basic.lookup/basic.lookup.qual/class.qual/p2.cpp Clang :: CXX/drs/dr3xx.cpp Clang :: CXX/special/class.ctor/p1.cpp Clang :: CXX/temp/temp.deduct.guide/p1.cpp Clang :: CodeGenObjCXX/block-in-template-inst.mm Clang :: Parser/explicit-bool.cpp Clang :: SemaCXX/constant-expression-cxx11.cpp Clang :: SemaCXX/constructor.cpp Clang :: SemaCXX/conversion-function.cpp Clang :: SemaCXX/cxx2a-compat.cpp Clang :: SemaCXX/destructor.cpp
clang/lib/Parse/ParseDecl.cpp | ||
---|---|---|
6485–6488 | This fix looks incredibly specific to the bug report, how does it handle other situations, like: template <typename T, template <typename> typename U> class PR28101 { public: PR28101(void *) {} T(PR28101<T, U<T>>) {} // Do we err here? }; template <typename T> struct S {}; PR28101<int, S> foo() { return PR28101<int, S>(nullptr); } | |
clang/test/CXX/temp/temp.decls/temp.mem/p3.cpp | ||
12–13 | It seems incorrect that we're issuing the same diagnostic twice. Is this because of template instantiation, or some other reason? | |
16 | The test location is a bit novel -- [temp.mem]p3 is "A member function template shall not be declared virtual." and I don't see anything related to p3 in this test. I think the test case is reasonable enough, but it should probably live in SemaCXX instead of here. |
The previous diff was indeed very specific and doesn't handle
template <typename T> struct A { A(void*) {} T A<T>{}; // expected-error{{member 'A' cannot have template arguments}} }; A<int> instantiate() { return {nullptr}; }
I have since realized the invalid decorator is the problem and the parentheses are insignificant. It's like:
template <typename T> struct S {}; int S<int>{};
which gcc diagnoses to
error: invalid declarator before '{' token
Then I found there was a new diagnostic added by D120881. So the fix is pretty straightforward at this point. I simply stand on the shoulder of the giant and set the invalid flag of the declarator.
Diagnose "same name as its class" before setting the declarator invalid as otherwise it would not be diagnosed. This also aligns with gcc's behavior.
clang/lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
3430 ↗ | (On Diff #417504) | I see we are diagnosing this ONLY inside of the case where it is a template. Where is this caught when we are in the non-template case, why doesn't THAT catch these cases, and what would it take to do so? |
clang/test/SemaCXX/PR28101.cpp | ||
8 ↗ | (On Diff #417504) | This isn't necessary, the compiler in verify mode should see all errors. I see above at most 2 invocations of the compiler as necessary. |
13 ↗ | (On Diff #417504) | This second error here is strange, I don't see it as particularly useful here. I see gcc at least gives the full name (int A<int>::A), perhaps that makes this more useful? |
16 ↗ | (On Diff #417504) | How come there are no notes in this test that say 'in instantiation of...'? I would expect those in at least some of these cases, right? |
clang/lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
3430 ↗ | (On Diff #417504) | CheckCompletedCXXClass should catch diag::err_member_name_of_class. However, these FieldDecl have no name hence uncaught. Maybe we should set the identifier for this declarator here. |
clang/test/SemaCXX/PR28101.cpp | ||
8 ↗ | (On Diff #417504) | Instantiation of each of the 4 classes could crash on its own, so I thought I'd separate them into 4 invocations. |
16 ↗ | (On Diff #417504) | Because it was caught before instantiation and when instantiating the FieldDecl is gone after calling D.setInvalidType();. If I set the identifier for the FieldDecl and let CheckCompletedCXXClass handle the "has the same name as its class" error, there will be the "in instantiation of..." note. |
clang/lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
3430 ↗ | (On Diff #417504) | I'm not sure what the side effect of that is... Can you give it a shot? |
clang/test/SemaCXX/PR28101.cpp | ||
8 ↗ | (On Diff #417504) | They shouldn't crash afterwards, right? So this shouldn't be a problem. |
16 ↗ | (On Diff #417504) | I think we definitely need to do so there, the instantiation trace not being here makes this a much less useful diagnostic. |
Nope, see release notes here: https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst
This fix looks incredibly specific to the bug report, how does it handle other situations, like: