[BUG] Default release flags changed to O3 with unified cmake toolchain in r23-beta5

[fplk0]

Description

In ndk r21e, default release flags for armeabi-v7a have Os, and for all other archs it's O2.
In ndk r23 beta 5, those flags are set to O3 for all archs.
This can potentially cause a significant bin size regressions for many consumers.
simple-cmake.zip
With that dummy project (the error is intentional to view the compiler flags w/ ninja):
cmake .. -DCMAKE_TOOLCHAIN_FILE='~/Downloads/android-ndk-r23-beta5/build/cmake/android.toolchain.cmake' -DANDROID_ABI="armeabi-v7a with NEON" -DANDROID_NATIVE_API_LEVEL=21 -G Ninja -DCMAKE_BUILD_TYPE=Release; ninja
shows that O3 is being passed to compile flags,
cmake .. -DCMAKE_TOOLCHAIN_FILE='~/Downloads/android-ndk-r21e/build/cmake/android.toolchain.cmake' -DANDROID_ABI="armeabi-v7a with NEON" -DANDROID_NATIVE_API_LEVEL=21 -G Ninja -DCMAKE_BUILD_TYPE=Release; ninja
shows that Oz is being passed, as expected.
Similar with arm64-v8a ABI, with O2 in older toolchain being replaced by O3.
AFAIK overall O3 is considered non-beneficial for performance, and I'd argue that on older armeabi-v7a devices code size savings from Oz will yield (much?) better performance overall than O3.
Note that for cmake 3.20 and below it implicitly uses legacy toolchain file instead, and still applies the correct flags (Oz/O2); it'd be quite unexpected that bumping the cmake version will change the compilation flags that dramatically without any notice.

Environment Details

• NDK Version: r23-beta5
• Build system: cmake + android toolchain; cmake version 3.21.0-rc3
• Host OS: mac / probably unrelated
• NDK API level: 21

[DanAlbert]

We need to think about how we want the default behavior to work. Reducing behavioral differences as compared to the stock CMake behavior was one of the key reasons for unifying the two interfaces, so my initial reaction is that the fix here is to document that folks should exercise caution and inspect their compile command lines (as you have) when updating past CMake 3.20. The alternative would be akin to undoing the work. I don't think there's much in the way of an option 3, but I haven't given it much thought yet.

That's the answer for what we need to do in the NDK. On your end, I'd guess you're better off being explicit and telling CMake that you want MinSizeRel and not Release. I don't know if I've ever tried that though, so hopefully it knows to do things like use -Oz for Clang instead of -Os...

