This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Implement virtual function calls
ClosedPublic

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
1648

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
1715–1722

Might as well simplify these a little bit.

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

Add some more tests for virtual destructors.

tbaeder set the repository for this revision to rG LLVM Github Monorepo.Mar 23 2023, 1:44 AM
tbaeder added inline comments.May 12 2023, 2:22 AM
clang/lib/AST/Interp/Interp.h
1648

Just so I don't forget: The assignment to ThisPtr here is dead.

tbaeder updated this revision to Diff 527331.Jun 1 2023, 2:33 AM
tbaeder added inline comments.Jun 1 2023, 2:42 AM
clang/lib/AST/Interp/Interp.h
1648

Lies. I explained above that it's for the testcase. ThisPtr is a reference, so this replaces the this pointer on the stack.

tbaeder updated this revision to Diff 527336.Jun 1 2023, 2:44 AM
aaron.ballman added inline comments.Jun 9 2023, 11:05 AM
clang/lib/AST/Interp/Interp.h
1648

Please do add the example in a follow-up.

clang/test/AST/Interp/records.cpp
650

We should also have test cases for calling virtual functions from within a constructor and a destructor, as that has special semantics. e.g., https://godbolt.org/z/snaj1zfM5

tbaeder added inline comments.Jun 10 2023, 11:55 PM
clang/test/AST/Interp/records.cpp
650

That's broken right now of course. I'l add the test and adjust the expected output. I'll probably have to save a few bits for "things we're currently doing" (like evaluating a constructor), but in a later patch.

FWIW, I expanded your test a bit: https://godbolt.org/z/vq5xT3xvq and it only fails in clang - with a reference:

// CWG issue 1517: we're constructing a base class of the object described by
// 'This', so that object has not yet begun its period of construction and
// any polymorphic operation on it results in undefined behavior.
aaron.ballman accepted this revision.Jun 13 2023, 5:10 AM

LGTM

clang/test/AST/Interp/records.cpp
650

Handled in a follow-up is fine by me, thanks!

This revision is now accepted and ready to land.Jun 13 2023, 5:10 AM
This revision was automatically updated to reflect the committed changes.