Skip to content

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

Merged
anatol merged 1 commit intonmeum:masterfrom
z3ntu:trivial-auto-var-init
Mar 20, 2024
Merged

CMake: Compile with -ftrivial-auto-var-init=zero#139
anatol merged 1 commit intonmeum:masterfrom
z3ntu:trivial-auto-var-init

Conversation

@z3ntu
Copy link

@z3ntu z3ntu commented Mar 20, 2024

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

@z3ntu z3ntu mentioned this pull request Mar 20, 2024
Closed
@anatol anatol force-pushed the trivial-auto-var-init branch from e45342c to 59ff36d Compare March 20, 2024 07:43
@anatol
Copy link
Collaborator

anatol commented Mar 20, 2024

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

@anatol
Copy link
Collaborator

anatol commented Mar 20, 2024 •
edited
Loading

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
Copy link
Contributor

munix9 commented Mar 20, 2024

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
Copy link
Contributor

munix9 commented Mar 20, 2024

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
Copy link
Contributor

munix9 commented Mar 20, 2024

I have created a setup that hopefully works everywhere:

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.

@z3ntu z3ntu force-pushed the trivial-auto-var-init branch from 59ff36d to 6b3d9ff Compare March 20, 2024 14:37
@anatol
Copy link
Collaborator

anatol commented Mar 20, 2024

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 z3ntu force-pushed the trivial-auto-var-init branch from 6b3d9ff to 9d7a194 Compare March 20, 2024 20:44
@z3ntu
Copy link
Author

z3ntu commented Mar 20, 2024

@anatol Like this?

@anatol
Copy link
Collaborator

anatol commented Mar 20, 2024

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

@anatol anatol merged commit c2a0610 into nmeum:master Mar 20, 2024
@z3ntu z3ntu deleted the trivial-auto-var-init branch March 21, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Build with -ftrivial-auto-var-init=zero

4 participants

@z3ntu @anatol @munix9 @lweiss-fairphone