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

Correctly detect if linker supports noexecstack #4061

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ryandesign
Copy link
Contributor

When testing for support of multiple flags, the flags must be separated by semicolons not spaces.

See https://gitlab.kitware.com/cmake/cmake/-/issues/26024

When referenced elsewhere, enclose the flag variable in quotation marks so that it is passed through unmodified.

@MarcusCalhoun-Lopez I believe this solution is preferable to the one proposed in #4056.

@ryandesign
Copy link
Contributor Author

Alternately, perhaps what you want is for EnableCompilerFlag to accept spaces in its flag argument and for EnableCompilerFlag to convert the spaces to semicolons prior to passing the value to CHECK_C_COMPILER_FLAG, CHECK_CXX_COMPILER_FLAG, or CHECK_LINKER_FLAG.

@ryandesign
Copy link
Contributor Author

No this is evidently not correct either. From the cmake-build-and-test-check log:

/usr/bin/ld: warning: -z ;noexecstack ignored

so the flag is still not being passed around in the correct way. I'm sorry, I don't know the correct way to fix this; I only know that what the code is currently doing doesn't work either.

This allows correct detection of linker noexecstack support.

When testing for support of multiple flags, the flags must be a list of
values separated by semicolons, not a string of values separated by
spaces. See https://gitlab.kitware.com/cmake/cmake/-/issues/26024

Enclose the flaglist variable in quotation marks when calling
CHECK_*_FLAG so that it is passed through unmodified.

After it is determined that a flag is supported, it is still the string
version of the flag variable that is appended to the corresponding
CMAKE_*_FLAGS variable.

Closes facebook#4056
@ryandesign
Copy link
Contributor Author

No this is evidently not correct either. From the cmake-build-and-test-check log:

/usr/bin/ld: warning: -z ;noexecstack ignored

so the flag is still not being passed around in the correct way.

The problem was that EnableCompilerFlag uses set to add ${_flag} to CMAKE_C_FLAGS / CMAKE_CXX_FLAGS / CMAKE_EXE_LINKER_FLAGS / CMAKE_SHARED_LINKER_FLAGS which assumes that ${_flag} is a space-separated string, not a semicolon-separated list. So I have changed it per my earlier remark:

Alternately, perhaps what you want is for EnableCompilerFlag to accept spaces in its flag argument and for EnableCompilerFlag to convert the spaces to semicolons prior to passing the value to CHECK_C_COMPILER_FLAG, CHECK_CXX_COMPILER_FLAG, or CHECK_LINKER_FLAG.

So now the way EnableCompilerFlag is called is unchanged—it still takes a space-separated string—and it converts spaces to semicolons before checking the flag(s).

@@ -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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants