nmeum / android-tools

Unoffical CMake-based build system for android command line utilities
Apache License 2.0
177 stars 51 forks source link

CMake: Compile with -ftrivial-auto-var-init=zero #139

Closed z3ntu closed 5 months ago

z3ntu commented 5 months ago

As per upstream guidance [0] we should compile the sources with this flag in order to not hit issues with using uninitialized variables.

[0] https://android-review.googlesource.com/c/platform/system/core/+/2963911/1#message-01ebff378bb51f7815b6ed8b035a57fbce0418ab

Fixes #133

anatol commented 5 months ago

Thank you. Rebased your change on top of master and started testing progress.

anatol commented 5 months ago

Build step failing with

-- Found assembler: /usr/bin/cc
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - not found
-- Check if compiler accepts -pthread
-- Check if compiler accepts -pthread - no
CMake Error at /usr/local/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Threads (missing: Threads_FOUND)
Call Stack (most recent call first):
  /usr/local/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  /usr/local/share/cmake-3.28/Modules/FindThreads.cmake:226 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  vendor/boringssl/crypto/CMakeLists.txt:340 (find_package)

Some missing dependency? cc @Biswa96

munix9 commented 5 months ago

I suspect something else. In the file CMakeFiles/CMakeError.log I found the following:

clang-15.0: error: '-ftrivial-auto-var-init=zero' hasn't been enabled; enable it at your own peril for benchmarking purpose only with '-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang'

If I then replace the following

-set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -ftrivial-auto-var-init=zero")
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -ftrivial-auto-var-init=zero")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang")

the build runs through. Found this https://reviews.llvm.org/D125142

I can't judge if -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang is useful - maybe add a version check to the CMakeLists.txt if possible (cmake <= 15 ?).

Tested on openSUSE Leap 15.5

munix9 commented 5 months ago

something like this (untested):

if(CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 16)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang")
else()
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -ftrivial-auto-var-init=zero")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -ftrivial-auto-var-init=zero")
endif()

It would still have to be found out from which clang version -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang became obsolete.

munix9 commented 5 months ago

I have created a setup that hopefully works everywhere:

https://github.com/nmeum/android-tools/compare/master...munix9:android-tools:trivial-auto-var-init3 https://github.com/munix9/android-tools/actions/runs/8359486025

I think it should work. Copy it here or let me know and I'll open a new PR.

anatol commented 5 months ago

Could you please also mention in the code why we check for the compiler flags this way? I would much prefer to drop the ugly if-else flag checking once the old version clang support is dropped (~2 years from now).

z3ntu commented 5 months ago

@anatol Like this?

anatol commented 5 months ago

Looks good. @Biswa96 let me know if you have any objections. If not then I'll merge the change.