Make SANITIZER_MIN_OSX_VERSION a cache variable

[cjappl]

It is desirable to be able to configure the -mmacosx-version-min flag for the sanitizers, but this flag was never made a CACHE variable in cmake.

By doing this, it will allow developers to select different minimum versions, which results in different interceptors being enabled or disabled on their platforms. This version can now persist between cmake runs, so it can be remembered by cmake, and edited in the cache file.

[cjappl]

@vitalybuka @ramosian-glider @kcc

Kindly requesting a review on this patch. Thanks in advance

[cjappl]

@vitalybuka @ramosian-glider @kcc

Ping

[cjappl]

@vitalybuka @ramosian-glider @kcc @petrhosek

Ping

[vitalybuka]

LGTM but please wait a few days for others feedback

@petrhosek For CMAKE expertise

@yln @sundahl as it impacts OSX

[yln]

Let me summarize my understanding: the intention here is to promote SANITIZER_MIN_OSX_VERSION to a CMAKE cache variable. We don't change any behavior or default.

LGTM, thanks!

@rsundahl @thetruestblue
At some point we should think about bumping the minimal sanitizer target enabling some cleanups.

[cjappl]

Let me summarize my understanding: the intention here is to promote SANITIZER_MIN_OSX_VERSION to a CMAKE cache variable. We don't change any behavior or default.

Your understanding is correct! No changes in behavior, just allowing people to tweak it easier.

At some point we should think about bumping the minimal sanitizer target enabling some cleanups.

One of the main reasons behind me putting up this review is I found a handful of things that would be cleaned up (allowing sanitizers to hook into calls introduced in newer MacOS versions). Once this is merged, I'm going to put up a few more in this series based on what I found! I'll make sure to add you to those reviews to get your thoughts @yln .

Thanks for the review.

[cjappl]

Pinging either:

@rsundahl for a review

Or one of my previous reviewers with write access for a merge, if appropriate.

@petrhosek @yln @vitalybuka

Thanks!

[cjappl]

Pinging either:

@rsundahl for a review

Or one of my previous reviewers with write access for a merge, if appropriate.

@petrhosek @yln @vitalybuka

[jfgoog]

This does actually change behavior. If -mmacosx-version-min is set, cmake will behave differently between the first and subsequent runs. More info at rust-lang/rust#122504 (comment)

[cjappl]

Agreed on your analysis @jfgoog . I see that on subsequent runs, MACOSX_VERSION_MIN_FLAG will be unset. This was an unintended side effect of this change (bug).

I think this patch would fix the problem, while maintaining the flexibility of this PR? Essentially hoisting the regex match out of the if statement. WDYT?

diff --git a/compiler-rt/cmake/config-ix.cmake b/compiler-rt/cmake/config-ix.cmake
index d5dc4a40c097..b2cae81d5032 100644
--- a/compiler-rt/cmake/config-ix.cmake
+++ b/compiler-rt/cmake/config-ix.cmake
@@ -463,9 +463,10 @@ if(APPLE)

   set(DEFAULT_SANITIZER_MIN_OSX_VERSION 10.10)
   set(DARWIN_osx_MIN_VER_FLAG "-mmacosx-version-min")
+  string(REGEX MATCH "${DARWIN_osx_MIN_VER_FLAG}=([.0-9]+)"
+         MACOSX_VERSION_MIN_FLAG "${CMAKE_CXX_FLAGS}")
+
   if(SANITIZER_MIN_OSX_VERSION STREQUAL "")
-    string(REGEX MATCH "${DARWIN_osx_MIN_VER_FLAG}=([.0-9]+)"
-           MACOSX_VERSION_MIN_FLAG "${CMAKE_CXX_FLAGS}")
     if(MACOSX_VERSION_MIN_FLAG)
       set(MIN_OSX_VERSION "${CMAKE_MATCH_1}")
     elseif(CMAKE_OSX_DEPLOYMENT_TARGET)

[jfgoog]

That seems reasonable, but I am hardly an expert here.

The underlying issue for me, though, which pre-dates your change, is that it seems like if the -mmacosx-version-min flag is set, we will never detect any architectures for compiler-rt, essentially making it impossible to build anything. Is that intended behavior?

[cjappl]

Pinging a couple experts to weigh in (as I am hardly an expert either)

@yln @vitalybuka @petrhosek

Pending on when they get back to us, if this is a blocking issue for rust you should file a bug report for this. I wouldn't want this "buried" in this PR with just us two talking about it

[jfgoog]

It's not blocking. It only happens with newer versions of the cc crate, and the rustc bootstrap code has an old version pinned that doesn't have this problem.

[cjappl]

I put up a review to at least revert this to the previous way of doing things:

#87580

I understand this doesn't fix your "bigger" issue, but I agree that something about that smells fishy.