This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Diagnose uninitialized array record fields
ClosedPublic

Authored by tbaeder on Oct 27 2022, 2:39 AM.

Diff Detail

Event Timeline

tbaeder created this revision.Oct 27 2022, 2:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 2:39 AM
tbaeder requested review of this revision.Oct 27 2022, 2:39 AM
tbaeder set the repository for this revision to rG LLVM Github Monorepo.
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 2:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Oct 27 2022, 6:19 AM
clang/lib/AST/Interp/Interp.cpp
410–412

Some comments describing what SL does would be helpful, since it's a bit special.

428
431–436
464–465

Do element qualifiers matter for checking initialization? (I don't think they do, but double-checking to be sure there's not something special for atomics or something like that.)

464–466

cast<> already asserts if given a null pointer.

clang/test/AST/Interp/cxx20.cpp
119

This note is kind of confusing to me. At this location, it's not a subobject of type int that matters, it's A that's not fully initialized, and within A the note points out which field is not initialized.

I think this could get especially confusing in a case like:

class C {
public:
  A a;
  int b = 0;

  constexpr C() {}
}

because we'll talk about int not being initialized and it will look very much like it is.

tbaeder marked 3 inline comments as done.Oct 27 2022, 7:56 AM
tbaeder added inline comments.
clang/lib/AST/Interp/Interp.cpp
464–465

I did a quick test with _Atomic and that seems to work.

clang/test/AST/Interp/cxx20.cpp
119

I thought this might even be an improvement over the current situation. It's followed by the "subobject declared here" note as well. With the current interpreter, the output is:

array.cpp:95:16: error: constexpr variable 'c2' must be initialized by a constant expression
  constexpr C2 c2; // expected-error {{must be initialized by a constant expression}} \
               ^~
array.cpp:95:16: note: subobject of type 'int' is not initialized
array.cpp:88:18: note: subobject declared here
  struct A { int a; };
                 ^

And with the new one:

array.cpp:95:16: error: constexpr variable 'c2' must be initialized by a constant expression
  constexpr C2 c2; // expected-error {{must be initialized by a constant expression}} \
               ^~
array.cpp:93:15: note: subobject of type 'int' is not initialized
    constexpr C2() {} // expected-note {{subobject of type 'int' is not initialized}}
              ^
array.cpp:95:16: note: in call to 'C2()'
  constexpr C2 c2; // expected-error {{must be initialized by a constant expression}} \
               ^
array.cpp:88:18: note: subobject declared here
  struct A { int a; };
                 ^

But I dislike both very much, no idea why there's no "subobject declared here is not initialized" diagnostic and we need two notes instead :(

tbaeder updated this revision to Diff 471404.Oct 27 2022, 11:31 PM
aaron.ballman accepted this revision.Nov 4 2022, 11:49 AM

Aside from the request for a comment about SourceLocation SL, this LGTM (though we may want to improve the diagnostic behavior in the future).

clang/test/AST/Interp/cxx20.cpp
119

I think we can live with the behavior for now and improve the diagnostics once we've got more basic functionality in place.

This revision is now accepted and ready to land.Nov 4 2022, 11:49 AM
tbaeder updated this revision to Diff 473405.Nov 5 2022, 12:53 AM
tbaeder marked an inline comment as done.
tbaeder added inline comments.
clang/lib/AST/Interp/Interp.cpp
410–412

Renamed it to SubObjLoc instead of adding a comment, I think that's enough.

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