Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add vtable pointers to class layout #4407

Merged
merged 18 commits into from
Oct 16, 2024

Conversation

dwblaikie
Copy link
Contributor

A small step to virtual functions - adding vtable pointers to the layout, but not initializing or otherwise using them at this stage.

A few open design questions I'd love feedback on:

  • Is this the right/good enough SemIR representation for now? This patch adds a is_dynamic attribute to SemIR::Class and populates/flags it based on the flag of the base class, or if any virtual function is declared in the class (or, at least that's my intent). Some other options include:
    • Each Class could store a ClassId (or TypeId?) of the (possibly indirect, possibly self) base class that is the first one that is dynamic/has a vtable pointer
    • Could make the property narrower, like has vtable pointer and have it true only on the type that introduces the vtable - then derived classes would have to walk their base classes to check if they're the one that needs to define the vtable pointer or not
  • Should the vtable be the first element in the type? If there's a non-dynamic base type, we could have a layout that's {<non-dynamic base type>, vtable ptr, <derived members>}? Derived types would still be able to uniquely identify where their vtable pointer is just fine... - and the vtable pointer is, in a sense, a member of that intermediate type, so it does seem a bit strange to force it to the front - but I guess it's probably more efficient in some ways?

Open to any other suggestions/advice/thoughts on the direction, etc.

* Still needs to check the base class to see if there's already a vtable
* Not sure what type to point to - void* is probably about right, it
  can't really be pointer-to-correctly-sized-array, because derived
  classes might make the array larger (with new virtual functions)
@zygoloid
Copy link
Contributor

  • Is this the right/good enough SemIR representation for now? This patch adds a is_dynamic attribute to SemIR::Class and populates/flags it based on the flag of the base class, or if any virtual function is declared in the class (or, at least that's my intent).

I think storing this flag is reasonable; it seems like a property of the class that we will want to query relatively frequently. One other question we will frequently need to answer is "where is the vptr?" -- eg, when forming a virtual call or initializing an instance of the class -- so perhaps it'd make sense to also store some information about that, when we get to the point of wanting to access the vptr.

  • Should the vtable be the first element in the type? If there's a non-dynamic base type, we could have a layout that's {<non-dynamic base type>, vtable ptr, <derived members>}? Derived types would still be able to uniquely identify where their vtable pointer is just fine... - and the vtable pointer is, in a sense, a member of that intermediate type, so it does seem a bit strange to force it to the front - but I guess it's probably more efficient in some ways?

I think we've said that we want the derived <-> base conversion to be a no-op, which would force the vptr to not be at the start of the object in this case. However, that would force a significant ABI difference from C++, which might be motivation to reconsider and accept that derived <-> base conversions might add an offset. @josh11b may have further thoughts.

How hard would this be to revisit in future?

@dwblaikie
Copy link
Contributor Author

  • Is this the right/good enough SemIR representation for now? This patch adds a is_dynamic attribute to SemIR::Class and populates/flags it based on the flag of the base class, or if any virtual function is declared in the class (or, at least that's my intent).

I think storing this flag is reasonable; it seems like a property of the class that we will want to query relatively frequently. One other question we will frequently need to answer is "where is the vptr?" -- eg, when forming a virtual call or initializing an instance of the class -- so perhaps it'd make sense to also store some information about that, when we get to the point of wanting to access the vptr.

Fair enough - yeah, can add more/change how we're doing this later. But sounds like you're on board with this as a first pass.

  • Should the vtable be the first element in the type? If there's a non-dynamic base type, we could have a layout that's {<non-dynamic base type>, vtable ptr, <derived members>}? Derived types would still be able to uniquely identify where their vtable pointer is just fine... - and the vtable pointer is, in a sense, a member of that intermediate type, so it does seem a bit strange to force it to the front - but I guess it's probably more efficient in some ways?

I think we've said that we want the derived <-> base conversion to be a no-op, which would force the vptr to not be at the start of the object in this case. However, that would force a significant ABI difference from C++, which might be motivation to reconsider and accept that derived <-> base conversions might add an offset. @josh11b may have further thoughts.

Oh, fair point... I'll see about making the change now to put the vptr after the base (I expect this'll be easy enough, and can flip back/forth in this review if folks have opinions).

How hard would this be to revisit in future?

Seems easy enough - don't have a stable ABI, so I think we can move it around as we see fit.

Mostly I'm asking pretty short-sightedly about design questions, just whatever questions/answers you folks want answered now/set us in the direction you'd like to head.

zygoloid

This comment was marked as resolved.

@zygoloid

This comment was marked as resolved.

toolchain/sem_ir/name.cpp Outdated Show resolved Hide resolved
@dwblaikie
Copy link
Contributor Author

(Maybe also a fail_todo_something case for initializing a class with a vptr, to show that it fails -- I'd expect a "missing initializer for field" error for now.)

Yep, on the money. Test added.

common/array_stack.h Outdated Show resolved Hide resolved
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks great!

@zygoloid zygoloid added this pull request to the merge queue Oct 16, 2024
Merged via the queue into carbon-language:trunk with commit dfed743 Oct 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants