This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Add function pointers
ClosedPublic

Authored by tbaeder on Jan 11 2023, 2:14 AM.

Details

Summary

This adds a FunctionPointer class as well as a new FnPtr primtype.

It also adds a CallPtr opcode that pops a FunctionPointer from the stack and calls its function.

Diff Detail

Event Timeline

tbaeder created this revision.Jan 11 2023, 2:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2023, 2:14 AM
tbaeder requested review of this revision.Jan 11 2023, 2:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2023, 2:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 488135.Jan 11 2023, 3:14 AM
tbaeder updated this revision to Diff 488139.Jan 11 2023, 3:23 AM
aaron.ballman added inline comments.Jan 18 2023, 9:56 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
138–139

Can we still reach this bit now?

tbaeder marked an inline comment as done.Jan 25 2023, 10:27 PM
tbaeder added inline comments.
clang/lib/AST/Interp/ByteCodeExprGen.cpp
138–139

Nope!

tbaeder updated this revision to Diff 492338.Jan 25 2023, 10:29 PM
tbaeder marked an inline comment as done.

Design question -- do you anticipate this class handling *all* function pointers, including pointer to member functions that might be virtual: https://godbolt.org/z/hT8fMY37n

Design question -- do you anticipate this class handling *all* function pointers, including pointer to member functions that might be virtual: https://godbolt.org/z/hT8fMY37n

I had to make a few adjustments but I have that example working locally now, of course no idea if that would work generally. Do you see any problem with the proposed FunctionPointer class?

According to https://clang.llvm.org/docs/ConstantInterpreter.html#pointers, there was a MemberPointer class that sounds related to this, but I don't know more about it. I guess, since you made func() virtual, that matters and becomes a bytecode-runtime decision instead of being known at compile-time, which seems to be the case in your example?

Design question -- do you anticipate this class handling *all* function pointers, including pointer to member functions that might be virtual: https://godbolt.org/z/hT8fMY37n

I had to make a few adjustments but I have that example working locally now, of course no idea if that would work generally. Do you see any problem with the proposed FunctionPointer class?

According to https://clang.llvm.org/docs/ConstantInterpreter.html#pointers, there was a MemberPointer class that sounds related to this, but I don't know more about it. I guess, since you made func() virtual, that matters and becomes a bytecode-runtime decision instead of being known at compile-time, which seems to be the case in your example?

Member pointers (for functions or for data) are weird in that they're not the typical pointer width. They're actually a pointer and between one-to-three other fields in a trenchcoat, depending on the circumstances. You generally need the function pointer, but you also may need various offsets (to this, to the vtable, etc). There's some more information about how it's done in MSVC (which is different from Itanium ABI, but we can do what we want for the constant expression interpreter): https://rants.vastheman.com/2021/09/21/msvc/

I don't think there's a problem with FunctionPointer per se, I'm more wondering are you planning to also add a MemberPointer type or are you planning to reuse FunctionPointer to handle function members (and presumably something else for data members)?

As for virtual functions in general, the standard has rules: http://eel.is/c++draft/expr.const#5.6 and http://eel.is/c++draft/expr.const#7

Member pointers (for functions or for data) are weird in that they're not the typical pointer width. They're actually a pointer and between one-to-three other fields in a trenchcoat, depending on the circumstances. You generally need the function pointer, but you also may need various offsets (to this, to the vtable, etc). There's some more information about how it's done in MSVC (which is different from Itanium ABI, but we can do what we want for the constant expression interpreter): https://rants.vastheman.com/2021/09/21/msvc/

I don't think there's a problem with FunctionPointer per se, I'm more wondering are you planning to also add a MemberPointer type or are you planning to reuse FunctionPointer to handle function members (and presumably something else for data members)?

As for virtual functions in general, the standard has rules: http://eel.is/c++draft/expr.const#5.6 and http://eel.is/c++draft/expr.const#7

I was thinking that the dynamicDispatch in https://godbolt.org/z/rf9Ks77Wo would be a good reproducer since the actual function to call is only known when calling dynamicDispatch(), but that example already works when doing a few changes to classify() the right types and adding a if (BO->isPtrMemOP()) { return visit(RHS); } to VisitBinaryOperator().

