[compiler-rt][Darwin] Add arm64 to simulator platforms

[bc-lee]

Summary

This patch is the reland of a8e5ce76b475a22546090a73c22fa4f83529aa4e,
which includes additional SDK version checks to ensure that
XCode's headers support arm64 builds.

[bc-lee]

I’ve tested with Xcode 11(which doesn’t support Arm64 Simulators) and Xcode 13. See build log [1] for details.

Also, I think I need to give @thieta's credit for the original patch in https://reviews.llvm.org/D118759. Is there a standard way to do it?

[1] https://github.com/bc-lee/llvm-build-test-D119174/actions/runs/1808508640

[thieta]

In D119174#3302239, @bc-lee wrote:

I’ve tested with Xcode 11(which doesn’t support Arm64 Simulators) and Xcode 13. See build log [1] for details.

Also, I think I need to give @thieta's credit for the original patch in https://reviews.llvm.org/D118759. Is there a standard way to do it?

[1] https://github.com/bc-lee/llvm-build-test-D119174/actions/runs/1808508640

Hi. Thanks for taking this further, I was about to revisit this myself later. My idea was to make the test if the arch worked in that platform more robust since this list is really just a list of archs to probe. Why is the test function enabling this arch for older platforms that don't support it?

Either way - if that approach is more complicated I am fine with this.

Also no need for attribution when the original fix was so easy - would have been another thing if my original code was many many lines. IMHO of course.

[bc-lee]

Hello, subscribers. Can this patch be reviewed?

Downstream project's issue is as follows; https://crbug.com/1223481

[thieta]

In D119174#3339310, @bc-lee wrote:

Hello, subscribers. Can this patch be reviewed?

Downstream project's issue is as follows; https://crbug.com/1223481

Hi,

Did you see my comment about moving the check to the place where the architectures are tested? If this has been investigated I am happy to give my 👍 for this one.

[bc-lee]

Sorry. I forgot your previous comment.

I understand that you have commented that reducing the target architecture is necessary. Is that right?

When I open the libSystem.tbd file of the iOS simulator in Xcode, it shows that i386, x86_64, arm64 are supported. So I’ll replace the use of ${X86} and ${X86_64} with i386 and x86_64 respectively. Would this be enough?

[thieta]

In D119174#3342072, @bc-lee wrote:

Sorry. I forgot your previous comment.

I understand that you have commented that reducing the target architecture is necessary. Is that right?

When I open the libSystem.tbd file of the iOS simulator in Xcode, it shows that i386, x86_64, arm64 are supported. So I’ll replace the use of ${X86} and ${X86_64} with i386 and x86_64 respectively. Would this be enough?

Sorry for the late reply.

No that's not exactly what I mean:

We add the arch names to the DARWIN_X_BUILTIN_ALL_POSSIBLE_ARCHS in this bit of code. Then we loop over them and call: darwin_test_archs to see if that arch is usable. What I wonder is if we can just keep arm64 on this list and then just make sure we catch whatever goes wrong during builds in darwin_test_archs so that we can programmatically detect if a arch is usable or not, this would be better IMHO than hard-coding versions of xcode in this method.

Right now the darwin_test_archs must be successful and then fail when trying to build this, IMHO we should fix that.

[bc-lee]

Change the darwin_test_archs function so that CMake can check that
the current compiler can compile and link regardless of the
TEST_COMPILE_ONLY variable.

[thieta]

In D119174#3350794, @bc-lee wrote:

Change the darwin_test_archs function so that CMake can check that
the current compiler can compile and link regardless of the
TEST_COMPILE_ONLY variable.

This is much more inline with what I was thinking. But I am not sure how this commit fixes the problem, did you test run this on a system with an older Xcode? The original issue was that cdefs.h complained about unsupported architecture, see this CI job: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/21780/console

Where you able to reproduce this problem and did this fix solve the problem?

[bc-lee]

This is much more inline with what I was thinking. But I am not sure how this commit fixes the problem, did you test run this on a system with an older Xcode? The original issue was that cdefs.h complained about unsupported architecture, see this CI job: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/21780/console

Where you able to reproduce this problem and did this fix solve the problem?

Yes. I tested it using Xcode 11.7 and it works as expected. https://github.com/bc-lee/llvm-build-test-D119174/runs/5373867116

This execution was done using the following commit: https://github.com/bc-lee/llvm-project/commits/4c5696c5165e35ae27b0ff5d78deb230f124134

[bc-lee]

Sorry, wrong link. Correct one is here: https://github.com/bc-lee/llvm-project/commits/4c5696c5165e35ae27b0ff5d78dedb230f124134

[bc-lee]

Can we re-land this and see if anything is broken by this?

[bc-lee]

Hi, could you have a look at it once more?

[thakis]

Can you talk a bit more about the TEST_COMPILE_ONLY change? That looks like it's against the spirit of D19692.

To me, the first version of this patch looks safer and makes more sense. The second version might be fine, but I don't immediately understand why.

[thakis]

In other words, how about we restore version 1, I lg that, and then we can think about doing version 2 as a follow-up in a separate patch?

[thieta]

