Thanks for looking at this. Ordering the CMake options makes a lot of sense. You probably should clean up the commit description to only be about the change you're making here and handle "Frequently used options" to a separate patch. Other than that LGTM.

In D103304#2797811, @delcypher wrote:

@kda This change has broken our internal builds on macOS. I'm not sure why it hasn't broken the public ones. If you add a new weak symbol it needs to be added to lib/asan/weak_symbols.txt so that the linker on macOS knowns it's okay for the symbol to not be defined. We'll put up a patch to fix this shortly.

In D103304#2797889, @thakis wrote:

Oh, I see that that was already reported over an hour ago. Reverted for now in 5c600dc6d4b75bc71dc3033f37ea187b3fd454d7

@kda This change has broken our internal builds on macOS. I'm not sure why it hasn't broken the public ones. If you add a new weak symbol it needs to be added to lib/asan/weak_symbols.txt so that the linker on macOS knowns it's okay for the symbol to not be defined. We'll put up a patch to fix this shortly.

The patch seems reasonable. I have a minor nit about the use of the % operator but other than that it seems fine.

I've landed b7f60d861ad7a8d03c09c0b72277eabaa53731fb to try to unbreak the likely misconfigured https://lab.llvm.org/buildbot/#/builders/119/builds/3801 bot.

In D101681#2761522, @delcypher wrote:

In D101681#2761503, @hubert.reinterpretcast wrote:

I understand this patch is trying to expose unintended configurations (thanks!). I'll note that it might not be the friendliest patch to land on a Friday.
Please see the state of the sanitizer-ppc64le-linux bot: https://lab.llvm.org/buildbot/#/builders/19.

Sorry about this. I'm going to land a patch now to downgrade the fatal error to warning to unbreak the bots for now. Owner's of the bot should investigate this warning though because it points to the testing configuration being wrong.

In D101681#2761503, @hubert.reinterpretcast wrote:

I understand this patch is trying to expose unintended configurations (thanks!). I'll note that it might not be the friendliest patch to land on a Friday.
Please see the state of the sanitizer-ppc64le-linux bot: https://lab.llvm.org/buildbot/#/builders/19.

Rename option name and lit variables to address @yln 's feedback.

Address @aralisza's feedback.

I tried out this patch on Linux with an in-tree build (tests run without failing the new check added) and an out of tree build (testing refuses to run as expected).

Ping.

Simplify patch because https://reviews.llvm.org/D101682 has landed.

LGTM

Use isOSDarwin() instead of explicitly checking Darwin OS enum values.

In D101681#2735499, @phosek wrote:

Do we know for sure that people are using compiler-rt test suite to test runtime libraries shipped with their compiler? I'm a bit surprised by that since it's not a case I've ever considered.

Use -print-runtime-dir to support non Darwin platforms.

• Support other Apple target triples.

I'm abandoning this change. It is superseded by the much more general https://reviews.llvm.org/D101681

@jansvoboda11 I'm going to land this patch as is (with your nit fixed). If you would like me to add an alias please follow up with me and I can put up a patch to do that.

Rename def too.

In D101491#2725348, @jansvoboda11 wrote:

In D101491#2724025, @delcypher wrote:

@jansvoboda11 Should I use the the alias feature so that the old -fsanitize-address-destructor-kind argument is still parsed? I'm not sure if it's worth it but if doing so is very simple and has a low maintenance burden we should probably do it.

I'm fine with omitting the alias if the original flag didn't make it into a release and it's unlikely that downstream TOT users are using it.

Remove metavar

@jansvoboda11 Should I use the the alias feature so that the old -fsanitize-address-destructor-kind argument is still parsed? I'm not sure if it's worth it but if doing so is very simple and has a low maintenance burden we should probably do it.

@yln Your comments have convinced me that I should try doing this a different way. I'll try the approach I outlined and I'll see how far I get.

In D101335#2720966, @yln wrote:

Would it be possible (and wouldn't it be better?) to set config.compiler_rt_libdir to the right path in the first place? This way we wouldn't get into the "path mismatch" state that we warn about.

LGTM other than my nits.

In D100998#2709526, @vitalybuka wrote:

In D100998#2709448, @delcypher wrote:

@vitalybuka Damn looks like you landed the patch while I was in the middle of writing a review. Could you make the fix I requested as a follow up patch?

sure, sorry, I didn't noticed comments.
I even can revert if there is serious issues.

@vitalybuka Damn looks like you landed the patch while I was in the middle of writing a review. Could you make the fix I requested as a follow up patch?

The approach looks good but I'd like to see us type check the new run_under parameter.

@vitalybuka Thanks for tackling this. I've always wanted to have our unit tests be runnable on targets that aren't the host. This looks like the first step towards that.

LGTM apart from the description. The description probably should mention this is the runtime counterpart to the previous patch that added static checks. We have to do this check at runtime because we get the VM size from the kernel at runtime.

LGTM

LGTM apart from the static keyword which "I think" shouldn't be necessary here.

LGTM

LGTM. Do we have a test that fails without this, but succeeds with it (on AS)?

Just floating this as on option. We could call the new define SANITIZER_MACOS.

@aralisza You can ignore the lint checks here.

I'm not a big fan of the name but the sensible name (SANTIZER_MAC) is already taken unfortunately.

@aralisza Sorry I missed this patch. LGTM.

@bruno Thanks for the patch. TSan's runtime isn't my specialty so I've added other reviewers.

In D99621#2670431, @phosek wrote:

LGTM (I've tested this together with D99621 locally for the Fuchsia build and everything seems to be working).

@phosek Thanks for the review.

• Renamed CRT_SRC_ROOT_PATH to COMPILER_RT_ROOT_SRC_PATH

LGTM

The code in compiler-rt/cmake/builtin-config-ix.cmake is reasonable. Although we (Apple) should probably upstream our change because it's slightly different. I'm not sure about the change in compiler-rt/lib/builtins/CMakeLists.txt we don't have the change you made. I'll put a patch up to upstream our version and we'll see how it goes.