This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Separate the detection Darwin platforms architectures for the built-ins from the rest of compiler-rt.
ClosedPublic

Authored by delcypher on Mar 8 2019, 12:59 PM.

Details

Summary

The detection of supported platform (os) architectures for Darwin relies
on the darwin_test_archs() CMake function. This is used both for
building the builtins (builtin-config-ix.cmake) and for the rest of
the compiler-rt (config-ix.cmake).

darwin_test_archs() implements a cache, presumably to speed up CMake
re-configures. Unfortunately this caching is buggy because it depends
on external global state (i.e. the TEST_COMPILE_ONLY variable) and
this is not taken into account. For config-ix.cmake
TEST_COMPILE_ONLY is not set and for builtin-config-ix.cmake
TEST_COMPILE_ONLY is set to On. This makes the
darwin_test_archs() function racey in the sense that a call from one
calling context will poison the cache for the other calling context.

This is actually an issue George Karpenkov discovered a while back
and had an incomplete patch for (https://reviews.llvm.org/D45337)
but this was never merged.

To workaround this, this patch switches to using a different set of
variables for the platform architecture builtins, i.e.
DARWIN_<OS>_ARCHS -> DARWIN_<OS>_BUILTIN_ARCHS. This avoids the
cache poisoning problem because the cached variable names are different.
This also has the advantage that the the configured architectures for
builtins and the rest of the compiler-rt are now independent and
can be set differently if necessary.

rdar://problem/48637491

Event Timeline

delcypher created this revision.Mar 8 2019, 12:59 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 8 2019, 12:59 PM
Herald added subscribers: Restricted Project, jdoerfert, mgorny, dberris. · View Herald Transcript
vsk added a subscriber: vsk.Sep 11 2019, 4:31 PM
delcypher updated this revision to Diff 220029.Sep 12 2019, 5:28 PM

Pass -w when calling try_compile_only(...) to make architecture detection more robust.

@vsk @arphaman Is it okay to land this?

vsk accepted this revision.Sep 12 2019, 5:41 PM

Lgtm, thanks!

This revision is now accepted and ready to land.Sep 12 2019, 5:41 PM

@vsk Thanks for the review.

delcypher added a comment.EditedSep 25 2019, 9:01 PM

This landed (r371871) but the "Differential Revision:" line was missing.

delcypher closed this revision.Sep 25 2019, 9:02 PM

Closing as change already landed.