In D119174#3461881, @thakis wrote:

In other words, how about we restore version 1, I lg that, and then we can think about doing version 2 as a follow-up in a separate patch?

As I stated above - I am not opposed to checking the version of Xcode like in version 1. At the same time I would like to understand why the testing of that architecture succeeds in testing - but not compiling later. But this is definitely not something I want to block this patch on, especially if everyone else thinks this is fine.

[bc-lee]

I'm fine with version 1. It can check SDK version, so it won't have errors like previous one.

In D119174#3461897, @thieta wrote:

At the same time I would like to understand why the testing of that architecture succeeds in testing - but not compiling later.

From what I understand, try_compile_only tries to compile using very simple source code: int foo(int x, int y) { return x + y; }\n , so it might succeed, but the real problem came later.

[thakis]

Cool, then let's land version 1 for now.

[bc-lee]

Revert to version 1

[bc-lee]

Please commit this on my behalf. Thanks.

[thieta]

In D119174#3464848, @bc-lee wrote:

Please commit this on my behalf. Thanks.

What would be your name and email you want me to use in the commit?

[bc-lee]

Byoungchan Lee <[e-mail]>. Thank you!

[iains]

unfortunately, this breaks what I believe to be a reasonable build setup on Darwin/macOS.

On most of my build machines, I have only the command line tools installed - to provide the bootstrap compiler and native SDK.

However, I usually enable X86, Arm, AArch64 and PowerPC backends .. (it would seem reasonable that I could check code-gen for aarch64-linux without needing an iphonesimulator SDK, ?)

With this patch, the compiler-rt cmake now fails with a fatal error because the command line tools do not include the SDKs for the embedded OSs.

Perhaps a solution might be to provide a flag to find_darwin_sdk_version() so that it can be made optional (and thus not a fatal error if missing) - and thus make the addition to the testing conditional on the SDK being available?

[bc-lee]

That's interesting. Does setting COMPILER_RT_ENABLE_IOS=Off when calling CMake fixes the problem?

[iains]

In D119174#3467044, @bc-lee wrote:

That's interesting. Does setting COMPILER_RT_ENABLE_IOS=Off when calling CMake fixes the problem?

Yes, that appears to Work For Me and seems a reasonable solution, thanks.

[delcypher]

@bc-lee This has broken some of Apple's internal builds. You didn't have a single person from Apple approve this code before landing it which is not acceptable given that it affects only Apple platforms. Please CC @yln @thetruestblue @kubamracek @rsundahl and me in the future if you want to land something like this.

I am going to revert this.

[delcypher]

Reverted in 3469cb14e2316a1e3cf64db5be3738379d9daa8d

@bc-lee While the change here looks reasonable to me, it conflicts with our (Apple) internal code and you didn't have any reviewer approve this patch.

I see you said

Hello, subscribers. Can this patch be reviewed?

Unfortunately that is very unlikely to get the eyeballs you want on a patch. If you ping specific people you are much more likely to get someone to take a look at your patch. Sometimes, it's hard to know who that is. I see you added @t.p.northover who is certainly a good person to add, but there are others. Apart from the above that I listed above @arphaman and @steven_wu are sometimes people worth speaking to as well.

I'll need to chat with my colleagues to see if we should upstream what we have internally or if the approach in this patch is adequate.

[bc-lee]

Okay. I understand. Sorry for broke your code.

I haven't contributed much to LLVM, so I wasn't familiar with the rules you're talking about. It would be nice if these rules were documented somewhere.
Anyway, I think it would be nice for Apple people to upstream Apple's internal changes to LLVM as well. Especially considering that Xcode already provides Arm64 simulator versions of compiler-rt.

[delcypher]

In D119174#3469454, @bc-lee wrote:

Okay. I understand. Sorry for broke your code.

Not a problem. Breaking our internal code isn't the problem I was concerned about here because you have know way of knowing that would happen and it's not your responsibility to be concerned about it. What I was concerned with is that the patch appeared (in the phabriactor UI) to be landed without approval from an Apple engineer, or in fact anyone.

I haven't contributed much to LLVM, so I wasn't familiar with the rules you're talking about.

I think they're more guidelines than rules. As a general rule of thumb though you ideally want at least one reviewer to approve your patch to land it. At a glance it looks like this patch wasn't reviewed but upon inspection I see it was reviewed but @thieta but they weren't listed as a reviewer and so the patch looked like it wasn't approved in phabriactor when it landed. Sorry I didn't look closer.