Member pointers (for functions or for data) are weird in that they're not the typical pointer width. They're actually a pointer and between one-to-three other fields in a trenchcoat, depending on the circumstances. You generally need the function pointer, but you also may need various offsets (to this, to the vtable, etc). There's some more information about how it's done in MSVC (which is different from Itanium ABI, but we can do what we want for the constant expression interpreter): https://rants.vastheman.com/2021/09/21/msvc/

I don't think there's a problem with FunctionPointer per se, I'm more wondering are you planning to also add a MemberPointer type or are you planning to reuse FunctionPointer to handle function members (and presumably something else for data members)?

As for virtual functions in general, the standard has rules: http://eel.is/c++draft/expr.const#5.6 and http://eel.is/c++draft/expr.const#7

I was thinking that the dynamicDispatch in https://godbolt.org/z/rf9Ks77Wo would be a good reproducer since the actual function to call is only known when calling dynamicDispatch(), but that example already works when doing a few changes to classify() the right types and adding a if (BO->isPtrMemOP()) { return visit(RHS); } to VisitBinaryOperator().

That is a reasonable test, but probably not sufficient as nothing is really testing the layout of those objects (the calls return a constant). How about a test like: https://godbolt.org/z/rhhhvxYxf where there are offsets to member variables involved?

tbaeder updated this revision to Diff 495786.Feb 8 2023, 3:13 AM

Member pointers (for functions or for data) are weird in that they're not the typical pointer width. They're actually a pointer and between one-to-three other fields in a trenchcoat, depending on the circumstances. You generally need the function pointer, but you also may need various offsets (to this, to the vtable, etc). There's some more information about how it's done in MSVC (which is different from Itanium ABI, but we can do what we want for the constant expression interpreter): https://rants.vastheman.com/2021/09/21/msvc/

I don't think there's a problem with FunctionPointer per se, I'm more wondering are you planning to also add a MemberPointer type or are you planning to reuse FunctionPointer to handle function members (and presumably something else for data members)?

As for virtual functions in general, the standard has rules: http://eel.is/c++draft/expr.const#5.6 and http://eel.is/c++draft/expr.const#7

I was thinking that the dynamicDispatch in https://godbolt.org/z/rf9Ks77Wo would be a good reproducer since the actual function to call is only known when calling dynamicDispatch(), but that example already works when doing a few changes to classify() the right types and adding a if (BO->isPtrMemOP()) { return visit(RHS); } to VisitBinaryOperator().

That is a reasonable test, but probably not sufficient as nothing is really testing the layout of those objects (the calls return a constant). How about a test like: https://godbolt.org/z/rhhhvxYxf where there are offsets to member variables involved?

Ah I get what you mean now.

The pointer we have on the stack is of type S but then we end up calling a function expecting a S2 pointer, and which one we're calling is not known at compile time.

I'm not sure what to do about this right now. I was wondering about restructuring pointers so all the metadata is before all the actual data, but that would be a large refactoring.

I'm not sure what to do about this right now. I was wondering about restructuring pointers so all the metadata is before all the actual data, but that would be a large refactoring.

For right now, we can move forward with basic function pointer support and we can revisit this when you get around to doing member function pointers.

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

Shouldn't this call emitNullPtr() in this case? e.g.,

struct S {
  int i;
  void (*fp)();
};

constexpr S s{ 12 };
static_assert(s.fp == nullptr);

I'm not sure what to do about this right now. I was wondering about restructuring pointers so all the metadata is before all the actual data, but that would be a large refactoring.

For right now, we can move forward with basic function pointer support and we can revisit this when you get around to doing member function pointers.

FWIW, I did that refactoring locally but it didn't really turn out to work like that. This example works now and there should be a test for it AFAIK.

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

Heh, that function is dead code right now, I keep it around because I think I might need it later. But your reproducer is broken anyway, I'll fix it, thanks.

tbaeder updated this revision to Diff 507965.Mar 23 2023, 11:20 PM
tbaeder set the repository for this revision to rG LLVM Github Monorepo.

Okay, fixed this by using visitZeroInitializer() so now it's not dead code anymore.

TODO: Test zero-initialization of floats.

tbaeder updated this revision to Diff 507966.Mar 23 2023, 11:22 PM
This revision is now accepted and ready to land.Mar 24 2023, 9:23 AM
This revision was landed with ongoing or failed builds.Mar 30 2023, 6:38 AM
This revision was automatically updated to reflect the committed changes.