Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App embedder Related to the embedder API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Embedder API] Lock the FlutterRect struct to guarantee ABI stability

4 participants