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.

Sign up for GitHub

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

Correctly detect if linker supports noexecstack #4061

Closed
wants to merge 1 commit into from
Closed

Correctly detect if linker supports noexecstack #4061

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions build/cmake/CMakeModules/AddZstdCompilationFlags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,19 @@ if (ZSTD_HAVE_CHECK_LINKER_FLAG)
endif()

function(EnableCompilerFlag _flag _C _CXX _LD)
string(REGEX REPLACE " " ";" flaglist "${_flag}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about separate_arguments() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In developing this fix I did encounter separate_arguments but it seems to do a whole lot of things that are unnecessary here. It is intended for parsing a program's command line arguments and handles quoting and escaping and other things which are not applicable here and would make it slower than just doing the single character replacement we want.

However, using just SHELL:${_flag}, and not splitting ${flag} into a list, may be a better bet. Flags given as a list are assumed to be independent and subject to deduplication which we would not want here. For example, if -z foo were already in the flags and we test for -z bar cmake might remove the "duplicate" -z and end up with the invalid -z foo bar.

Copy link
Contributor

@Cyan4973 Cyan4973 Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance shouldn't be a concern here.
Rather I'm concerned by :

  • 2 keywords compilation flags, that should be considered as a single item: this space-character replacement will automatically separate both sides, this won't end well (and yes, I suspect everything works fine for now because there is no such 2-keywords flag at the moment the regex replace is invoked, but that's a recipe for future surprises)
  • I wasn't aware of any implicit deduplication of a cmake list, it would indeed be a problem
  • The general "magic" of a regex replace, which doesn't explain the intent. I was initially considering a request for code comment to explain why this line is there. Then I discovered separate_arguments(), which would have the advantage of being a bit more clear about the intention, but possibly a bit less clear about the implementation.

string(REGEX REPLACE "\\+" "PLUS" varname "${_flag}")
string(REGEX REPLACE "[^A-Za-z0-9]+" "_" varname "${varname}")
string(REGEX REPLACE "^_+" "" varname "${varname}")
string(TOUPPER "${varname}" varname)
if (_C)
CHECK_C_COMPILER_FLAG(${_flag} C_FLAG_${varname})
CHECK_C_COMPILER_FLAG("${flaglist}" C_FLAG_${varname})
if (C_FLAG_${varname})
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${_flag}" PARENT_SCOPE)
endif ()
endif ()
if (_CXX)
CHECK_CXX_COMPILER_FLAG(${_flag} CXX_FLAG_${varname})
CHECK_CXX_COMPILER_FLAG("${flaglist}" CXX_FLAG_${varname})
if (CXX_FLAG_${varname})
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${_flag}" PARENT_SCOPE)
endif ()
Expand All @@ -39,7 +40,7 @@ function(EnableCompilerFlag _flag _C _CXX _LD)
# results for this configuration,
# see: https://gitlab.kitware.com/cmake/cmake/-/issues/22023
if (ZSTD_HAVE_CHECK_LINKER_FLAG AND NOT MSVC)
CHECK_LINKER_FLAG(C ${_flag} LD_FLAG_${varname})
CHECK_LINKER_FLAG(C "${flaglist}" LD_FLAG_${varname})
else ()
set(LD_FLAG_${varname} false)
endif ()
Expand Down