Skip to content

Conversation

@huanglizhuo
Copy link
Contributor

@huanglizhuo huanglizhuo commented Sep 22, 2024

Android 15 support 16k page sizes, it need enable 16kb elf alignment.

the current plugin_ffi template will have below error if run on android 15 16k emulator

Failed to load dynamic library 'libffigen_app.so': dlopen
failed: empty/missing
DT_HASH/DT_GNU_HASH in
"/data/app/~~Ixsgxu2mj5fKxP1cXpjV6Q==/com.example.ffigen_app_example-6d_efR__WGu
4dsF4tLIaHw==/lib/arm64/libffigen_app.so"
(new hash type from the future?)

Pre-launch Checklist

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

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@google-cla
Copy link

google-cla bot commented Sep 22, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 22, 2024
@andrewkolos
Copy link
Contributor

andrewkolos commented Sep 26, 2024

I don't know much of anything here, but I do wonder if this change could have other implications. Could this break running Flutter on older devices? (edit: lots of typos; good grief)

Also, even with this PR landed, would the issue reported here persist? Specifically, it would occur instead when we try to load the engine binaries (I don't know what the page size there is), no?

@reidbaker (cc @dcharkes)

@reidbaker
Copy link
Contributor

@zanderso I think you are the most knowledgeable flutter engineer when it comes to 16kb page sizes.

@dcharkes dcharkes changed the title fix: support android 15 16k page size for template package_ffi fix: support android 15 16k page size for template plugin_ffi Sep 27, 2024
@dcharkes
Copy link
Contributor

Reading up on this, requiring 16kb alignment of elf sections makes sense. I wouldn't worry about that breaking older devices, but it might add some kilobytes to the compiled binary.

In general, fixing compilation settings in the CMake file for plugin_ffi sgtm. Is this a NOOP for other operating systems? The CMake file is shared across all OSes. If not, we'll probably want to guard in the CMake file on target OS.

Also, we should have a test. Since Android 15 is not yet out, I suppose we don't have devices/simulators in our devicelab @reidbaker?

And note to self: We also need to fix this in --template package_ffi and/or package:native_toolchain_c.

@reidbaker
Copy link
Contributor

Reading up on this, requiring 16kb alignment of elf sections makes sense. I wouldn't worry about that breaking older devices, but it might add some kilobytes to the compiled binary.

In general, fixing compilation settings in the CMake file for plugin_ffi sgtm. Is this a NOOP for other operating systems? The CMake file is shared across all OSes. If not, we'll probably want to guard in the CMake file on target OS.

Also, we should have a test. Since Android 15 is not yet out, I suppose we don't have devices/simulators in our devicelab @reidbaker?

And note to self: We also need to fix this in --template package_ffi and/or package:native_toolchain_c.

Android 15 is out for developers and we have updated the api levels the engine/framework can call to api 35 (android 15). We dont have emulator support in CI yet because, last we checked, launching api 35 emulators was more unstable than our flake budget.

@dcharkes I am very open to feedback on how we should test. This class of bug stretches what I am comfortable reviewing.

@reidbaker
Copy link
Contributor

@dcharkes
Copy link
Contributor

Maybe we could get our hands on a phone that has an Android 15 beta with the 16kb memory pages and try it locally to see if things work yes or no. If we then verify what changes are needed (for both the engine and plugin_ffi and package_ffi) we could land these without enabling Android 15 tests yet on the CI and leave an open bug for ourselves to enable tests when we deem such infra stable enough. (This is not super satisfactory from a test coverage point of view, but enables us to preempt users running into this being completely not fixed/tested. I presume that once we make the necessary changes to the compiler settings in various places, we're unlikely to break those settings in the meantime.)

I feel fine reviewing an extra compiler flag in a build file after reading what it does, but I would want us to at least locally verify it for ourselves on an Android 15 with 16kb memory pages for ourselves.

@HosseinYousefi
Copy link
Member

I feel fine reviewing an extra compiler flag in a build file after reading what it does, but I would want us to at least locally verify it for ourselves on an Android 15 with 16kb memory pages for ourselves.

You can also use an emulator: https://developer.android.com/guide/practices/page-sizes#test

@reidbaker
Copy link
Contributor

Maybe we could get our hands on a phone that has an Android 15 beta with the 16kb memory pages and try it locally to see if things work yes or no. If we then verify what changes are needed (for both the engine and plugin_ffi and package_ffi) we could land these without enabling Android 15 tests yet on the CI and leave an open bug for ourselves to enable tests when we deem such infra stable enough. (This is not super satisfactory from a test coverage point of view, but enables us to preempt users running into this being completely not fixed/tested. I presume that once we make the necessary changes to the compiler settings in various places, we're unlikely to break those settings in the meantime.)

I feel fine reviewing an extra compiler flag in a build file after reading what it does, but I would want us to at least locally verify it for ourselves on an Android 15 with 16kb memory pages for ourselves.

I am happy to setup a 16kb page emulator and test this pr (if someone has example code ready to compile I would be grateful). I my question around testing was more about what does a unit test or non integration test look like for this class of change.

@dcharkes
Copy link
Contributor

For package_ffi: flutter test test/integration.shard/isolated/native_assets_test.dart.
For plugin_ffi: flutter create --template=plugin_ffi ... && cd xx/example/ && flutter run -d ... (I don't know which test does this by heart.) @huanglizhuo is this what you did to reproduce it?

Sometimes the emulators don't behave the same way as devices. So I'd only feel comfortable landing a fix if we can first reproduce the error on the emulator without the fix.

@reidbaker
Copy link
Contributor

lutter test test/integration.shard/isolated/native_assets_test.dart

Thank you I am working on getting an emulator setup now with 16kb page sizes.

@reidbaker
Copy link
Contributor

reidbaker commented Sep 27, 2024

running flutter test test/integration.shard/isolated/native_assets_test.dart from flutter/packages/flutter_tools/ on a tablet setup using the android 16kb page size instructions.
Confirmed by running adb shell getconf PAGE_SIZE returning 16384.
All tests pass without this change.

flutter test test/integration.shard/isolated/native_assets_test.dart
09:39 +22: All tests passed!

@reidbaker
Copy link
Contributor

Using the following commands I tested on several different emulators.

flutter create --template=plugin_ffi android_page_size 
cd android_page_size
flutter create -t plugin_ffi --platforms android android_ffi
cd android_ffi/example
flutter run

Flutter red screen with error text listed in this pr description

Flutter red screen with error text listed in this pr description

  • Android 35 tablet with 16kb page size (same emulator that had the integration tests pass)
  • sdk gphone16k arm64 (mobile) aka emulated pixel

Normal template app behavior
Normal template app behavior

  • Pixel 8 pro

Next step is to patch this pr and try again on the failing devices.

@reidbaker
Copy link
Contributor

Confirmed that after patching this pr and regenerating a test app all previously broken emulators show the example code correctly.

diff --git a/android_ffi/src/CMakeLists.txt b/android_ffi/src/CMakeLists.txt
index 8867fa5..fb58e86 100644
--- a/android_ffi/src/CMakeLists.txt
+++ b/android_ffi/src/CMakeLists.txt
@@ -15,3 +15,6 @@ set_target_properties(android_ffi PROPERTIES
 )

 target_compile_definitions(android_ffi PUBLIC DART_SHARED_LIB)
+
+# Support android 15 16k page size
+target_link_options(android_ffi PRIVATE "-Wl,-z,max-page-size=16384")
\ No newline at end of file
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 09a3dcc..1d776b3 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -15,3 +15,6 @@ set_target_properties(android_page_size PROPERTIES
 )

 target_compile_definitions(android_page_size PUBLIC DART_SHARED_LIB)
+
+# Support android 15 16k page size
+target_link_options(android_page_size PRIVATE "-Wl,-z,max-page-size=16384")
\ No newline at end of file

@dcharkes @HosseinYousefi Do you think a migrator needs to be written for apps that use ffi?
Also if we can find the test that verifies the plugin_ffi template launched successfully then I would say that this feature will be tested when the android team migrates to api 35 emulators.

Copy link
Contributor

@reidbaker reidbaker 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 for this pr

@reidbaker
Copy link
Contributor

flutter/packages/flutter_tools/test/integration.shard/android_plugin_example_app_build_test.dart I think is the test that checks plugin_ffi.

I think a simple test can be written for this by modifying test/commands.shard/permeable/create_test.dart to check for the new flags in the cmake file.

@reidbaker
Copy link
Contributor

@huanglizhuo I hope you don't mind I added a test for this change.

@reidbaker
Copy link
Contributor

@dcharkes holding this merge on your approval and thoughts on if this needs a migrator in addition to the template change.

@reidbaker
Copy link
Contributor

Related to #149836

auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 8, 2024
Manual roll Flutter from ead6b0d to 6bba08c (37 revisions)

Manual roll requested by [email protected]

flutter/flutter@ead6b0d...6bba08c

2024-10-01 [email protected]  Feat: Add opportunity to change CupertinoTextField suffix alignment (flutter/flutter#154601)
2024-10-01 [email protected] Roll Flutter Engine from da28db8ff41d to 3fdb546bf595 (2 revisions) (flutter/flutter#155993)
2024-10-01 [email protected] Roll Flutter Engine from 6b21b796cc94 to da28db8ff41d (1 revision) (flutter/flutter#155985)
2024-10-01 [email protected] Roll Flutter Engine from 259f56c6e91b to 6b21b796cc94 (1 revision) (flutter/flutter#155983)
2024-10-01 [email protected] Roll Flutter Engine from 6ee04ed763d9 to 259f56c6e91b (1 revision) (flutter/flutter#155981)
2024-10-01 [email protected] Roll Flutter Engine from bfb6dddb2b30 to 6ee04ed763d9 (3 revisions) (flutter/flutter#155978)
2024-10-01 [email protected] Roll Flutter Engine from e61bc853acb2 to bfb6dddb2b30 (8 revisions) (flutter/flutter#155967)
2024-10-01 [email protected] Disable flaky menu test  (flutter/flutter#155968)
2024-10-01 [email protected] Fix crash in Linux platform channel example. (flutter/flutter#155735)
2024-09-30 [email protected] Fix leak in input_decorator [prod-leak-fix] (flutter/flutter#155885)
2024-09-30 [email protected] Move platform specific text selection behavior out of styled TextField classes (flutter/flutter#155774)
2024-09-30 [email protected] Roll Flutter Engine from b466a0dd7834 to e61bc853acb2 (5 revisions) (flutter/flutter#155952)
2024-09-30 [email protected] `RenderParagraph`s `_SelectableFragment.boundingBoxes` should consider max line height (flutter/flutter#155892)
2024-09-30 [email protected] Roll Packages from 0321757 to 27c9853 (3 revisions) (flutter/flutter#155945)
2024-09-30 [email protected] Roll Flutter Engine from f4507e7a4beb to b466a0dd7834 (1 revision) (flutter/flutter#155944)
2024-09-30 [email protected] when `ResidentRunner.tryInitLogReader` fails, only log warning on Android (flutter/flutter#155800)
2024-09-30 [email protected] Move FlutterLogo from material to widget (flutter/flutter#155864)
2024-09-30 [email protected] fix: support android 15 16k page size for template plugin_ffi  (flutter/flutter#155508)
2024-09-30 [email protected] Roll Flutter Engine from daf126b38b8f to f4507e7a4beb (1 revision) (flutter/flutter#155932)
2024-09-30 [email protected] Roll Flutter Engine from 734205fbcd62 to daf126b38b8f (1 revision) (flutter/flutter#155929)
2024-09-30 [email protected] Roll Flutter Engine from 338f09c4ea72 to 734205fbcd62 (1 revision) (flutter/flutter#155923)
2024-09-30 [email protected] Roll Flutter Engine from 897f5caffe2d to 338f09c4ea72 (2 revisions) (flutter/flutter#155917)
2024-09-30 [email protected] Roll Flutter Engine from 569abc4044b8 to 897f5caffe2d (1 revision) (flutter/flutter#155912)
2024-09-29 [email protected] Roll Flutter Engine from c4784aa7eade to 569abc4044b8 (2 revisions) (flutter/flutter#155894)
2024-09-28 [email protected] Roll Flutter Engine from ff4541712df4 to c4784aa7eade (2 revisions) (flutter/flutter#155889)
2024-09-28 [email protected] Fixes column text width calculation in CupertinoDatePicker (flutter/flutter#151128)
2024-09-28 [email protected] Roll Flutter Engine from 380fd814448c to ff4541712df4 (1 revision) (flutter/flutter#155886)
2024-09-28 [email protected] Optimize `Overlay` sample to avoid overflow (flutter/flutter#155861)
2024-09-28 [email protected] Roll Flutter Engine from f3b11bcd9c37 to 380fd814448c (1 revision) (flutter/flutter#155876)
2024-09-28 [email protected] Roll Flutter Engine from 9c8e5cb226e4 to f3b11bcd9c37 (3 revisions) (flutter/flutter#155865)
2024-09-28 [email protected] Roll Flutter Engine from f9e4ed28f103 to 9c8e5cb226e4 (1 revision) (flutter/flutter#155857)
2024-09-27 [email protected] Roll Flutter Engine from f21f2b232b8a to f9e4ed28f103 (2 revisions) (flutter/flutter#155855)
2024-09-27 [email protected] Add  magnificationScale to CupertinoMagnifier for Zoom Effect (flutter/flutter#155276)
2024-09-27 [email protected] Fix typo on theme_data (flutter/flutter#155644)
2024-09-27 [email protected] Roll Flutter Engine from 7c603de2dca7 to f21f2b232b8a (6 revisions) (flutter/flutter#155843)
2024-09-27 [email protected] Turn the packages roller bot back on (flutter/flutter#155842)
2024-09-27 [email protected] Roll Packages from f38b780 to 0321757 (4 revisions) (flutter/flutter#155832)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

...
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 tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants