Page MenuHomePhabricator

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

[clang][Interp] Fix initializing base class members
ClosedPublic

Authored by tbaeder on Feb 6 2023, 11:23 PM.

Details

Summary
For the given test case, we were trying to initialize a member of C,
which doesn't have any. Get the proper base pointer instead and
initialize the members there.

Diff Detail

Event Timeline

tbaeder created this revision.Feb 6 2023, 11:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 11:23 PM
tbaeder requested review of this revision.Feb 6 2023, 11:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 11:23 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Mar 1 2023, 7:49 AM
clang/test/AST/Interp/records.cpp
258

We should test that the initialization actually happened.

260

Another test would be for invalid base initialization, like:

struct Base {
  int a;
};

struct Intermediate : Base {
  int b;
};

struct Final : Intermediate {
  int c;

  constexpr Final(int a, int b, int c) : c(c) {}
};

static_assert(Final{1, 2, 3}.c == 3, ""); // OK
static_assert(Final{1, 2, 3}.a == 0, ""); // Error, reads uninitialized member

or with multiple bases:

struct Base {
  int a;
};

struct Mixin  {
  int b;

  constexpr Mixin() = default;
  constexpr Mixin(int b) : b(b) {}
};

struct Final : Base, Mixin {
  int c;

  constexpr Final(int a, int b, int c) : Mixin(b), c(c) {}
  constexpr Final(int a, int b, int c, bool) : c(c) {}
};

static_assert(Final{1, 2, 3}.c == 3, ""); // OK
static_assert(Final{1, 2, 3}.b == 2, ""); // OK
static_assert(Final{1, 2, 3}.a == 0, ""); // Error, reads uninitialized member

or in a different form:

struct Base {
  int a;
};

struct Mixin  {
  int b;
};

struct Final : Base, Mixin {
  int c;

  constexpr Final(int a, int b, int c) : c(c) { this->b = b; }
  constexpr Final(int a, int b, int c, bool) : c(c) {}
};

static_assert(Final{1, 2, 3}.c == 3, ""); // OK
static_assert(Final{1, 2, 3}.b == 2, ""); // OK
static_assert(Final{1, 2, 3}.a == 0, ""); // Error, reads uninitialized member
tbaeder marked an inline comment as done.Mar 9 2023, 4:28 AM

The tests you proposed need https://reviews.llvm.org/D143480 first so we can cast up more than one level.

The tests you proposed need https://reviews.llvm.org/D143480 first so we can cast up more than one level.

Okay, how about we land that one first then come back to this one? I left some comments on the other review.

tbaeder updated this revision to Diff 504146.Mar 10 2023, 7:31 AM
tbaeder marked an inline comment as done.Mar 10 2023, 7:32 AM
tbaeder added inline comments.
clang/test/AST/Interp/records.cpp
271

These lines are added by https://reviews.llvm.org/D143480 now, but the tests are left commented since they require this patch as well.

This revision is now accepted and ready to land.Mar 10 2023, 11:15 AM
This revision was landed with ongoing or failed builds.Apr 3 2023, 4:51 AM
This revision was automatically updated to reflect the committed changes.