I don't think there's MinSizeRelWithDebInfo though, so not an answer for everyone (and I don't know how much control over the build type AGP users have).

[fplk0]

Thanks @DanAlbert, I understand your view, and, if the end goal is unification with cmake, then this all sounds reasonable.
The point to discuss is the fact that "Release" in cmake is like a de-facto default configuration(and afaik Gradle uses it by default too According to this, agp started to use RelWithDebInfo, which is never documented anywhere afaik), and I'm not sure, from android's infra standpoint, that size-bloated binary without debug info is a meaningful default for the android platform that most of the developers want to release to prod.
Yet another point to discuss is that RelWithDebInfo uses O2 while Release uses O3 :)
Regarding your point about MinSizeRel, no, it uses Os for clang instead of Oz :(
If we look in the other direction, on xcode/ios the default is Os for both architectures (and with debug info). I can understand why such a release configuration that cmake uses makes sense for desktop platforms, but I'm not sure if it's the best for mobile platforms.
Sorry, this message ended up very chaotic, but I just made several discoveries in the process of writing it.

So, to summarize, in my opinion:

• I think that at least both of those flags need to be very visible in the changelog as those are quite nasty side effects which some people might discover on accident after, say, anything bumps cmake on the builder.
• I think that it makes sense to have Os as default, but I don't think that this change is viable at this point. Those who care about performance or size will choose the flag they need or LTO; for those who don't, anything different from O0 is fine and the change might cause extra confusion.
• I don't think that O3 makes sense anywhere as default.
• Continued, I think that RelWithDebInfo and Release should only differ by the "-g" flag which they are not atm. It makes sense to make both of those O2 (or even Os).

[DanAlbert]

I think that at least both of those flags need to be very visible in the changelog as those are quite nasty side effects which some people might discover on accident after, say, anything bumps cmake on the builder.

Agreed. Done: https://android-review.googlesource.com/c/platform/ndk/+/1773073

[the other points]

More or less agreed here as well. The last point (minimizing differences between RelWithDebInfo and Release) isn't something we have control over since upstream CMake can introduce new differences whenever they want, but for the others I suspect we can teach CMake that the default release optimization for Android should be something other than what it is for desktops and servers.

The info about iOS's defaults is interesting as well. The old NDK behavior here was to default to -Oz for 32-bit Thumb (yes, ARM and thumb were actually different 😭), and -O2 for everything else. That disparity has always bothered me but I lack the data to know in which direction to unify them (or if they should be changed at all). I'm hesitant to decrease the default optimization level without being able to quantify the pros and cons...

I think the thing for us to do here that no one will argue with will be be to fix CMake to make MinSizeRel use -Oz for Clang, and probably to give CMake a MinSizeRelWithDebInfo build type.

Changing the default flags of Release is something I'm less confident we could do, and I don't think it's even possible to change the default CMAKE_BUILD_TYPE based on the platform. It's worth noting that AGP will already use RelWithDebInfo though, so if we get MinSizeRelWithDebInfo added we could use that instead in AGP.

[enh-google]

(yeah, thank you @Sfairat for the detailed and well thought-out commentary!)

[fplk0]

Thanks @DanAlbert,
Looked deeper into what Cmake does and yes, it's by design that RelWithDebInfo and Release are different (see the discussion here); so there's nothing else to do w/ defaults, and I'm closing the issue. Thanks for the discussion!

[DanAlbert]

Filed https://gitlab.kitware.com/cmake/cmake/-/issues/22458 to track the definite piece of follow up work. I'm pretty sure I have the patch to send, but need to work out how to run their tests...

[fplk0]

btw @DanAlbert I realized that there's another inconsistency I didn't point about - right now, non-legacy toolchain file has

# https://github.com/android/ndk/issues/133
if(CMAKE_ANDROID_ARCH_ABI MATCHES "^armeabi" AND NOT CMAKE_ANDROID_ARM_MODE)
  string(APPEND _ANDROID_NDK_INIT_CFLAGS_RELEASE " -Oz")
endif()

..Now, looking at this, I realized that I missed a very well actionable item for this.
What's happening is that later in flags.cmake we discard _ANDROID_NDK_INIT_CFLAGS_RELEASE via set(_ANDROID_NDK_INIT_CFLAGS_RELEASE).
There're two real ways around this to fix it in NDK - set the value as CACHE inside android.toolchain.cmake (probably more of a hack, it's not really CACHE var); remove the "clearing" in flags.cmake.
Alternatively, Oz can be just removed from android.toolchain.cmake given that unification with cmake is one of the goals. However, it'll be a reversal of #133.
Reopening the issue given that it IMO is an actionable concern.

[DanAlbert]

I think the fix is actually simpler than that. It looks like that block just needs to move to flags.cmake. I think that was just an oversight in the migration. I test that manually and it seems fine, but am writing a test to verify consistency between the various modes before submitting it.

[DanAlbert]

Yet another point to discuss is that RelWithDebInfo uses O2 while Release uses O3 :)

Ugh, and more annoying than that, with the old toolchain for arm Release uses -Oz and RelWithDebInfo uses -O2 :( I'll go figure out how long that has been the default behavior in AGP. If it's been this way for a long time I think this is a great time for us to lose this arch-specific behavior, but if it's recent we'll need to decide whether to fix it or leave it.

[DanAlbert]

The new behavior has only been shipping in AGP since the latest release, but I'm still leaning toward just admitting that we lost -Oz and removing some per-arch and android-only behavior from CMake. https://android-review.googlesource.com/c/platform/ndk/+/1826817 does that, but we're still discussing whether that's the right decision.

[DanAlbert]

Decided to go that route. It's submitted and will be in r23b.

[bommo1]

Is there a workaround besides using the legacy toolchain with an older CMake?

[DanAlbert]

Just set your flags the way you want them.

[fplk0]

@bommo1 @DanAlbert To clarify, there's one more caveat to that - you will need to set CMAKE_<LANG>_FLAGS_<BUILD_TYPE>, as if you just append -Odesired to CMAKE_C_FLAGS it will be overridden by the default one, as in CMake O2/O3 is set via CMAKE_<LANG>_FLAGS_<BUILD_TYPE>, which are appended after regular CMAKE_<LANG>_FLAGS.

[rprichard]

Fixed in r23b.