This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Handle PtrMemOps
ClosedPublic

Authored by tbaeder on Feb 15 2023, 11:57 PM.

Details

Summary

This is a result of the discussions in https://reviews.llvm.org/D141472 and https://reviews.llvm.org/D142630.

We need to consider virtual functions when calling function pointers as well.

Diff Detail

Event Timeline

tbaeder created this revision.Feb 15 2023, 11:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2023, 11:57 PM
tbaeder requested review of this revision.Feb 15 2023, 11:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2023, 11:57 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.May 17 2023, 4:57 AM
clang/lib/AST/Interp/Context.cpp
122

Do you have test coverage for the bound member case?

tbaeder added inline comments.Jun 1 2023, 2:58 AM
clang/lib/AST/Interp/Context.cpp
122

I was sure I had, I added this to make some test work... but now I removed it and nothing breaks, so I guess I don't.

tbaeder updated this revision to Diff 527346.Jun 1 2023, 2:58 AM
aaron.ballman added inline comments.Jul 19 2023, 12:47 PM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
213–214

Do we need similar changes for unary operators, pointer arithmetic operators, etc?

clang/lib/AST/Interp/Context.cpp
121

isFunctionType() in general?

aaron.ballman added inline comments.Jul 19 2023, 12:49 PM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
234

Should we be checking !T earlier (it's used within the Discard lambda with an unprotected dereference)?

tbaeder added inline comments.Jul 20 2023, 4:23 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
213–214

In what case would we need similar changes for those?

clang/lib/AST/Interp/Context.cpp
121

You mean additionally to those or as a replacement? isFunctionType() return false for function pointers it seems.

aaron.ballman added inline comments.Jul 20 2023, 7:05 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
213–214

I could use some more explanation for the changes here then; I thought this was to support overloaded binary operators. e.g.,

struct S {
  virtual constexpr int operator+(S s) const {
    return 12;
  }
};

struct T : S {
  constexpr int operator+(S s) const override {
    return 100;
  }
};

constexpr T t1, t2;
constexpr const S &s1 = t1, &s2 = t2;
constexpr const S s3, s4;
static_assert(s1 + s2 == 100);
static_assert(s3 + s4 == 12);
clang/lib/AST/Interp/Context.cpp
121

I mean instead of T->isFunctionProtoType(); a function without a prototype is still a kind of FnPtr, right? (moot for C++ constexpr, but I'm wondering why there's a requirement for a prototype or whether that's an oversight)

tbaeder marked an inline comment as done.Jul 20 2023, 7:57 AM
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
213–214

For this snippet from the tests:

constexpr S s;
constexpr decltype(&S::func) foo = &S::func;
constexpr int value = (s.*foo)();
static_assert(value == 1);

value is:

VarDecl 0x6210000a5180 <array.cpp:131:3, col:34> col:17 value 'const int' constexpr cinit
`-CXXMemberCallExpr 0x6210000a52d8 <col:25, col:34> 'int'
  `-ParenExpr 0x6210000a52b0 <col:25, col:32> '<bound member function type>'
    `-BinaryOperator 0x6210000a5288 <col:26, col:29> '<bound member function type>' '.*'
      |-DeclRefExpr 0x6210000a51f0 <col:26> 'const S':'const S' lvalue Var 0x6210000a0cf8 's' 'const S':'const S'
      `-ImplicitCastExpr 0x6210000a5268 <col:29> 'decltype(&S::func)':'int (S::*)() const' <LValueToRValue>
        `-DeclRefExpr 0x6210000a5240 <col:29> 'const decltype(&S::func)':'int (S::*const)() const' lvalue Var 0x6210000a0fc0 'foo' 'const decltype(&S::func)':'int (S::*const)() const' non_odr_use_constant

notice the BinaryOperator.

aaron.ballman added inline comments.Jul 20 2023, 10:53 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
213–214

Ah! Now I see why we needed this, thank you!

tbaeder updated this revision to Diff 542762.Jul 20 2023, 9:57 PM
tbaeder marked 2 inline comments as done.
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
234

Probably, but I changed this again a bit in https://reviews.llvm.org/D148925, so I'd like to do that change in a later commit.

aaron.ballman accepted this revision.Jul 25 2023, 7:18 AM

LG!

clang/lib/AST/Interp/ByteCodeExprGen.cpp
234

At some point, we need to stop having these interdependencies -- they make reviewing the code *much* harder because the changes are split across so many patches (some of which are in progress, some already accepted but not landed, etc). The new interpreter was in a weird state regarding how little we could test with it before, but we should strive to get to fully isolated changes ASAP (I think we're pretty close).

This revision is now accepted and ready to land.Jul 25 2023, 7:18 AM
This revision was landed with ongoing or failed builds.Jul 25 2023, 10:06 PM
This revision was automatically updated to reflect the committed changes.