Can you add a comment to the header of the public function that eventually surfaced the "99.0" version (I guess that's GetMacosAlignedVersionInternal)? Something like "might return version 99 in rare cases".

@dvyukov a bit more ping :)

@dvyukov more ping :)

@dvyukov friendly ping :)

Specifically, I don't want to mess with the setup for Android and/or non-Darwin crosscompiling, because I have no way of testing that.

Also, I understand that the code in question isn't best already, but I don't have the bandwidth to refactor and test any more extensive changes. I'm only trying to address the problem of getting -arch arm64 / arm64e passed into tests, otherwise we're testing the wrong thing. Let's focus on that, please.

@dvyukov Any feedback here, or is this OK to go in?

Looks good for get rid of the test failure.

Great finds, thanks! I've updated the missing places. I'm leaving the Go parts alone, for now, as Go isn't using ptrauth.

Definitely agree that this is very annoying to read. I think it would be best if we were able to change the ifdef to use something like “HAS_48_BIT_ADDRESS_SPACE” and compute that bool separately. @yln, @dvyukov WDYT?

Ah, sorry :) The context is that any test config for arm64 or arm64e isn’t getting the right -arch flag passed in.

This looks reasonable, but just for verification, could you post the "before" and "after" of the little shadow map printout from ASAN_OPTIONS=verbosity=1?

If we use #if !SANITIZER_MAC || defined(__MAC_10_14), do we still need the forward declarations and weak_import stuff?

This looks good to me. Note that I was actually suggesting to avoid the dynamic_lookup + forward declare and requiring that the *SDK* is at least 10.14 to run tests (which is separate from your OS version). But this way it's even more compatible, so I think we should go with it.

LGTM. Maybe add an if to check if the 10.14-only API is available and just do nothing in the test otherwise?

Does the testcase actually fail without the code change?

If I read the output right, it seems that even before this change, running with TSan messes up the permissions argument. I think this may be some latent bug in glibc that is only triggered in unusual environments (like TSan). I found this comment at https://lkml.org/lkml/2014/10/30/812:

...and could you also see if perhaps this change helps:

diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -137,7 +137,11 @@ const int PTHREAD_BARRIER_SERIAL_THREAD = -1;
 #endif
 const int MAP_FIXED = 0x10;
 typedef long long_t;
+#if SANITIZER_MAC || SANITIZER_FREEBSD || SANITIZER_NETBSD
 typedef __sanitizer::u16 mode_t;
+#else
+typedef __sanitizer::u32 mode_t;
+#endif

I can provide you access to a PPC64LE machine or I can check the details myself if you can tell me exactly what to do and how.

CC @simoatze, @wschmidt:

Would anyone with access to a Linux/PPC64 machine be willing to take a look if the open() syscall is receiving the right value (0600 from the testcase)?

Actually given the symptom, I'm thinking this could be a real problem with the variadic interceptor:

This fails on Linux/PPC64: http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/52026/steps/ninja%20check%201/logs/FAIL%3A%20ThreadSanitizer-powerpc64%3A%3A%20variadic-open.cpp

Let's handle simulators separately.

Looks reasonable. Could you check that swift_demangle has been correctly returning NULL for a "long time" (a couple years at least)? I'm asking because we can run on older OS's.

Overall LGTM - would be great to hear feedback from @eugenis though.

Nice.

I've mentioned this to Julian in person: The code change look good to me, but any changes in this piece of code need a high level of manual testing as this code is very critical and has potential for breaking something even with innocent changes like this.

Looks good to me. @dvyukov ?

LGTM, even if it's just for Darwin now.

Adding test

Thanks! This looks like a great step in the right direction. An overall question first: Should we use posix_spawn for *all platforms*? IIRC, unifying the launch paths was discussed on mailing lists before and sounds desirable.

LGTM, thanks!

Looks like there's no reviewers set here. @t.p.northover who should review this?

Nice, thanks. I don’t understand the ELF code so someone else should review it, but from my view this LGTM.