Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[Clang] Fix PR28101
ClosedPublic

Authored by rZhBoYao on Dec 7 2021, 6:11 AM.

Details

Summary

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.

Diff Detail

Event Timeline

rZhBoYao requested review of this revision.Dec 7 2021, 6:11 AM
rZhBoYao created this revision.
rZhBoYao updated this revision to Diff 392399.Dec 7 2021, 7:40 AM

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
rZhBoYao updated this revision to Diff 392774.Dec 8 2021, 7:42 AM
rZhBoYao edited the summary of this revision. (Show Details)

I added a test. There were 17 module and PCH related failed tests on my local machine; however, when I ran the test on then ToT the same 17 tests failed, but on GitHub it does show 2 successful checks. So maybe it's something else.

rZhBoYao retitled this revision from [clang] Fix Bug 28101 to [clang] Fix PR28101.Dec 9 2021, 9:08 AM
rZhBoYao edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.Dec 13 2021, 11:30 AM
clang/lib/Parse/ParseDecl.cpp
6485–6488 ↗(On Diff #392774)

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 ↗(On Diff #392774)

It seems incorrect that we're issuing the same diagnostic twice. Is this because of template instantiation, or some other reason?

16 ↗(On Diff #392774)

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.

rZhBoYao updated this revision to Diff 417461.Mar 22 2022, 6:46 PM
rZhBoYao retitled this revision from [clang] Fix PR28101 to [Clang] Fix PR28101.
rZhBoYao edited the summary of this revision. (Show Details)
rZhBoYao added a reviewer: cor3ntin.
rZhBoYao set the repository for this revision to rG LLVM Github Monorepo.

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.

Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 6:46 PM
rZhBoYao edited the summary of this revision. (Show Details)Mar 22 2022, 7:29 PM
rZhBoYao removed reviewers: rsmith, v.g.vassilev, rnk.
rZhBoYao updated this revision to Diff 417475.Mar 22 2022, 8:10 PM

Don't break template declarations.
re-clang-format

rZhBoYao updated this revision to Diff 417504.Mar 22 2022, 11:02 PM
rZhBoYao set the repository for this revision to rG LLVM Github Monorepo.

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.

erichkeane added inline comments.Mar 23 2022, 6:11 AM
clang/lib/Sema/SemaDeclCXX.cpp
3430

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
9

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.

14

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?

17

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?

rZhBoYao marked 3 inline comments as done.Mar 23 2022, 7:57 AM
rZhBoYao added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
3430

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
9

Instantiation of each of the 4 classes could crash on its own, so I thought I'd separate them into 4 invocations.

17

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.

erichkeane added inline comments.Mar 23 2022, 8:03 AM
clang/lib/Sema/SemaDeclCXX.cpp
3430

I'm not sure what the side effect of that is... Can you give it a shot?

clang/test/SemaCXX/PR28101.cpp
9

They shouldn't crash afterwards, right? So this shouldn't be a problem.

17

I think we definitely need to do so there, the instantiation trace not being here makes this a much less useful diagnostic.

rZhBoYao updated this revision to Diff 417632.Mar 23 2022, 8:30 AM
rZhBoYao marked 3 inline comments as done.

This passes check-clang-semacxx on my machine.

rZhBoYao marked 3 inline comments as done.Mar 23 2022, 8:31 AM

Please add a 'release notes'. Otherwise, LGTM as long as the bots are ok with it!

By 'release notes' do you mean a more detailed commit message?

rZhBoYao updated this revision to Diff 417648.Mar 23 2022, 9:05 AM
rZhBoYao edited the summary of this revision. (Show Details)
rZhBoYao set the repository for this revision to rG LLVM Github Monorepo.

Added an entry in release notes. Waiting for CI...

erichkeane accepted this revision.Mar 23 2022, 9:20 AM
This revision is now accepted and ready to land.Mar 23 2022, 9:20 AM
This revision was automatically updated to reflect the committed changes.