[fix] handle spaces in link flags in a portable way

[MarcusCalhoun-Lopez]

See: https://cmake.org/cmake/help/latest/command/target_link_options.html#command:target_link_options

[facebook-github-bot]

Hi @MarcusCalhoun-Lopez!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[Cyan4973]

The cmake-build-and-test-check test is failing with this patch.
The relation between the patch and the error message is unclear to me,
since the patch is focused on the noexecstack flags, while the error is on detecting pthread.
Maybe the change globally impacts the way linker flags are passed down the chain, which may in turn impact pthread?
Anyway, this should be investigated.

[MarcusCalhoun-Lopez]

The cmake-build-and-test-check test is failing with this patch. The relation between the patch and the error message is unclear to me, since the patch is focused on the noexecstack flags, while the error is on detecting pthread. Maybe the change globally impacts the way linker flags are passed down the chain, which may in turn impact pthread? Anyway, this should be investigated.

Thank you for looking into this.
Unfortunately, I only have access to macOS.
The reason I started looking into this issue in the first place is because on macOS, the test should fail and wasn't because check_linker_flag doesn't seem to handle spaces properly by default.
Is there any way I can help track down the issue?

[Cyan4973]

It seems you are already trying to raise some support from the right place,
which is the right thing to do.

It may be that several lines of cmake will need to be updated together to generate the behavior you are looking for.

[ryandesign]

I'm unfamiliar with LINKER:SHELL: but the developers' response to the cmake bug report I filed was that multiple flags must be separated by semicolons, not spaces. I submitted #4061 to fix the issue that way instead.

[ryandesign]

Maybe both fixes are desired.

EnableCompilerFlag did not support flags containing a space. #4061 fixed that.

I'm unfamiliar with LINKER:SHELL:

I've now read up on it from the link you provided earlier (https://cmake.org/cmake/help/latest/command/target_link_options.html#command:target_link_options). This sounds like a reasonable way to send flags to the linker, but it seems wrong to me that it should need to be specified when calling EnableCompilerFlag. It seems like EnableCompilerFlag should prepend that internally when its fourth argument is true.

(EnableCompilerFlag calls CHECK_LINKER_FLAG and it makes me wonder why CHECK_LINKER_FLAG doesn't automatically use LINKER:, but evidently it doesn't; the fact that one could use LINKER: is even mentioned in the CHECK_LINKER_FLAG documentation.)

[ryandesign]

The relation between the patch and the error message is unclear to me,
since the patch is focused on the noexecstack flags, while the error is on detecting pthread.

With old versions of gcc, the configure check came to the incorrect conclusion that noexecstack was supported when it wasn't. Subsequent configure checks, like the one for pthread, used the unsupported -z noexecstack flag, thus causing those later configure checks to fail for the wrong reason.

[ryandesign]

The relation between the patch and the error message is unclear to me,
since the patch is focused on the noexecstack flags, while the error is on detecting pthread.

With old versions of gcc, the configure check came to the incorrect conclusion that noexecstack was supported when it wasn't. Subsequent configure checks, like the one for pthread, used the unsupported -z noexecstack flag, thus causing those later configure checks to fail for the wrong reason.

Excuse me, I was describing the problem that was reported to MacPorts and made @MarcusCalhoun-Lopez and me aware of this issue in the first place. A user trying to build zstd with CMake on an old version of macOS with an old version of gcc discovered that the old gcc would silently ignore the unknown single argument -z noexecstack, misleading your build system into thinking the flag was supported. Subsequently, the two arguments -z noexecstack were passed to the compiler when checking for pthread which failed because the compiler didn't support the -z flag.

Here, in this CI run, I would have to see the contents of CMakeConfigureLog.yaml to be sure, and I didn't see that file saved anywhere by your CI setup, but what I assume is happening is that EnableCompilerFlag passes LINKER:SHELL:-z noexecstack to CHECK_LINKER_FLAG, which transforms those flags to -Wl,-z,noexecstack (since the compiler is gcc), and the check succeeds. Then EnableCompilerFlag appends LINKER:SHELL:-z noexecstack to CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS. Then when checking for pthread it fails because the two literal arguments LINKER:SHELL:-z noexecstack are being passed to the compiler and it does not understand that. The solution would be what I proposed above: the call of EnableCompilerFlag("-z noexecstack") should not be changed, but EnableCompilerFlag should be modified to prepend LINKER:SHELL: to the flag(s) when calling CHECK_LINKER_FLAG (but not when appending the flags to CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS).

[ryandesign]

The solution would be […] EnableCompilerFlag should be modified to prepend LINKER:SHELL: to the flag(s) when calling CHECK_LINKER_FLAG (but not when appending the flags to CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS).

Well, that's not going to work either. What gets used at build time needs to be what was verified to work at configure time.

Some linkers (like the GNU linker) support -z flags like -z noexecstack and other linkers (like the macOS linker) don't. And some compilers know that -z flags like -z noexecstack should be passed to the linker and some don't.

If the configure test uses LINKER:SHELL:-z noexecstack then for the configure test -z noexecstack gets wrapped in whatever additional syntax the compiler uses to pass flags directly to the linker, for example -Wl,-z,noexecstack. You have to use-or-not-use that wrapping both at configure time and at build time, otherwise the build will fail for a user whose linker supports -z noexecstack but whose compiler doesn't know to pass it to the linker.

I don't know how to get that -Wl-or-whatever wrapping into CMAKE_*_LINKER_FLAGS. It could be done with a generator expression, but generator expressions are not available when you're simply using set to append to a string. Generator expressions are available in add_link_options but I don't know if you can simply replace your set commands with add_link_options; I don't know if they have the same scope or side effects. You're using PARENT_SCOPE with your set commands and I'm not familiar with that. You could hardcode -Wl,-z,noexecstack but that will fail if the user's compiler uses a different wrapping syntax.

The simplest thing therefore would be what I've done in #4061: Send -z noexecstack directly to the compiler, both for the configure test and for the build, and accept the fact that, for an older compiler that doesn't understand that -z flags should be sent to the linker, the feature will not be used even if the linker supports it.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!