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

[compiler-rt/darwin] Disable building sanitizers on platforms without fork(). #83485

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

rohit-rao
Copy link
Contributor

The watchOS and tvOS sanitizers do not compile with publicly-available SDKs, failing on calls such as fork() and posix_spawn(). Updating the darwin_test_archs test program to include a call to fork() allows cmake to exclude those platforms when compiling runtimes. This allows public builds to enable watchOS/tvOS and compile builtins but not sanitizers.

The watchOS and tvOS sanitizers do not compile with publicly-available
SDKs, failing on calls such as fork() and posix_spawn(). Updating the
darwin_test_archs test program to include a call to fork() allows
cmake to exclude those platforms when compiling runtimes. This allows
public builds to enable watchOS/tvOS and compile builtins but not
sanitizers.
@rohit-rao
Copy link
Contributor Author

@delcypher per discussion on https://discourse.llvm.org/t/refined-controlling-of-options-in-distribution-builds-for-darwin/69976/2, the watchOS/tvOS sanitizer runtimes cannot build against public SDKs. I don't need to build sanitizers locally, but I would like to have the watchOS builtins library, and currently COMPILER_RT_ENABLE_WATCHOS=ON tries to build both (and fails).

Adding a test for fork() when evaluating archs should correlate directly with whether or not the sanitizer runtimes will actually build -- this gives us a way to exclude watchOS/tvOS while maintaining support for builtins.

Copy link
Contributor

@nico nico left a comment

Choose a reason for hiding this comment

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

I think this is a good approach (after addressing the below comment).

From what I can tell, we only need fork() to invoke the external llvm-symbolizer. Maybe in the future we could just not do that on systems where fork() isn't available and require people to symbolize on the host machine. But that can be a separate PR done by someone else at some point, and until then this seems like a good improvement to me :)

(I'll wait a day or two for @delcypher to comment as well.)

@@ -116,7 +116,7 @@ function(darwin_test_archs os valid_archs)
if(NOT TEST_COMPILE_ONLY)
message(STATUS "Finding valid architectures for ${os}...")
set(SIMPLE_C ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/src.c)
file(WRITE ${SIMPLE_C} "#include <stdio.h>\nint main(void) { printf(__FILE__); return 0; }\n")
file(WRITE ${SIMPLE_C} "#include <stdio.h>\nint main(void) { printf(__FILE__); fork(); return 0; }\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs an #include <unistd.h>\n too, else it will disable the sanitizers on macOS too:

% cat test.c
#include <stdio.h>
int main() { fork(); }
% clang -c test.c
test.c:3:14: error: call to undeclared function 'fork'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
int main() { fork(); }
             ^
1 error generated.

@vitalybuka
Copy link
Collaborator

vitalybuka commented Mar 1, 2024

Please put into title that this is Darwin.

@nico nico changed the title [compiler-rt] Disable building sanitizers on platforms without fork(). [compiler-rt/darwin] Disable building sanitizers on platforms without fork(). Mar 1, 2024
@nico
Copy link
Contributor

nico commented Mar 1, 2024

@jroelofs fyi

@jroelofs jroelofs requested review from yln and kubamracek March 1, 2024 16:53
@nico
Copy link
Contributor

nico commented Mar 5, 2024

This seems harmless enough, I'll go ahead and merge this. Others: Please shout if this causes issues!

@nico nico merged commit 9faca1e into llvm:main Mar 5, 2024
4 checks passed
@jyknight
Copy link
Member

jyknight commented Mar 5, 2024

FWIW, last time I looked at this a few years ago, from what I could tell, tvOS/watchOS do have fork/etc calls. They're marked as "unavailable", but that's only an issue with the headers. Apple actually does ship these libraries (containing calls to fork/etc!), so I suspect they internally have a build option which disables the "unavailable" annotations.

It seemed that modifying the Availability.h header to define __WATCHOS_PROHIBITED, __TVOS_PROHIBITED, and __API_UNAVAILABLE(...) as no-op macros does let the sanitizers libraries build successfully.

So, if someone wanted to, they could probably fix the compiler-rt cmake to support building the sanitizer runtimes on these platforms just by creating a wrapper Availability.h file to undef/redefine those macros as no-ops, and putting it on the include path ahead of the system headers.

@nico
Copy link
Contributor

nico commented Mar 6, 2024

Does having the sanitizers runtimes help you if you can't use them though?

As mentioned further up, the fork isn't really needed for anything – is someone wanted to build them, the nicer fix seems to not run llvm-symbolizer on-device on these platforms, and then there's no need for fork.

@jyknight
Copy link
Member

jyknight commented Mar 6, 2024

I think you can use sanitizer libs you build...can't you?

W.r.t. fork/exec, certainly it is denied by the kernel at runtime on any "normal" i/tv/watch OS device, but I suspect it probably works fine in the simulator. Don't know whether that gets used in practice or not.

@nico
Copy link
Contributor

nico commented Mar 6, 2024

I think you can use sanitizer libs you build...can't you?

I'd expect them to be killed once the fork() is run that tries to execute llvm-sybmbolizer when an issue is found as you say, right?

Taking a step back, is this causing problems on your end, or is this more a hypothetical situation we're discussing? :)

@jyknight
Copy link
Member

jyknight commented Mar 6, 2024

No problems -- I was actually just trying to record information that was in my head, in case someone wanted to revisit this in the future.

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.

None yet

5 participants