Skip to content

Commit

Permalink
[cmake] Fix -z noexecstack portability
Browse files Browse the repository at this point in the history
Summary:
Issue reported by @ryandesign and @MarcusCalhoun-Lopez.

CMake doesn't support spaces in flags. This caused older versions of gcc to
ignore the unknown flag "-z noexecstack" on MacOS since it was interpreted as a
single flag, not two separate flags. Then, during compilation it was treated as
"-z" "noexecstack", which was correctly forwarded to the linker. But the MacOS
linker does not support `-z noexecstack` so compilation failed.

The fix is to use `-Wl,-z,noexecstack`. This is never misinterpreted by a
compiler. However, not all compilers support this syntax to forward flags to the
linker. To fix this issue, we check if all the relevant `noexecstack` flags have
been successfully set, and if they haven't we disable assembly.

See also PR#4056 and PR#4061. I decided to go a different route because this is
simpler. It might not successfully set these flags on some compilers, but in that
case it also disables assembly, so they aren't required.

Test Plan:
```
mkdir build-cmake
cmake ../build/cmake/CMakeLists.txt
make -j
```

See that the linker flag is successfully detected & that assembly is enabled.

Run the same commands on MacOS which doesn't support `-Wl,-z,noexecstack` and see
that everything compiles and that `LD_FLAG_WL_Z_NOEXECSTACK` and
`ZSTD_HAS_NOEXECSTACK` are both false.
  • Loading branch information
terrelln committed Dec 20, 2024
1 parent 5a7f5c7 commit be9f1eb
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 2 deletions.
18 changes: 17 additions & 1 deletion build/cmake/CMakeModules/AddZstdCompilationFlags.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ function(EnableCompilerFlag _flag _C _CXX _LD)
endfunction()

macro(ADD_ZSTD_COMPILATION_FLAGS)
# We set ZSTD_HAS_NOEXECSTACK if we are certain we've set all the required
# compiler flags to mark the stack as non-executable.
set(ZSTD_HAS_NOEXECSTACK false)

if (CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang" OR MINGW) #Not only UNIX but also WIN32 for MinGW
# It's possible to select the exact standard used for compilation.
# It's not necessary, but can be employed for specific purposes.
Expand All @@ -75,10 +79,22 @@ macro(ADD_ZSTD_COMPILATION_FLAGS)
endif ()
# Add noexecstack flags
# LDFLAGS
EnableCompilerFlag("-z noexecstack" false false true)
EnableCompilerFlag("-Wl,-z,noexecstack" false false true)
# CFLAGS & CXXFLAGS
EnableCompilerFlag("-Qunused-arguments" true true false)
EnableCompilerFlag("-Wa,--noexecstack" true true false)
# NOTE: Using 3 nested ifs because the variables are sometimes
# empty if the condition is false, and sometimes equal to false.
# This implicitly converts them to truthy values. There may be
# a better way to do this, but this reliably works.
if (${LD_FLAG_WL_Z_NOEXECSTACK})
if (${C_FLAG_WA_NOEXECSTACK})
if (${CXX_FLAG_WA_NOEXECSTACK})
# We've succeeded in marking the stack as non-executable
set(ZSTD_HAS_NOEXECSTACK true)
endif()
endif()
endif()
elseif (MSVC) # Add specific compilation flags for Windows Visual

set(ACTIVATE_MULTITHREADED_COMPILATION "ON" CACHE BOOL "activate multi-threaded compilation (/MP flag)")
Expand Down
2 changes: 1 addition & 1 deletion build/cmake/lib/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ file(GLOB DecompressSources ${LIBRARY_DIR}/decompress/*.c)
if (MSVC)
add_compile_options(-DZSTD_DISABLE_ASM)
else ()
if(CMAKE_SYSTEM_PROCESSOR MATCHES "amd64.*|AMD64.*|x86_64.*|X86_64.*")
if(CMAKE_SYSTEM_PROCESSOR MATCHES "amd64.*|AMD64.*|x86_64.*|X86_64.*" AND ${ZSTD_HAS_NOEXECSTACK})
set(DecompressSources ${DecompressSources} ${LIBRARY_DIR}/decompress/huf_decompress_amd64.S)
else()
add_compile_options(-DZSTD_DISABLE_ASM)
Expand Down

0 comments on commit be9f1eb

Please sign in to comment.