This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Check that constructor calls initialize all record fields
ClosedPublic

Authored by tbaeder on Oct 25 2022, 9:08 AM.

Details

Summary

We need to do this, even for global variables (it seems).

The diagnostic output is slightly different than what we get from the current interpreter, but I'm not sure if it's worse or not or if that needs to be changed.

Diff Detail

Event Timeline

tbaeder created this revision.Oct 25 2022, 9:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2022, 9:08 AM
tbaeder requested review of this revision.Oct 25 2022, 9:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2022, 9:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 470750.Oct 26 2022, 2:16 AM

It's worth noting that there are core issues in this area: https://wg21.link/cwg2558 and https://wg21.link/cwg2536 and they're not yet resolved.

clang/test/AST/Interp/records.cpp
275–276

Comment is now stale

tbaeder updated this revision to Diff 471161.Oct 27 2022, 7:23 AM
tbaeder marked an inline comment as done.
shafik added inline comments.Oct 27 2022, 1:50 PM
clang/lib/AST/Interp/Interp.cpp
1

Probably a separate NFC commit

465

Should we also check isLive() or should that just turn into a diagnostic?

tbaeder marked an inline comment as done.Oct 28 2022, 12:35 AM
tbaeder added inline comments.Oct 28 2022, 6:01 AM
clang/lib/AST/Interp/Interp.cpp
465

Since cc79ddb52c310be50d2ed0e0307b695cc7c142ce, we use CheckInvoke() for the instance pointer, which already calls isLive().

aaron.ballman added inline comments.Nov 30 2022, 10:07 AM
clang/lib/AST/Interp/Interp.cpp
447

Do we have to worry about static data members, or are those excluded in fields()?

clang/lib/AST/Interp/Interp.h
1346–1347

Should we assert that ThisPtr is valid?

1365–1368
tbaeder marked 3 inline comments as done.Dec 1 2022, 2:16 AM
tbaeder added inline comments.
clang/lib/AST/Interp/Interp.cpp
447

They aren't in RecordDecl::fields(), so aren't in Record::fields() either.

clang/lib/AST/Interp/Interp.h
1346–1347

We could only assert !isZero() as far as I understand, but that's part of what CheckInvoke does.

tbaeder updated this revision to Diff 479209.Dec 1 2022, 2:18 AM
tbaeder marked 2 inline comments as done.
aaron.ballman accepted this revision.Dec 1 2022, 8:25 AM

LGTM!

clang/lib/AST/Interp/Interp.h
1346–1347

Ah, then no need.

This revision is now accepted and ready to land.Dec 1 2022, 8:25 AM

Oh I think this patch is doing too much. The checking should only occur for global variables.

shafik accepted this revision.Dec 21 2022, 6:14 PM

LGTM

clang/lib/AST/Interp/Interp.cpp
1

One more time :-)

This revision was landed with ongoing or failed builds.Jan 19 2023, 12:42 AM
This revision was automatically updated to reflect the committed changes.