I also see that on closer inspection that an older version of this patch (https://reviews.llvm.org/D118759) was reviewed by an Apple engineer.

So I think I reverted this a bit too hastily. Sorry about that.

It would be nice if these rules were documented somewhere.

There is a little bit of discussion here about pinging reviewers: https://llvm.org/docs/Contributing.html#how-to-submit-a-patch that might have helped in this case but perhaps it's not very clear. When I "ping" I typically @ the reviewers so that they hopefully get notified, asking the "subscribers" to review the patch is very unlikely to get the attention of the reviewers.

Anyway, I think it would be nice for Apple people to upstream Apple's internal changes to LLVM as well. Especially considering that Xcode already provides Arm64 simulator versions of compiler-rt.

I've spoken to the people who own the internal code that conflicts with this patch. They are happy for this patch to land. So what I'll do is formally approve the patch and reland the patch on your behalf.

Sorry for the noise this has generated.

[delcypher]

Approved.

[delcypher]

Patch relanded as a680c212cb213bf73be7d3e2ee919fdc743cef0c

[bc-lee]

In D119174#3469474, @delcypher wrote:

Sorry for the noise this has generated.

Well, it help me understand how to pick a reviewer and how to ping when a patch is stall.
Thanks for the quick conclusion and relanding the patch! Have a nice friday/weekend.

[thakis]

I'd like to push back on this a bit. D121868 has arphaman listed as reviewer and has been sitting without comments for over a week. Multiple catalyst patches in general haven't seen any interaction.

V1 of this patch here had been blessed by t.p.northover as you mention.

If there's someone on your end who's interested in reviewing catalyst patches, great! Let us know who and we'll gladly take their feedback into account. But we've been trying and it's been hard to find traction. We had to wait several months for the initial catalyst stuff to land upstream. Saying "you can land anything without approval from us" but also ghosting reviews isn't very helpful.

[thakis]

(Thanks for relanding, though!)

[thakis]

(closing since patch has relanded)

compiler-rt/cmake/builtin-config-ix.cmake

if(APPLE)
   if(COMPILER_RT_ENABLE_IOS)
     list(APPEND DARWIN_EMBEDDED_PLATFORMS ios)
     set(DARWIN_ios_MIN_VER_FLAG -miphoneos-version-min)
     set(DARWIN_ios_BUILTIN_MIN_VER 6.0)
     set(DARWIN_ios_BUILTIN_MIN_VER_FLAG
       ${DARWIN_ios_MIN_VER_FLAG}=${DARWIN_ios_BUILTIN_MIN_VER})
     set(DARWIN_ios_BUILTIN_ALL_POSSIBLE_ARCHS ${ARM64} ${ARM32})
     set(DARWIN_iossim_BUILTIN_ALL_POSSIBLE_ARCHS ${X86} ${X86_64})
+    find_darwin_sdk_version(iossim_sdk_version "iphonesimulator")
+    if ("${iossim_sdk_version}" VERSION_GREATER 14.0 OR "${iossim_sdk_version}" VERSION_EQUAL 14.0)
+      list(APPEND DARWIN_iossim_BUILTIN_ALL_POSSIBLE_ARCHS arm64)
+    endif()
   endif()
   if(COMPILER_RT_ENABLE_WATCHOS)
     list(APPEND DARWIN_EMBEDDED_PLATFORMS watchos)
     set(DARWIN_watchos_MIN_VER_FLAG -mwatchos-version-min)
     set(DARWIN_watchos_BUILTIN_MIN_VER 2.0)
     set(DARWIN_watchos_BUILTIN_MIN_VER_FLAG
       ${DARWIN_watchos_MIN_VER_FLAG}=${DARWIN_watchos_BUILTIN_MIN_VER})
     set(DARWIN_watchos_BUILTIN_ALL_POSSIBLE_ARCHS armv7 armv7k arm64_32)
     set(DARWIN_watchossim_BUILTIN_ALL_POSSIBLE_ARCHS ${X86})
+    find_darwin_sdk_version(watchossim_sdk_version "watchsimulator")
+    if ("${watchossim_sdk_version}" VERSION_GREATER 7.0 OR "${watchossim_sdk_version}" VERSION_EQUAL 7.0)
+      list(APPEND DARWIN_watchossim_BUILTIN_ALL_POSSIBLE_ARCHS arm64)
+    endif()
   endif()
   if(COMPILER_RT_ENABLE_TVOS)
     list(APPEND DARWIN_EMBEDDED_PLATFORMS tvos)
     set(DARWIN_tvos_MIN_VER_FLAG -mtvos-version-min)
     set(DARWIN_tvos_BUILTIN_MIN_VER 9.0)
     set(DARWIN_tvos_BUILTIN_MIN_VER_FLAG
       ${DARWIN_tvos_MIN_VER_FLAG}=${DARWIN_tvos_BUILTIN_MIN_VER})
     set(DARWIN_tvos_BUILTIN_ALL_POSSIBLE_ARCHS armv7 arm64)
     set(DARWIN_tvossim_BUILTIN_ALL_POSSIBLE_ARCHS ${X86} ${X86_64})
+    find_darwin_sdk_version(tvossim_sdk_version "appletvsimulator")
+    if ("${tvossim_sdk_version}" VERSION_GREATER 14.0 OR "${tvossim_sdk_version}" VERSION_EQUAL 14.0)
+      list(APPEND DARWIN_tvossim_BUILTIN_ALL_POSSIBLE_ARCHS arm64)
+    endif()
   endif()
 
   set(BUILTIN_SUPPORTED_OS osx)
 
   # We're setting the flag manually for each target OS
   set(CMAKE_OSX_DEPLOYMENT_TARGET "")
 
   # NOTE: We deliberately avoid using `DARWIN_<os>_ARCHS` here because that is