Beginnings of native Android build #110471
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,6 +79,8 @@ build_native() | |
| fi | ||
|
|
||
| if [[ "$targetOS" == android || "$targetOS" == linux-bionic ]]; then | ||
| # Keep in sync with $(AndroidApiLevelMin) in Directory.Build.props in the repository rooot | ||
| local ANDROID_API_LEVEL=29 | ||
|
There was a problem hiding this comment. @grendello has mentioned in #110470 that even API level 28 is unfortunate and that we will want to support older API level, e.g. the originally used level 21. There was a problem hiding this comment. It's not good in the long term and is something we should fix. Moving it to 29 for now allows us to not have to deal with emulated TLS usage. There was a problem hiding this comment. Unless we can fix it now, I don't think this should block progress. We can do it sometime down the road. There was a problem hiding this comment. In perspective, this would drop 12.8% of the devices (with API level from 21 to 29) according to https://apilevels.com/ Then again: There was a problem hiding this comment. I think I can make the emulated TLS work without too much hassle. The question though is if we are worried about the perf implications of the emulated TLS vs the standard TLS and thus we would like to end up having support for both. There was a problem hiding this comment. Perf is important, so I think the long-term solution should be that we have two builds of the runtime (alas): one with emulated TLS for the older devices (we mustn't ignore the older Android versions, 12.8% is still a lot of devices out there), and another targeting API 29 and onwards. It will increase nuget size and the runtime build times on CI, but I think it would be the best way forward. There was a problem hiding this comment. I'd like to avoid runtime explosion if we can help it. |
||
| if [[ -z "$ANDROID_NDK_ROOT" ]]; then | ||
| echo "Error: You need to set the ANDROID_NDK_ROOT environment variable pointing to the Android NDK root." | ||
| exit 1 | ||
|
|
@@ -87,7 +89,7 @@ build_native() | |
| cmakeArgs="-C $__RepoRootDir/eng/native/tryrun.cmake $cmakeArgs" | ||
|
|
||
| # keep ANDROID_PLATFORM in sync with SetOSTargetMinVersions in the root Directory.Build.props | ||
| cmakeArgs="-DCMAKE_TOOLCHAIN_FILE=$ANDROID_NDK_ROOT/build/cmake/android.toolchain.cmake -DANDROID_PLATFORM=android-${ANDROID_API_LEVEL} -DANDROID_NATIVE_API_LEVEL=${ANDROID_API_LEVEL} $cmakeArgs" | ||
|
|
||
| # Don't try to set CC/CXX in init-compiler.sh - it's handled in android.toolchain.cmake already | ||
| __Compiler="default" | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -250,8 +250,10 @@ if(NOT CLR_CMAKE_HOST_MACCATALYST AND NOT CLR_CMAKE_HOST_IOS AND NOT CLR_CMAKE_H | |
| add_subdirectory(utilcode) | ||
| add_subdirectory(inc) | ||
|
|
||
| if (NOT CLR_CMAKE_TARGET_ANDROID) | ||
|
There was a problem hiding this comment. Could we instead define a property for "build executables" instead of per-platform? |
||
| add_subdirectory(ilasm) | ||
| add_subdirectory(ildasm) | ||
| endif(NOT CLR_CMAKE_TARGET_ANDROID) | ||
| add_subdirectory(gcinfo) | ||
| add_subdirectory(jit) | ||
| add_subdirectory(vm) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,11 +16,17 @@ | |
|
|
||
| #include <stdint.h> | ||
|
|
||
| #ifdef __cplusplus | ||
| #define CORECLR_HOSTING_API_LINKAGE extern "C" | ||
| #else | ||
| #define CORECLR_HOSTING_API_LINKAGE | ||
| #endif | ||
|
Comment on lines
+19
to
+23
|
||
|
|
||
| // For each hosting API, we define a function prototype and a function pointer | ||
| // The prototype is useful for implicit linking against the dynamic coreclr | ||
| // library and the pointer for explicit dynamic loading (dlopen, LoadLibrary) | ||
| #define CORECLR_HOSTING_API(function, ...) \ | ||
| CORECLR_HOSTING_API_LINKAGE int CORECLR_CALLING_CONVENTION function(__VA_ARGS__); \ | ||
| typedef int (CORECLR_CALLING_CONVENTION *function##_ptr)(__VA_ARGS__) | ||
|
|
||
| // | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -696,15 +696,15 @@ typedef struct _T_KNONVOLATILE_CONTEXT_POINTERS { | |
| #define DAC_CS_NATIVE_DATA_SIZE 24 | ||
| #elif defined(TARGET_FREEBSD) && defined(TARGET_ARM64) | ||
| #define DAC_CS_NATIVE_DATA_SIZE 24 | ||
| #elif (defined(TARGET_LINUX) || defined(TARGET_ANDROID)) && defined(TARGET_ARM) | ||
| #define DAC_CS_NATIVE_DATA_SIZE 80 | ||
| #elif (defined(TARGET_LINUX) || defined(TARGET_ANDROID)) && defined(TARGET_ARM64) | ||
| #define DAC_CS_NATIVE_DATA_SIZE 104 | ||
| #elif defined(TARGET_LINUX) && defined(TARGET_LOONGARCH64) | ||
| #define DAC_CS_NATIVE_DATA_SIZE 96 | ||
| #elif (defined(TARGET_LINUX) || defined(TARGET_ANDROID)) && defined(TARGET_X86) | ||
| #define DAC_CS_NATIVE_DATA_SIZE 76 | ||
| #elif (defined(TARGET_LINUX) || defined(TARGET_ANDROID)) && defined(TARGET_AMD64) | ||
| #define DAC_CS_NATIVE_DATA_SIZE 96 | ||
| #elif defined(TARGET_LINUX) && defined(TARGET_S390X) | ||
| #define DAC_CS_NATIVE_DATA_SIZE 96 | ||
|
|
@@ -748,4 +748,3 @@ struct T_CRITICAL_SECTION { | |
| #else | ||
| #define T_CRITICAL_SECTION CRITICAL_SECTION | ||
| #endif | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
| #include <linux/memfd.h> | ||
| #include <sys/syscall.h> // __NR_memfd_create | ||
| #define memfd_create(...) syscall(__NR_memfd_create, __VA_ARGS__) | ||
| #elif defined(TARGET_ANDROID) | ||
| #include <sys/syscall.h> // __NR_memfd_create | ||
| #define memfd_create(...) syscall(__NR_memfd_create, __VA_ARGS__) | ||
| #endif // TARGET_LINUX && !MFD_CLOEXEC | ||
| #include "minipal.h" | ||
| #include "minipal/cpufeatures.h" | ||
|
|
@@ -48,12 +51,13 @@ bool VMToOSInterface::CreateDoubleMemoryMapper(void** pHandle, size_t *pMaxExecu | |
|
|
||
| #ifdef TARGET_FREEBSD | ||
| int fd = shm_open(SHM_ANON, O_RDWR | O_CREAT, S_IRWXU); | ||
| #elif defined(TARGET_LINUX) || defined(TARGET_ANDROID) | ||
| int fd = memfd_create("doublemapper", MFD_CLOEXEC); | ||
| #else | ||
| int fd = -1; | ||
|
|
||
| #ifndef TARGET_ANDROID | ||
| // Bionic doesn't have shm_{open,unlink} | ||
| // POSIX fallback | ||
| if (fd == -1) | ||
| { | ||
|
|
@@ -64,11 +68,13 @@ bool VMToOSInterface::CreateDoubleMemoryMapper(void** pHandle, size_t *pMaxExecu | |
| fd = shm_open(name, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW, 0600); | ||
| shm_unlink(name); | ||
| } | ||
| #endif // !TARGET_ANDROID | ||
|
|
||
| if (fd == -1) | ||
| { | ||
| return false; | ||
| } | ||
| #endif | ||
|
|
||
| if (ftruncate(fd, MaxDoubleMappedSize) == -1) | ||
| { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -936,7 +936,8 @@ elseif(CLR_CMAKE_TARGET_HAIKU) | |
| set(DEADLOCK_WHEN_THREAD_IS_SUSPENDED_WHILE_BLOCKED_ON_MUTEX 0) | ||
| set(HAVE_SCHED_OTHER_ASSIGNABLE 1) | ||
| else() # Anything else is Linux | ||
| # LTTNG is not available on Android, so don't error out | ||
| if(NOT HAVE_LTTNG_TRACEPOINT_H AND NOT CLR_CMAKE_TARGET_ANDROID AND FEATURE_EVENT_TRACE) | ||
| unset(HAVE_LTTNG_TRACEPOINT_H CACHE) | ||
| message(FATAL_ERROR "Cannot find liblttng-ust-dev. Try installing liblttng-ust-dev (or the appropriate packages for your platform)") | ||
| endif() | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||
| set(EVENT_MANIFEST ${VM_DIR}/ClrEtwAll.man) | ||||||
|
|
||||||
| # TODO: LTTNG is not supported on Android. We need a solution. | ||||||
|
There was a problem hiding this comment. In lieu of a TODO, please file a github issue and provide a link in the comment. |
||||||
| if(CLR_CMAKE_HOST_LINUX AND NOT CLR_CMAKE_TARGET_ANDROID) | ||||||
|
There was a problem hiding this comment. This should also use the host defines.
Suggested change
|
||||||
| add_subdirectory(lttngprovider) | ||||||
| else() | ||||||
| add_subdirectory(dummyprovider) | ||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2558,6 +2558,8 @@ BOOL MAPMarkSectionAsNotNeeded(LPCVOID lpAddress) | |
| } | ||
|
|
||
| BOOL retval = TRUE; | ||
|
|
||
| #ifndef TARGET_ANDROID | ||
| CPalThread * pThread = InternalGetCurrentThread(); | ||
| InternalEnterCriticalSection(pThread, &mapping_critsec); | ||
| PLIST_ENTRY pLink, pLinkNext = NULL; | ||
|
|
@@ -2588,6 +2590,7 @@ BOOL MAPMarkSectionAsNotNeeded(LPCVOID lpAddress) | |
| } | ||
|
|
||
| InternalLeaveCriticalSection(pThread, &mapping_critsec); | ||
| #endif // TARGET_ANDROID | ||
|
|
||
| TRACE_(LOADER)("MAPMarkSectionAsNotNeeded returning %d\n", retval); | ||
| return retval; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,16 +78,24 @@ | |
| <ItemGroup Condition="('$(TargetsAndroid)' == 'true' or '$(TargetsLinuxBionic)' == 'true') and '$(ANDROID_NDK_ROOT)' != ''"> | ||
| <_CoreClrBuildArg Include="-DCMAKE_TOOLCHAIN_FILE=$(ANDROID_NDK_ROOT)/build/cmake/android.toolchain.cmake"/> | ||
| <_CoreClrBuildArg Include="-DANDROID_NDK=$(ANDROID_NDK_ROOT)"/> | ||
| <_CoreClrBuildArg Include="-DANDROID_STL=c++_static"/> | ||
| <_CoreClrBuildArg Include="-DANDROID_CPP_FEATURES="no-rtti exceptions""/> | ||
|
Comment on lines
-81
to
+82
There was a problem hiding this comment. Does this affect size of native AOT runtime packs? (Build with There was a problem hiding this comment. Yes, it will affect the size, but less than if we included the shared libc++ (which is something we cannot do anyway) and I think we just need to pay the price. NativeAOT will be but one of the supported ways to build the application, we need to keep that in mind. I don't like the fact that CoreCLR needs exceptions (Android runtime doesn't use exceptions nor RTTI), but that's what we have and we need to support it (at least for the foreseeable future). Ideally, for NativeAOT we should be able to link the application .so at the app build time (from a set of .o or .a files), in which case this setting here won't matter at all. There was a problem hiding this comment. My question is whether we could set this in the CMake files instead of as argument to CMake. That way pieces of this can still compile with exceptions turned off instead of globally forcing the VM policy. There was a problem hiding this comment. I'm not sure we can? The variables are used by the Android toolchain definition file - I think they need to be set on the command line for this to work. There was a problem hiding this comment. Yeah this needs to be set at the CMake build level sadly. There's not a supported way to do it per-target. We could likely manually set flags to do it if we figure out how the toolchain file sets flags. |
||
| <_CoreClrBuildArg Include="-DANDROID_PLATFORM=android-$(AndroidApiLevelMin)"/> | ||
| <_CoreClrBuildArg Include="-DANDROID_NATIVE_API_LEVEL=$(AndroidApiLevelMin)"/> | ||
| <_CoreClrBuildArg Condition="'$(Platform)' == 'arm64'" Include="-DANDROID_ABI=arm64-v8a" /> | ||
| <_CoreClrBuildArg Condition="'$(Platform)' == 'arm'" Include="-DANDROID_ABI=armeabi-v7a" /> | ||
| <_CoreClrBuildArg Condition="'$(Platform)' == 'x86'" Include="-DANDROID_ABI=x86" /> | ||
| <_CoreClrBuildArg Condition="'$(Platform)' == 'x64'" Include="-DANDROID_ABI=x86_64" /> | ||
|
|
||
| <!-- | ||
| TODO: No LTTNG on Android, it's disabled at the eventprovider level. Will need an answer. | ||
|
There was a problem hiding this comment. Same as above, please file an issue and add a link to the issue here. |
||
| Disabling FEATURE_EVENT_TRACE is too wide. | ||
| --> | ||
| <!-- | ||
| Unsure if something like this is actually defined. We need this to differentiate between | ||
| features that are enabled in mono android, but not in coreclr. | ||
| --> | ||
| <_CoreClrBuildArg Include="-cmakeargs -DRUNTIME_CORECLR=1" /> | ||
|
Comment on lines
+94
to
+98
There was a problem hiding this comment. Yeah, one spot. Is there a better existing way to do this? There was a problem hiding this comment. Could we add a general feature flag that the Mono vs CoreCLR builds would set? Is this expected to work for the NativeAOT build? |
||
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,7 +60,7 @@ public static TargetOS GetTargetOS(string token) | |
| { | ||
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| return TargetOS.Windows; | ||
| else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.Create("ANDROID"))) | ||
| return TargetOS.Linux; | ||
| else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | ||
| return TargetOS.OSX; | ||
|
|
@@ -81,6 +81,7 @@ public static TargetOS GetTargetOS(string token) | |
| "ios" => TargetOS.iOS, | ||
| "tvossimulator" => TargetOS.tvOSSimulator, | ||
| "tvos" => TargetOS.tvOS, | ||
| "android" => TargetOS.Android, | ||
| _ => throw new CommandLineException($"Target OS '{token}' is not supported") | ||
| }; | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,8 @@ public enum TargetOS | |
| FreeBSD, | ||
| NetBSD, | ||
| SunOS, | ||
| WebAssembly, | ||
| Android | ||
steveisok marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| public enum TargetAbi | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,7 @@ public static MachineOSOverride MachineOSOverrideFromTarget(this TargetDetails t | |
| return MachineOSOverride.Windows; | ||
|
|
||
| case TargetOS.Linux: | ||
| case TargetOS.Android: | ||
| return MachineOSOverride.Linux; | ||
|
|
||
| case TargetOS.OSX: | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,12 +41,19 @@ | |
| <Compile Include="..\..\Common\InstructionSetHelpers.cs" Link="InstructionSetHelpers.cs" /> | ||
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <_LibPrefix Condition="'$(_IsCrossHostPublish)' == 'true'">$(LibPrefix)</_LibPrefix> | ||
| <_LibPrefix Condition="'$(_IsCrossHostPublish)' != 'true'">$(HostLibPrefix)</_LibPrefix> | ||
| <_LibSuffix Condition="'$(_IsCrossHostPublish)' == 'true'">$(LibSuffix)</_LibSuffix> | ||
| <_LibSuffix Condition="'$(_IsCrossHostPublish)' != 'true'">$(HostLibSuffix)</_LibSuffix> | ||
steveisok marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetArchitectureForSharedLibraries Condition="'$(CrossHostArch)' == ''">$(TargetArchitecture)</TargetArchitectureForSharedLibraries> | ||
| <TargetArchitectureForSharedLibraries Condition="'$(CrossHostArch)' != ''">$(CrossHostArch)</TargetArchitectureForSharedLibraries> | ||
| <TargetArchitectureForSharedLibraries Condition="'$(TargetArchitectureForSharedLibraries)'=='armel'">arm</TargetArchitectureForSharedLibraries> | ||
|
|
||
| <JitInterfaceLibraryName>$(_LibPrefix)jitinterface_$(TargetArchitectureForSharedLibraries)$(_LibSuffix)</JitInterfaceLibraryName> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
|
|
@@ -56,7 +63,7 @@ | |
| Link="%(FileName)%(Extension)" | ||
| /> | ||
|
|
||
| <Content Include="$(CoreCLRArtifactsPath)/$(CrossHostArch)/$(_LibPrefix)clrjit_*_$(TargetArchitectureForSharedLibraries)$(_LibSuffix)" | ||
| CopyToOutputDirectory="PreserveNewest" | ||
| CopyToPublishDirectory="PreserveNewest" | ||
| Link="%(FileName)%(Extension)" | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,12 +2,14 @@ | |
| <!-- This project is publishes a self-contained copy of crossgen2. --> | ||
| <PropertyGroup> | ||
| <_IsPublishing>true</_IsPublishing> | ||
| <RuntimeIdentifier Condition="'$(CrossBuild)' == 'true'">$(NETCoreSdkPortableRuntimeIdentifier)</RuntimeIdentifier> | ||
| <RuntimeIdentifier Condition="'$(CrossBuild)' != 'true'">$(PackageRID)</RuntimeIdentifier> | ||
| <PublishDir>$(RuntimeBinDir)crossgen2-published/</PublishDir> | ||
| <SelfContained>true</SelfContained> | ||
| <PublishTrimmed>true</PublishTrimmed> | ||
| <TrimmerSingleWarn>false</TrimmerSingleWarn> | ||
| <AotOrSingleFile>true</AotOrSingleFile> | ||
| <_IsCrossHostPublish>true</_IsCrossHostPublish> | ||
steveisok marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </PropertyGroup> | ||
|
|
||
| <Import Project="crossgen2.props" /> | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be conditioned on CoreCLR so we don't change the Mono build. same in eng/native/build-commons.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be 28? @janvorli tested it recently with 28 https://github.com/dotnet/arcade/blob/88a10ba00ed798ee289d786db9f6339acd682a99/eng/common/cross/build-android-rootfs.sh#L24. We can provide a fallback for that glob API to further lower the requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 29 is necessary because that's the first that does not use emulated TLS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add a run-time check for emulated TLS selection and still lower the requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we add a workaround for that? When I ran things previously we only had that for NativeAOT. Assuming that's true, the build will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emulated TLS works on linux-bionic and has been for a while, so in theory it should work on older Androids, newer ones can use the real TLS. I have recently dealt with Emulated TLS on riscv64 (and later switched to real TLS but still having some issues #111317 (comment)). We can tweak things in asm code to select which path to take
GETTHREAD_ETLS_1vs.INLINE_GETTHREAD. Right now it's under the build conditionFEATURE_EMULATED_TLS.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that there is going to be some amount of work to get this right < API level 29. My preference would be to do any of that work in a follow up.