Page MenuHomePhabricator

[clang][Interp] Implement virtual function calls
Needs ReviewPublic

Authored by tbaeder on Jan 26 2023, 7:32 AM.

Details

Summary

@aaron.ballman Not sure if this (the added early return) is all you meant when you talked about virtual destructors.

I also wasn't sure about the added getOverridingFunction() - I think isDerivedFrom(..., paths) is what I should be using, but that seemed excessively hard to use.

Diff Detail

Event Timeline

tbaeder created this revision.Jan 26 2023, 7:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 7:32 AM
tbaeder requested review of this revision.Jan 26 2023, 7:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 7:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 495059.Feb 6 2023, 3:08 AM

Hmm, this doesn't work if the callee is somewhere in between the path from the dynamic to the static type, e.g.

struct A {
  virtual constexpr int foo() const { return 1; }
};

struct B : A {
  virtual constexpr int foo() const { return A::foo(); }
};

constexpr B a;
static_assert(a.foo() == 1);

For the A::foo() call, the type of the This pointer is B, so we will just select the B::foo() to call.

Hmm, this doesn't work if the callee is somewhere in between the path from the dynamic to the static type, e.g.

struct A {
  virtual constexpr int foo() const { return 1; }
};

struct B : A {
  virtual constexpr int foo() const { return A::foo(); }
};

constexpr B a;
static_assert(a.foo() == 1);

For the A::foo() call, the type of the This pointer is B, so we will just select the B::foo() to call.

Ah, it sounds like we're not handling qualified calls properly. That makes me think of a few more test cases:

struct A {
  virtual constexpr int foo() const { return 1; }
};

struct B : A {
  // Same behavior as your test case but with more baffling syntax. :-)
  virtual constexpr int foo() const { return this->A::foo(); }
};

constexpr B a;
static_assert(a.foo() == 1);

or with shadowed member variables (not related to virtual functions directly, but is related to qualified lookup):

struct A {
  virtual constexpr int foo() const { return 0; }
  int Val = 1;
};

struct B : A {
  virtual constexpr int foo() const { return A::Val; }
  int Val = 2;
};

constexpr B a;
static_assert(a.foo() == 1);

note, the rules are different for constructors and destructors than they are for other functions, so similar test cases involving those would be a good idea as well (though perhaps in a separate patch).

tbaeder updated this revision to Diff 495789.Feb 8 2023, 3:20 AM
tbaeder set the repository for this revision to rG LLVM Github Monorepo.

Fixed all the new test cases by adding a new CallVirt opcode and only using that if appropriate.

tbaeder updated this revision to Diff 497276.Feb 14 2023, 4:48 AM
tbaeder added inline comments.Feb 14 2023, 4:51 AM
clang/lib/AST/Interp/Interp.h
1560

I think this test case was from the function pointer review, but this fixes:

struct S {
  virtual constexpr int func() const { return 1; }
};

struct Middle : S {
  constexpr Middle(int i) : i(i) {}
  int i;
};

struct Other {
  constexpr Other(int k) : k(k) {}
  int k;
};

struct S2 : Middle, Other {
  int j;
  constexpr S2(int i, int j, int k) : Middle(i), Other(k), j(j) {}
  virtual constexpr int func() const { return i + j + k  + S::func(); }
};


constexpr S s;
constexpr decltype(&S::func) foo = &S::func;
constexpr S2 s2(1, 2, 3);
constexpr int value3 = (s2.*foo)();

even I am confused by all of this now, so the comment might not be the clearest :)

(I did not add that test case since it needs other changes in`classify() and VisitBinaryOperator()`, but can do that in a follow-up commit.)

(Still thinking about the implementation work here, mostly just drive-by comments at the moment. My gut instinct is that we've not got sufficient test coverage here, but that might be partially because of other parts of the interpreter not being ready yet.)

clang/lib/AST/Interp/ByteCodeExprGen.cpp
1687–1694

Might as well simplify these a little bit.

clang/lib/AST/Interp/Interp.h
1546–1547
tbaeder updated this revision to Diff 501506.Wed, Mar 1, 7:26 AM
tbaeder marked an inline comment as done.
tbaeder marked an inline comment as done.
tbaeder updated this revision to Diff 507648.Thu, Mar 23, 1:44 AM

Add some more tests for virtual destructors.

tbaeder set the repository for this revision to rG LLVM Github Monorepo.Thu, Mar 23, 1:44 AM