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

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

Merged
merged 1 commit into from
Mar 20, 2024

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

@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

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).

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 nmeum#133

Co-authored-by: munix9 <44939650+munix9@users.noreply.github.com>
@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
13 of 14 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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