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

[BUG] NDK-r25 CMAKE_BUILD_TYPE=MinSizeRel(-Os) CMAKE_BUILD_TYPE=Release(null), NDK-r22b get MinSizeRel(-Os) Release(-O2) #1740

Closed
DaydreamCoding opened this issue Jul 28, 2022 · 4 comments
Assignees
Labels

Comments

@DaydreamCoding
Copy link

DaydreamCoding commented Jul 28, 2022

Description

The NDK r25 compile with CMAKE_BUILD_TYPE=Release missing default compiler optimizations options.

R22b

  • MinSizeRel -Os -DNDEBUG -fPIE
  • Release -O2 -DNDEBUG -fPIE

R25, R24, R23c

  • MinSizeRel -Os -DNDEBUG -fPIE
  • Release -DNDEBUG -fPIE

Example: project https://github.com/DaydreamCoding/neon-intrinsics-test/tree/1c123bc9391eec4f9c8305d17f9d24e69d70f237

  • R22b CMAKE_BUILD_TYPE=Release
    export ANDROID_NDK=$ANDROID_NDK_R22b
    ./build/android_build_Release.sh
    cat build/libsample/build/android/arm64-v8a/test/CMakeFiles/test.dir/flags.make
    CXX_FLAGS = -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -O2 -DNDEBUG -fPIE

  • R22b CMAKE_BUILD_TYPE=MinSizeRel
    export ANDROID_NDK=$ANDROID_NDK_R22b
    ./build/android_build_MinSizeRel.sh
    cat build/libsample/build/android/arm64-v8a/test/CMakeFiles/test.dir/flags.make
    CXX_FLAGS = -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -Os -DNDEBUG -fPIE

  • R25 CMAKE_BUILD_TYPE=Release
    export ANDROID_NDK=$ANDROID_NDK_R25
    ./build/android_build_Release.sh
    cat build/libsample/build/android/arm64-v8a/test/CMakeFiles/test.dir/flags.make
    CXX_FLAGS = -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -DNDEBUG -fPIE

  • R25 CMAKE_BUILD_TYPE=MinSizeRel
    export ANDROID_NDK=$ANDROID_NDK_R25
    ./build/android_build_MinSizeRel.sh
    cat build/libsample/build/android/arm64-v8a/test/CMakeFiles/test.dir/flags.make
    CXX_FLAGS = -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -Os -DNDEBUG -fPIE

This is very confusing behavior, R22b Release and MinSizeRel both have default compilation optimizations, R25 MinSizeRel does and Release does not

cmake --version
cmake version 3.23.2

CMake suite maintained and supported by Kitware (kitware.com/cmake).

Associated issue
#1693
#1607

Affected versions

r25

Canary version

No response

Host OS

Mac

Host OS version

macOS 12.4

Affected ABIs

arm64-v8a

Build system

CMake

Other build system

No response

minSdkVersion

21

Device API level

No response

@DanAlbert
Copy link
Member

