[go: nahoru, domu]

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

[Embedder API] Lock nested structs for ABI stability #40069

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

loic-sharma
Copy link
Member
@loic-sharma loic-sharma commented Mar 4, 2023

Part of flutter/flutter#121176
Fixes flutter/flutter#121347

Background

Imagine the following change:

Bad change...
typedef struct {
  /// The size of this struct. Must be sizeof(A).
  size_t struct_size;
  int one;
+ int two;
} A;

typedef struct {
  /// The size of this struct. Must be sizeof(B).
  size_t struct_size;
  A a;
  int three;
} B;

Adding a member to struct A would change the memory layout of struct B; this breaks ABI compatibility. The structs' struct_size does not protect against this ABI break. Instead, struct B should've nested struct A by pointer:

Good change...
typedef struct {
  /// The size of this struct. Must be sizeof(A).
  size_t struct_size;
  int one;
+ int two;
} A;

typedef struct {
  /// The size of this struct. Must be sizeof(B).
  size_t struct_size;
  A* a;
  int three;
} B;

This problem affects the following structs:

  1. FlutterTransformation - Would break FlutterSemanticsNode here
  2. FlutterRect - Would break FlutterSemanticsNode here. Also, FlutterDamage is affected by the "array of structs" problem here
  3. FlutterPoint would break FlutterLayer here
  4. FlutterDamage - Would break FlutterPresentInfo here

I did not include the following nested structs as adding members to them does not seem to introduce any ABI issues:

  1. FlutterSize
  2. FlutterUIntSize
  3. FlutterRoundedRect

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the embedder Related to the embedder API label Mar 4, 2023
@loic-sharma loic-sharma marked this pull request as ready for review March 4, 2023 00:57
@loic-sharma loic-sharma changed the title [Embedder API] Lock nested structs to guarantee ABI [Embedder API] Lock nested structs for ABI stability Mar 4, 2023
Copy link
Member
@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

lgtm! Nice.

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2023
@auto-submit auto-submit bot merged commit 7b9af00 into flutter:main Mar 9, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App embedder Related to the embedder API
Projects
None yet
4 participants