This is an archive of the discontinued LLVM Phabricator instance.

Mark lambda decl as invalid if a captured variable has an invalid type.
ClosedPublic

Authored by jgorbe on Nov 14 2018, 3:26 PM.

Details

Summary

This causes the compiler to crash when trying to compute a layout for
the lambda closure type (see included test).

Diff Detail

Event Timeline

jgorbe created this revision.Nov 14 2018, 3:26 PM

Thanks!

clang/lib/Sema/SemaExpr.cpp
14972

You'll need to use FieldType->getBaseElementTypeUnsafe() or similar here to handle the case where the field type is an array whose element type is a class. (That can happen in a lambda if you capture a local array variable by value.)

14974

For consistency with the class case, I think we should mark the field invalid in this case too.

jgorbe updated this revision to Diff 174118.Nov 14 2018, 4:25 PM

Fixed some issues pointed out in review comments:

  • call to getBaseElementType before checking type validity.
  • when the type is incomplete, mark not only the lambda closure type as invalid but also the field

Also, added a couple of checks to resemble more closely the code that checks user-declared fields in classes.

jgorbe marked 2 inline comments as done.Nov 14 2018, 4:27 PM
jgorbe added inline comments.
clang/lib/Sema/SemaExpr.cpp
14972

Done, I ended up using Context.getBaseElementType because the call to RequireCompleteType below required a QualType.

14974

Done!

Thanks! Some simplifications are possible here, but otherwise this looks good.

clang/lib/Sema/SemaExpr.cpp
14972

Hah, OK, now you've switched to using RequireCompleteType this has become redundant again (it'll walk to the base element type for you). =)

14978–14984

I believe the else case here is redundant: if RequireCompleteType returns false, the type is complete and there's nothing more you need to do.

clang/test/SemaCXX/lambda-invalid-capture.cpp
9–13

Can you also add a test for the "capturing an array of incomplete type" case please?

jgorbe updated this revision to Diff 174853.Nov 20 2018, 4:13 PM
jgorbe marked 2 inline comments as done.

Added a test for the "capturing an array of incomplete type" case. See also responses to inline comments below.

clang/lib/Sema/SemaExpr.cpp
14972

Removed call to getBaseElementType(), changed appearances of EltTy to FieldType instead.

14978–14984

If I just remove the else block, the test case included with the patch keeps crashing. Is there any way a Decl can be invalid even after RequireCompleteType returns false?

rsmith accepted this revision.Nov 20 2018, 9:51 PM

Looks good, thanks! (Though we generally prefer fewer test files with more test per file, because a lot of the overhead of running tests is the setup / teardown time, so can you merge the two added tests into a single file?)

clang/lib/Sema/SemaExpr.cpp
14978–14984

I think I was just wrong before :) Looks like RequireCompleteType doesn't check whether the type is invalid.

This revision is now accepted and ready to land.Nov 20 2018, 9:51 PM
jgorbe updated this revision to Diff 174944.Nov 21 2018, 9:34 AM

Folded the two test cases (capturing an invalid type and capturing an invalid array type) into a single file.

jgorbe closed this revision.Nov 21 2018, 10:05 AM