Here are some relevant discussions for r23b (probably not useful until you've read the rest of this post though):

#1536
https://android-review.googlesource.com/c/platform/ndk/+/1826817
https://github.com/android/ndk/wiki/Changelog-r23#r23b

The changelog mentions only -Oz, not -O2, which was a gap in the documentation. If I use your build script with r23b, I see -O3 rather than nothing or -O2. That was intentional. Like the bug says, the bandaid was already off, so we took the opportunity to align with CMake's upstream behavior. CMake uses -O3 for Release, -Os for MinSizeRel, and -O2 for RelWithDebInfo.

This has actually been broken since r23b, but was asymptomatic in the default configuration because it's only present in the legacy toolchain file, and even then only in a CMake configuration that AGP does not use. If I force r23b to use the legacy toolchain, I get the same behavior there. It seems that Release (and only Release) is missing optimization flags. MinSizeRel and RelWithDebInfo are getting -Os and -O2 respectively as expected.

I'll upload a fix shortly, since as best as I can make out this was not intentional. It's not strictly a regression either (the same configuration behaves the same way), but since the default behavior has changed to include this bug, I think this is worth getting into r25b (no current release plans for that, but I'll start kicking those tires since we also have some toolchain fixes we want to ship).

@DanAlbert DanAlbert self-assigned this Jul 28, 2022
@DanAlbert
Copy link
Member

https://android-review.googlesource.com/c/platform/ndk/+/2168845

Thanks for the detailed bug report and repro instructions btw. It still took a lot of head scratching to figure out what broke and when, but it would have been a lot harder without that :)

@DanAlbert
Copy link
Member

https://ci.android.com/builds/branches/aosp-master-ndk/grid?head=8881098&tail=8881098 has the fix if you want to double check. I added tests to cover the missing configurations, but I haven't retested your repro case against the fixed NDK.

@DaydreamCoding
Copy link
Author

Thanks for your reply and fix. with r25b we can distribute this version on a large scale without having to change a lot of source code build parameters. this will be the most reliable version since the r22b.

edgchen1 added a commit to microsoft/onnxruntime that referenced this issue Aug 1, 2022
With recent versions of NDK (since 23), the `-O` optimization level compile flag is not being passed when building in the "Release" configuration.
More details here: android/ndk#1740

Our "Release" Android builds have been built without the optimization flag since we upgraded from NDK 21.

This change is a workaround to manually add `-O3` for "Release" Android builds.
RandySheriffH pushed a commit to microsoft/onnxruntime that referenced this issue Aug 2, 2022
With recent versions of NDK (since 23), the `-O` optimization level compile flag is not being passed when building in the "Release" configuration.
More details here: android/ndk#1740

Our "Release" Android builds have been built without the optimization flag since we upgraded from NDK 21.

This change is a workaround to manually add `-O3` for "Release" Android builds.
RandySheriffH added a commit to microsoft/onnxruntime that referenced this issue Aug 3, 2022
* update package version

* Prevent unbounded growth of command allocator memory (#12114)

Prevent unbounded growth of command allocator memory

* Update supported ops md for NNAPI/CoreML EP (#12245)

* update supported ops md

* address pr comments

* address pr comments

* wording

* Change native folder name for java macos arm64 (#12335)

* Bump async from 2.6.3 to 2.6.4 in /js/react_native/e2e (#11280)

Bumps [async](https://github.com/caolan/async) from 2.6.3 to 2.6.4.
- [Release notes](https://github.com/caolan/async/releases)
- [Changelog](https://github.com/caolan/async/blob/v2.6.4/CHANGELOG.md)
- [Commits](caolan/async@v2.6.3...v2.6.4)

---
updated-dependencies:
- dependency-name: async
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* [js/rn] upgrade dependencies for e2e test (#11863)

* [js/rn] upgrade dependencies for e2e test

* use JDK11 only for gradle

* expand variable

* [js/rn] upgrade package react-native@^0.69.1 (#12155)

* [js/rn] upgrade package react-native@^0.69.1

* upgrade compile sdk to v31

* update ios version requirement

* update pod path for onnxruntime-react-native

* add missing build_java in Android testing stage. (#12187)

add missing build_java in testing

* Use specific Android NDK version in CI builds. (#12350)

Current builds use a NDK version that happens to be on the build machine. The build machine environment may change in ways that are outside of our control.
This change installs a specific version of NDK (the current LTS version 25.0.8775105) and uses it.

* Remove preview keyword from DirectML pacakge (#12368)

Remove preview keyword

Co-authored-by: Sumit Agarwal <sumitagarwal@microsoft.com>

* Scope CreateFileMapping2 to valid API partitions (#12374)

* Fix TRT custom op issue (#12283)

* Pass schema registry on CreateModel.

* Fix ORT_MINIMAL_BUILD.

* Fix build issue.

* Manually add optimization flag for Android Release builds. (#12390)

With recent versions of NDK (since 23), the `-O` optimization level compile flag is not being passed when building in the "Release" configuration.
More details here: android/ndk#1740

Our "Release" Android builds have been built without the optimization flag since we upgraded from NDK 21.

This change is a workaround to manually add `-O3` for "Release" Android builds.

* resolve conflicts in tensorRT related changes

* Enable support of multi-level nested control flow ops model for TRT EP (#12147)

* Make multiple-level nested control flow op model work

* find correct input index

* find correct input index (cont.)

* enable nested layer unit tests for TRT EP

* add comment

* add Scan op to current workaround support of control flow op

Co-authored-by: Jeff Bloomfield <38966965+jeffbloo@users.noreply.github.com>
Co-authored-by: Rachel Guo <35738743+YUNQIUGUO@users.noreply.github.com>
Co-authored-by: Changming Sun <chasun@microsoft.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Yulong Wang <7679871+fs-eire@users.noreply.github.com>
Co-authored-by: Yi Zhang <zhanyi@microsoft.com>
Co-authored-by: Edward Chen <18449977+edgchen1@users.noreply.github.com>
Co-authored-by: sumitsays <sumitagarwal330@gmail.com>
Co-authored-by: Sumit Agarwal <sumitagarwal@microsoft.com>
Co-authored-by: Justin Stoecker <justoeck@microsoft.com>
Co-authored-by: Yateng Hong <yatengh@microsoft.com>
Co-authored-by: Chi Lo <54722500+chilo-ms@users.noreply.github.com>
osspop pushed a commit to osspop/android-ndk that referenced this issue Jan 17, 2023
Bug: android/ndk#1740
Test: expanded existing test to cover more configurations
Change-Id: If1986d549edc19bbf21c6b94d2c84af2b1c5dcd1
osspop pushed a commit to osspop/android-ndk that referenced this issue Jan 17, 2023
Bug: android/ndk#1740
Test: expanded existing test to cover more configurations
Change-Id: If1986d549edc19bbf21c6b94d2c84af2b1c5dcd1
(cherry picked from commit 4ad4cb4)
Merged-In: If1986d549edc19bbf21c6b94d2c84af2b1c5dcd1
cortinico added a commit to cortinico/react-native that referenced this issue Jun 5, 2023
Summary:
I'm backporting this comming `60a452b4853dc5651c465867344904dd6fc86703`
from the release branch of 0.72-stable to main which fixes this issue
in the Android NDK:
android/ndk#1740 (comment)

Changelog:
[Internal] [Changed] - Backport Hermes release fix on `main`

Reviewed By: mdvacca

Differential Revision: D46398346

fbshipit-source-id: 477f4da5f1137574b37f6189572f583007a2624f
cortinico added a commit to cortinico/react-native that referenced this issue Jun 5, 2023
Summary:
Pull Request resolved: facebook#37685

I'm backporting this comming `60a452b4853dc5651c465867344904dd6fc86703`
from the release branch of 0.72-stable to main which fixes this issue
in the Android NDK:
android/ndk#1740 (comment)

Changelog:
[Internal] [Changed] - Backport Hermes release fix on `main`

Reviewed By: mdvacca

Differential Revision: D46398346

fbshipit-source-id: 170991a8838d53e77b4b11efceab64f706389600
cortinico added a commit to cortinico/react-native that referenced this issue Jun 5, 2023
Summary:
Pull Request resolved: facebook#37685

I'm backporting this comming `60a452b4853dc5651c465867344904dd6fc86703`
from the release branch of 0.72-stable to main which fixes this issue
in the Android NDK:
android/ndk#1740 (comment)

Changelog:
[Internal] [Changed] - Backport Hermes release fix on `main`

Reviewed By: mdvacca

Differential Revision: D46398346

fbshipit-source-id: 7071c3324ade74cbad9b5646af2442f0e462cfeb
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jun 5, 2023
Summary:
Pull Request resolved: #37685

I'm backporting this comming `60a452b4853dc5651c465867344904dd6fc86703`
from the release branch of 0.72-stable to main which fixes this issue
in the Android NDK:
android/ndk#1740 (comment)

Changelog:
[Internal] [Changed] - Backport Hermes release fix on `main`

Reviewed By: mdvacca

Differential Revision: D46398346

fbshipit-source-id: 593ca9523cb223e7c2d9fe6cccc1abb3ed331203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Merged
Development

No branches or pull requests

2 participants