This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Fix FFI-based tonic layer to use proper C++ static_cast<>()s#47644
Merged
mkustermann merged 1 commit intoflutter:mainfrom Nov 6, 2023
Merged
Fix FFI-based tonic layer to use proper C++ static_cast<>()s#47644mkustermann merged 1 commit intoflutter:mainfrom
mkustermann merged 1 commit intoflutter:mainfrom
Conversation
Flutter makes use of C++ multiple inheritence in it's C++ classes that
have corresponding Dart objects attached. An example is e.g.
class Canvas : public RefCountedDartWrappable<Canvas>, DisplayListOpFlags { }
template <typename T>
class RefCountedDartWrappable : public fml::RefCountedThreadSafe<T>,
public tonic::DartWrappable { }
When a C++ class has multiple base classes, the C++ compiler has the
liberty to decide the order in which they get layed out in memory. (It
doesn't follow declaration order, in fact it has a preference for having
classes with vtables before classes without vtables.)
Two casts to consider (with multiple inheritance in mind):
* upcasts: Those are done by C++ automatically but can modify the
actual address (i.e. pointer value)
* downcasts: Those have to be done with `static_cast<>()`, which can also
modify the address (i.e. pointer value) - using `reinterpret_cast<>()`
is incorrect (as it doesn't modify the address)
Now the Dart objects that refer to C++ objects have a native field in
them. The value of that field is a `tonic::DartWrappable*`. That means
whenever we convert between the C++ object pointer (e.g. `flutter::Canvas*`)
and the value of the native field (`tonic::Wrappable*`) the actual
address / pointer may need modification.
There were bugs in the C FFI based tonic layer: Pointer values coming
from Dart (via C FFI) are `dart::Wrappable*` and not pointers to
subtypes.
=> For methods we type the first parameter in tonic trampolines as
`dart::Wrappable*` and `static_cast<Class*>()` that value
=> Similarly for arguments, the parameter has to be typed as
`dart::Wrappable*` and `static_cast<Class*>()` that value
How did it ever work? Well it happens to be that the C++ compiler
decided to layout `tonic::DartWrappable` before
`fml::RefCountedThreadSafe`, making the `tonic::DartWrappable*` and the
`flutter::Canvas*` (and other classes) have the same pointer value.
=> This worked due to implementation choice of C++ compiler.
=> This breaks immediately if one decided to add another base class
(with virtual methods) to `RefCountedDartWrappable`
jason-simmons
approved these changes
Nov 3, 2023
Member
Author
|
@jason-simmons Thank you for taking a look! |
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 6, 2023
fluttermirroringbot
pushed a commit
to flutter/flutter
that referenced
this pull request
Nov 6, 2023
flutter/engine@555ffa1...b9b3269 2023-11-06 [email protected] Fix FFI-based tonic layer to use proper C++ static_cast<>()s (flutter/engine#47644) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Flutter makes use of C++ multiple inheritence in it's C++ classes that have corresponding Dart objects attached. An example is e.g.
When a C++ class has multiple base classes, the C++ compiler has the liberty to decide the order in which they get layed out in memory. (It doesn't necessarily follow declaration order, in fact it has a preference for having classes with
vtables before classes without vtables.)
Two casts to consider (with multiple inheritance in mind):
upcasts: Those are done by C++ automatically but can modify the actual address (i.e. pointer value)
downcasts: Those have to be done with
static_cast<>(), which can also modify the address (i.e. pointer value) - usingreinterpret_cast<>()is incorrect (as it doesn't modify the address)Now the Dart objects that refer to C++ objects have a native field in them. The value of that field is a
tonic::DartWrappable*. That means whenever we convert between the C++ object pointer (e.g.flutter::Canvas*) and the value of the native field (tonic::Wrappable*) the actual address / pointer may need modification.There were bugs in the C FFI based tonic layer: Pointer values coming from Dart (via C FFI) are
dart::Wrappable*and not pointers to subtypes.=> For methods we type the first parameter in tonic trampolines as
dart::Wrappable*andstatic_cast<Class*>()that value=> Similarly for arguments, the parameter has to be typed as
dart::Wrappable*andstatic_cast<Class*>()that valueHow did it ever work? Well it happens to be that the C++ compiler decided to layout
tonic::DartWrappablebeforefml::RefCountedThreadSafe, making thetonic::DartWrappable*and theflutter::Canvas*(and other classes) have the same pointer value.=> This worked due to implementation choice of C++ compiler.
=> This breaks immediately if one decided to add another base class (with virtual methods) to
RefCountedDartWrappable