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] CMake toolchain file sets CMAKE_ANDROID_EXCEPTION instead of CMAKE_ANDROID_EXCEPTIONS #1618

Closed
craigscott-crascit opened this issue Dec 4, 2021 · 5 comments
Assignees
Labels
Projects

Comments

@craigscott-crascit
Copy link

craigscott-crascit commented Dec 4, 2021

Description

The NDK's CMake toolchain file sets a variable named CMAKE_ANDROID_EXCEPTION, but it should be CMAKE_ANDROID_EXCEPTIONS (note the plural rather than singular). This variable will therefore have no effect when CMake is deciding whether to add -fexceptions or -fno-exceptions to the CMAKE_<LANG>_FLAGS_INIT flags, resulting in the flag being determined purely based on the selected STL type. This only affects build arrangements where ANDROID_CPP_FEATURES is set (I'm not sure what situations do that).

Environment Details

Found while working on macOS on an M1 machine. NDK 23.1.7779620 was installed via Android Studio, but the toolchain file from a standalone NDK package (r24-beta1) still has the same problem.

@DanAlbert DanAlbert added this to Triaged in r24 via automation Dec 7, 2021
@DanAlbert DanAlbert added this to Triaged in r23c via automation Dec 7, 2021
@DanAlbert
Copy link
Member

While the fix is trivial, we're having trouble putting together a regression test. What's the behavior difference you're seeing? Exceptions are on by default, and if I pass -DANDROID_CPP_FEATURES=no-exceptions it does actually disable them (maybe CMake understands both options?)

$ cat ./foo.cpp
int main(int argc, char** argv) {
  try {
    throw 42;
  } catch (const int& ex) {
    return ex;
  }
  return 0;
}
$ cat ./CMakeLists.txt
cmake_minimum_required(VERSION 3.6.0)

add_executable(foo foo.cpp)
$ cat ./build.sh
#!/bin/sh
if [ $# -lt 1 ]; then
  >&2 echo "usage: ./gen.sh NDK_PATH"
  exit 1
fi

set -e

NDK_PATH=$(realpath $1)

cd "$(dirname "$0")"
rm -rf build
mkdir build
cd build
~/src/ndk/prebuilts/cmake/linux-x86/bin/cmake \
  -DANDROID_CPP_FEATURES=no-exceptions \
  -DCMAKE_TOOLCHAIN_FILE=$NDK_PATH/build/cmake/android.toolchain.cmake   ${@:2} -GNinja ..

ninja -v

That does fail to build as expected:

FAILED: CMakeFiles/foo.dir/foo.cpp.o 
/home/danalbert/src/ndk/out/linux/android-ndk-r24-canary/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ --target=armv7-none-linux-androideabi19 --gcc-toolchain=/home/danalbert/src/ndk/out/linux/android-ndk-r24-canary/toolchains/llvm/prebuilt/linux-x86_64 --sysroot=/home/danalbert/src/ndk/out/linux/android-ndk-r24-canary/toolchains/llvm/prebuilt/linux-x86_64/sysroot   -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -march=armv7-a -mthumb -Wformat -Werror=format-security -fno-exceptions  -fPIE -MD -MT CMakeFiles/foo.dir/foo.cpp.o -MF CMakeFiles/foo.dir/foo.cpp.o.d -o CMakeFiles/foo.dir/foo.cpp.o -c ../foo.cpp
../foo.cpp:3:5: error: cannot use 'throw' with exceptions disabled
    throw 42;
    ^
../foo.cpp:2:3: error: cannot use 'try' with exceptions disabled
  try {
  ^
2 errors generated.

But if I remove the -DANDROID_CPP_FEATURES=no-exceptions flag it builds fine.

@rprichard
Copy link
Collaborator

The typo is in the non-legacy toolchain file, so you need to test with a new cmake (3.21.x and up).

@DanAlbert
Copy link
Member

Ah, crap. Good catch. My system's CMake is 3.21.4 but the script I used to generate a CMake test case uses the prebuilt :)

@DanAlbert
Copy link
Member

Yeah, there we go. The same test case using the correct version of CMake compiles successfully when -DANDROID_CPP_FEATURES=no-exceptions is used, but shouldn't have because exceptions should be disabled.

@DanAlbert DanAlbert moved this from Triaged to Needs cherry-pick in r24 Dec 15, 2021
@DanAlbert DanAlbert moved this from Triaged to Needs cherry-pick in r23c Dec 15, 2021
@DanAlbert DanAlbert moved this from Needs cherry-pick to Merged in r24 Jan 4, 2022
@DanAlbert
Copy link
Member

Should be fixed in r23 build 8486889.

r23c automation moved this from Needs cherry-pick to Merged Apr 22, 2022
MaoHan001 pushed a commit to riscv-android-src/platform-ndk that referenced this issue Jun 22, 2022
CMAKE_ANDROID_EXCEPTIONS

Bug: android/ndk#1618
Test: None
Change-Id: I40c6db01bd34123640e826ebda5eb4f9c94fe075
(cherry picked from commit 379a69bce48ee049f21f91490f0429c8c1c6a92a)
Merged-In: I40c6db01bd34123640e826ebda5eb4f9c94fe075
MaoHan001 pushed a commit to riscv-android-src/platform-ndk that referenced this issue Jun 22, 2022
Tests: No
Bugs: android/ndk#1618
Change-Id: I211593312ae6858a30a401a10681230cafb3214d
(cherry picked from commit 6bd8bfd9c3b5dbdbdfa1be5eb25f614c0d724b05)
Merged-In: I211593312ae6858a30a401a10681230cafb3214d
MaoHan001 pushed a commit to riscv-android-src/platform-ndk that referenced this issue Jun 22, 2022
Bug: android/ndk#1618
Test: None
Change-Id: Ifd678a5a8d30470165b33edd4264a23d9e689a97
(cherry picked from commit 7e6c50f3538a9b289d1f57166d3ebcdd74c13c84)
Merged-In: Ifd678a5a8d30470165b33edd4264a23d9e689a97
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
r23c
  
Merged
r24
  
Merged
Development

No branches or pull requests

4 participants