This is an archive of the discontinued LLVM Phabricator instance.

[clang] Make CXXRecrdDecl invalid if it contains any undeduced fields.
ClosedPublic

Authored by adamcz on Jul 6 2021, 6:35 AM.

Details

Summary

Undeduced fields in a valid record cause crashes when trying to obtain
the size and/or layout of the record.

Diff Detail

Event Timeline

adamcz requested review of this revision.Jul 6 2021, 6:35 AM
adamcz created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2021, 6:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hokein added inline comments.Jul 7 2021, 12:19 AM
clang/test/SemaCXX/cxx11-crashes.cpp
117

I think the root cause is that this incomplete var declaration is marked valid.

Looks like there are a few inconsistencies in clang:

struct Bar {
  static constexpr auto A1; // case1: invalid
  static constexpr auto A2 = ; // case2: valid
};

// static var decl in global context
static constexpr auto A3; // case3: invalid
static constexpr auto A4 = ; // case4: invalid

so it looks like marking case2 valid is a bug, I think we should invalidate it.

adamcz updated this revision to Diff 356925.Jul 7 2021, 5:35 AM

changed to marking field type as invalid

adamcz added inline comments.Jul 7 2021, 5:37 AM
clang/test/SemaCXX/cxx11-crashes.cpp
117

I updated the change to mark a FieldDecl with undeduced type as invalid, thus making the record invalid too. Also added a test using -ast-dump to verify that both field and record are invalid, but kept the crash test too.

hokein added inline comments.Jul 7 2021, 6:00 AM
clang/test/SemaCXX/cxx11-crashes.cpp
117

sorry, my comment was not clear enough -- if I understand your new change correctly, the change makes the field decl Foo<decltype(A)>::type B; but not the decl A.

The problem here is static constexpr auto A = ;, this declaration (var, not field) is valid, which causes the field decl below Foo<decltype(A)>::type B; valid. If we invalidate the Decl A, then the filed decl B should be invalidated automatically.

leaving A valid is problematic -- we can't have a valid decl with an undeduced auto type in clang. the following case would lead another crash:

struct Bar {
  static constexpr auto A = ;
};
constexpr int s = sizeof(Bar::A);

So the solution is to make Decl A invalid.

adamcz updated this revision to Diff 357511.Jul 9 2021, 8:02 AM

Fix a more generic case of invalid static initializer

adamcz added a comment.Jul 9 2021, 8:03 AM

OK, I think we've got it now :-)

I kept the original crash test in the change, but I'm not sure if it's appropriate anymore. Let me know if you think I should remove it.

OK, I think we've got it now :-)

I kept the original crash test in the change, but I'm not sure if it's appropriate anymore. Let me know if you think I should remove it.

Thanks, that sounds good to me, the original crash test is valuable, worth to keep it.

clang/lib/Parse/ParseDeclCXX.cpp
2987 ↗(On Diff #357511)

it seems to me calling Sema::ActOnInitializerError(ThisDecl) is a better fit, rather than invalidating the decl once the initializer is failed to parse, for example static const int Member = ; we want to keep the decl valid.

adamcz updated this revision to Diff 363495.Aug 2 2021, 9:32 AM

ActOnInializerError instead of SetInvalidDecl

clang/lib/Parse/ParseDeclCXX.cpp
2987 ↗(On Diff #357511)

Amusingly, this change led me to discover https://bugs.llvm.org/show_bug.cgi?id=36064, which seems like the same issue, only solved for a very specific example. I had to update the test for that, since now we solve it more generically and thus error messages are different.

hokein accepted this revision.Aug 2 2021, 12:03 PM

Thanks!

This revision is now accepted and ready to land.Aug 2 2021, 12:03 PM