[android][test] Fix a handful of tests and disable one CxxToSwiftToCxx bridging test #65968

finagolfin · 2023-05-17T07:27:38Z

Also, make the analogous change to apple/swift-driver#1372, which gets the sanitizer tests working on Android again, and remove the lld_lto feature in the tests, which is now unused.

The first test initially worked, but then something changed in trunk to add the noundef, breaking the test.

The second broke a couple weeks ago because of the last command line added in #65662, as I think those C++ modules don't currently work on Android, so I disabled the test.

@hyp and @drodriguez, this should get the Android CI passing again.

drodriguez · 2023-05-17T20:08:45Z

@swift-ci please test

drodriguez · 2023-05-17T20:10:50Z

I agree with disabling the second test.

Not sure about the noundef, since the check will allow it in every platform, and it might be incorrect for those other platforms. I leave that to decide to others that might have more context.

finagolfin · 2023-05-18T07:27:28Z

@hyp, any idea why the noundef is only added for Android armv7, breaking this first test there?

finagolfin · 2023-05-23T06:03:51Z

@egorzhdan, maybe you can review?

finagolfin · 2023-05-27T20:03:54Z

Rebased and added fix for just-added IRGen/section.swift test from #65901 that is failing on the Android CI:

       67:  .type $s7section2g0_Wz,@object 
       68:  .local $s7section2g0_Wz 
       69:  .comm $s7section2g0_Wz,8,8 
       70:  .hidden $s7section2g0Sivp 
       71:  .type $s7section2g0Sivp,@object 
       72:  .section "__TEXT,__mysection","aw",@progbits,unique,1 
not:41      !~~~~~~~                                               error: no match expected
       73:  .globl $s7section2g0Sivp 
       74:  .p2align 3 
       75: $s7section2g0Sivp: 
       76:  .xword 1 
       77:  .size $s7section2g0Sivp, 8

@kubamracek, I haven't tested this fix, but am just following your lead on the syntax and the test output shown above. Let me know if this should work.

finagolfin · 2023-05-30T05:28:29Z

@drodriguez, can you approve and merge? We've given the C++ Interop people enough time to chime in, meanwhile this will get the Android CI green again.

kubamracek · 2023-06-02T04:24:41Z

test/IRGen/section.swift

@@ -38,6 +38,8 @@
 // ASM-NOT: .section
 // ASM: $s7section3fooyyF:
 // ASM-linux-gnu: .section{{.*}}__TEXT,__mysection
+// ASM-linux-android: .section{{.*}}__TEXT,__mysection
+// ASM-linux-androideabi: .section{{.*}}__TEXT,__mysection


This looks great.

I have now tested this and made sure it works.

drodriguez · 2023-06-02T16:43:09Z

test/Interop/Cxx/class/copy-move-assignment-irgen.swift

@@ -20,7 +20,7 @@ public func copyAssign() {
 // CHECK: call {{void|\%struct.NonTrivialCopyAndCopyMoveAssign\*}} @{{_ZN31NonTrivialCopyAndCopyMoveAssignC1Ev|_ZN31NonTrivialCopyAndCopyMoveAssignC2Ev|"\?\?0NonTrivialCopyAndCopyMoveAssign@@QEAA@XZ"}}(%struct.NonTrivialCopyAndCopyMoveAssign* %[[COPY_INSTANCE:.*]])
 // CHECK: call {{void|\%struct.NonTrivialCopyAndCopyMoveAssign\*}} @{{_ZN31NonTrivialCopyAndCopyMoveAssignC1Ev|_ZN31NonTrivialCopyAndCopyMoveAssignC2Ev|"\?\?0NonTrivialCopyAndCopyMoveAssign@@QEAA@XZ"}}(%struct.NonTrivialCopyAndCopyMoveAssign* %[[COPY_INSTANCE2:.*]])
 // CHECK: call {{void|\%struct.NonTrivialCopyAndCopyMoveAssign\*}} @{{_ZN31NonTrivialCopyAndCopyMoveAssignD1Ev|_ZN31NonTrivialCopyAndCopyMoveAssignD2Ev|"\?\?1NonTrivialCopyAndCopyMoveAssign@@QEAA@XZ"}}(%struct.NonTrivialCopyAndCopyMoveAssign* %[[COPY_INSTANCE]])
-// CHECK: call {{void|\%struct.NonTrivialCopyAndCopyMoveAssign\*}} @{{_ZN31NonTrivialCopyAndCopyMoveAssignC1EOS_|_ZN31NonTrivialCopyAndCopyMoveAssignC2EOS_|_ZN31NonTrivialCopyAndCopyMoveAssignC1ERKS_Tm|_ZN31NonTrivialCopyAndCopyMoveAssignC2ERKS_Tm|"\?\?0NonTrivialCopyAndCopyMoveAssign@@QEAA@AEBU0@@Z"}}(
+// CHECK: call {{(noundef )?}}{{void|\%struct.NonTrivialCopyAndCopyMoveAssign\*}} @{{_ZN31NonTrivialCopyAndCopyMoveAssignC1EOS_|_ZN31NonTrivialCopyAndCopyMoveAssignC2EOS_|_ZN31NonTrivialCopyAndCopyMoveAssignC1ERKS_Tm|_ZN31NonTrivialCopyAndCopyMoveAssignC2ERKS_Tm|"\?\?0NonTrivialCopyAndCopyMoveAssign@@QEAA@AEBU0@@Z"}}(


From what I can read from the LLVM IR manual, noundef should only apply to the %struct, right? Can you confirm that Android is returning the %struct and not void?

If that's the case, to reduce the affected area it should be moved to be only in front of the %struct and not apply to a possible void.

CHECK: call {{void|(noundef )?\%struct.NonTrivialCopyAndCopyMoveAssign\*}}

I am still unsure why Android needs that, and if it is safe to apply to every platform. It seems to be a stricter version of the naked type, but I am not sure. The least we can do is to apply it the type that it should be affecting.

Can you confirm that Android is returning the %struct and not void?

Yep:

%7 = call noundef %struct.NonTrivialCopyAndCopyMoveAssign* @_ZN31NonTrivialCopyAndCopyMoveAssignC2ERKS_Tm(%struct.NonTrivialCopyAndCopyMoveAssign* noundef nonnull returned align 4 dereferenceable(5) %1, %struct.NonTrivialCopyAndCopyMoveAssign* noundef nonnull align 4 dereferenceable(5) %4)

I've made the change you asked for here.

I am still unsure why Android needs that, and if it is safe to apply to every platform.

It isn't applied on Android AArch64, so it is more likely only applied to 32-bit platforms. I'm guessing it would break on linux armv7 too, if anyone were still building that. @colemancda, do you run the compiler validation suite for any 32-bit platforms, even if only on a 64-bit linux host like the Android armv7 CI, and see this failure?

@hyp or @egorzhdan, maybe one of you knows if this is expected.

finagolfin · 2023-06-08T07:31:57Z

Rebased, updated the noundef test, and added a driver change.

finagolfin · 2023-06-08T10:44:35Z

lib/Driver/ToolChains.cpp

+ T.isOSDarwin() ? "darwin" : getPlatformNameForTriple(Triple);
+ if (platformName == "android")
+ platformName = "linux";
+ llvm::sys::path::append(LibPath, "clang", "lib", platformName);


This is a translation of apple/swift-driver#1372, see that pull for more info.

I wonder if we can use llvm::Triple::getOSName instead.

I wonder if we can use llvm::Triple::getOSName instead.

Just looked into this: clang uses ToolChain::getOSLibName() instead, which has this same darwin override. We can't call that without instantiating a clang::ToolChain() though, and given all the ceremony you added to do so elsewhere, that would end up with more lines.

@compnerd, I don't think that clang method is worth invoking, let me know if you disagree.

finagolfin · 2023-06-16T16:43:15Z

@drodriguez, the Android CI no longer builds the stdlib since last week, because #66334 calls process_vm_readv() that was added in Android API 23:

/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/stdlib/public/runtime/CrashHandlerLinux.cpp:665:12: error: use of undeclared identifier 'process_vm_readv'
    return process_vm_readv(memserver_pid, &local, 1, &remote, 1, 0);
           ^
1 error generated.

If I cross-compile the stdlib to API 24, which is what I usually do, I don't see this problem. I think you'll want to update the Android CI to API 23 to remedy this.

Once that's fixed, this pull should get the CI green again, and I can add some doc updates here about the new minimum Android API.

finagolfin · 2023-07-04T22:18:28Z

Rebased and dropped the noundef change for now, as that test was recently changed in #67092 and I'm not sure if it still fails.

@drodriguez, do you want to increase the Android API level tested on the CI to get it building again?

finagolfin · 2023-07-16T19:33:01Z

Rebased, fixed some more tests failing on Android, and updated the noundef change for that test now using opaque pointers.

finagolfin · 2023-07-16T19:39:03Z

test/Interop/Cxx/class/constructors-copy-irgen-android.swift

-// ITANIUM_ARM: call void @_ZN30HasUserProvidedCopyConstructorC2ERKS_(%struct.HasUserProvidedCopyConstructor* [[ARG1_AS_STRUCT]], %struct.HasUserProvidedCopyConstructor* [[ARG2_AS_STRUCT]])
+// ITANIUM_ARM-SAME: (ptr {{.*}}[[ARG0:%.*]], ptr {{.*}}[[ARG1:%.*]], ptr {{.*}}[[ARG2:%.*]])
+// ITANIUM_ARM: call void @_ZN30HasUserProvidedCopyConstructorC2ERKS_(ptr [[ARG0]], ptr [[ARG2]])
+// ITANIUM_ARM: call void @_ZN30HasUserProvidedCopyConstructorC2ERKS_(ptr [[ARG1]], ptr [[ARG2]])


These constructor tests started failing recently and had to be updated for Android similarly to #67092.

finagolfin · 2023-07-16T19:41:41Z

test/Reflection/conformance_descriptors.swift

@@ -13,7 +13,7 @@

 // CHECK: CONFORMANCES:
 // CHECK: =============
-// CHECK-DAG: 16ConformanceCheck10SomeStructV6${{[0-9a-f]*}}yXZ0c6NestedD06${{[0-9a-f]*}}LLV (ConformanceCheck.SomeStruct.(unknown context at ${{[0-9a-f]*}}).(SomeNestedStruct in ${{[0-9a-f]*}})) : ConformanceCheck.MyProto
+// CHECK-DAG: 16ConformanceCheck10SomeStructV{{[56]}}${{[0-9a-f]*}}yXZ0c6NestedD0{{[56]}}${{[0-9a-f]*}}LLV (ConformanceCheck.SomeStruct.(unknown context at ${{[0-9a-f]*}}).(SomeNestedStruct in ${{[0-9a-f]*}})) : ConformanceCheck.MyProto


I don't know why this started failing all of a sudden, as the length of this symbol on Android was 6 and it passed before, but lately it is 5, so this change gets it passing again.

@artemcm, this test is disabled on all other arm64 platforms, is this the reason? Should I just disable it for Android AArch64 too?

finagolfin · 2023-07-24T19:44:23Z

Alright, #67478 got the stdlib building again and turned up some more failing tests in the compiler validation suite. I will look into those and update this pull.

finagolfin · 2023-07-25T13:43:18Z

Rebased and added fixes for remaining tests failing on Android CI, @drodriguez, let me know if you can take a look at this.

finagolfin · 2023-07-25T13:45:40Z

test/IRGen/debug_fragment_merge.sil

@@ -3,7 +3,7 @@
 // Checking the below on 32 bit architectures would be cumbersome: each
 // fragment is 32 bits long, which changes the number of checks as well as the
 // arithmethic on the bounds of each fragment.
-// UNSUPPORTED: OS=watchos
+// UNSUPPORTED: OS=watchos, OS=linux-androideabi


These two debug tests fail on 32-bit arches, as the comment says, so disable both for Android armv7 too.

I assume that this "works" but only because for ARM64 the environment is not EABI, so you end up with the OS of linux-android and that would not match?

Yes, linux-androideabi only matches armv7, so this test is only disabled for that 32-bit arch.

I've removed these debug_fragment test changes, as more general requirements are being put in by others in #67699.

finagolfin · 2023-07-25T13:48:05Z

test/Interop/Cxx/templates/class-template-non-type-parameter-irgen.swift

-// CHECK: @"${{s4main1pSo0034MagicArrayInt32_UInt_2_zoAFhhiEngcVvp|s4main1pSo0036MagicArrayInt32_UInt64_2_JsAEiFiuomcVvp}}"
-// CHECK: @"${{s4main1tSo0034MagicArrayInt32_UInt_3_zoAFhhiEngcVvp|s4main1tSo0036MagicArrayInt32_UInt64_3_JsAEiFiuomcVvp}}"
+// CHECK: @"${{s4main1pSo0034MagicArrayInt32_UInt_2_zoAFhhiEngcVvp|s4main1pSo0036MagicArrayInt32_UInt(64|32)_2_JsAEiFiuomcVvp}}"
+// CHECK: @"${{s4main1tSo0034MagicArrayInt32_UInt_3_zoAFhhiEngcVvp|s4main1tSo0036MagicArrayInt32_UInt(64|32)_3_JsAEiFiuomcVvp}}"


This new test fails on 32-bit arches like Android armv7, but these two changes got it passing there too.

@egorzhdan, you added this new test recently, please review this change to get it working on 32-bit platforms.

finagolfin · 2023-07-25T13:56:52Z

test/Interpreter/llvm_link_time_opt.swift

@@ -1,7 +1,7 @@
 // UNSUPPORTED: OS=windows-msvc
 // static library is not well supported yet on Windows

-// REQUIRES: lld_lto
+// XFAIL: OS=linux-android, OS=linux-androideabi


This started failing recently on the community Android CI, as the LLVM 14 toolchain in the Android LTS NDK 25c doesn't support opaque pointers much:

error: Opaque pointers are only supported in -opaque-pointers mode (Producer: 'LLVM13.0.0git' Reader: 'LLVM 14.0.6git')

That flag wasn't added till clang 15, llvm/llvm-project@d69e9f9d89783, so this won't work till the next NDK, which will include clang 17. It currently works natively in the Termux app, where I use LLVM 16.

finagolfin · 2023-07-25T14:09:42Z

test/lit.cfg

@@ -501,8 +501,6 @@ backtrace_on_crash = lit_config.params.get('backtrace_on_crash', None)
 if backtrace_on_crash is not None:
 config.environment['SWIFT_BACKTRACE'] = 'enable=on'

-config.available_features.add('lld_lto')


This was used to avoid issues on Android by @kateinoigakukun in #32430, but I enabled it for Android too when the NDK switched to lld, #39921. I left this feature in then because I didn't know if anyone else needed it, but looking into that commit history for the first time now, it should be fine to remove this feature, since it's always enabled now.

finagolfin · 2023-07-26T16:34:28Z

@egorzhdan, it's been a couple months since the CI was run on this, would you rerun?

egorzhdan · 2023-07-26T19:47:53Z

@swift-ci please smoke test

egorzhdan · 2023-07-26T19:48:03Z

@swift-ci please test

finagolfin · 2023-08-01T12:51:09Z

@compnerd, @drodriguez appears to be away, would you review?

compnerd · 2023-08-01T14:34:41Z

lib/Driver/ToolChains.cpp

- : getPlatformNameForTriple(T));
+ auto platformName =
+ T.isOSDarwin() ? "darwin" : getPlatformNameForTriple(Triple);
+ if (platformName == "android")


Would you mind using T.isOSAndroid() instead for this?

Sure, T.isAndroid() is equivalent, fine by me, will make that change.

@compnerd, made this change, let me know if it's okay in latest commit.

compnerd · 2023-08-01T14:35:47Z

test/IRGen/debug_fragment_merge.sil

@@ -3,7 +3,7 @@
 // Checking the below on 32 bit architectures would be cumbersome: each
 // fragment is 32 bits long, which changes the number of checks as well as the
 // arithmethic on the bounds of each fragment.
-// UNSUPPORTED: OS=watchos
+// UNSUPPORTED: OS=watchos, OS=linux-androideabi


I assume that this "works" but only because for ARM64 the environment is not EABI, so you end up with the OS of linux-android and that would not match?

finagolfin · 2023-08-05T21:48:06Z

@drodriguez, I see you're active again lately, any feedback on this pull? Just needs one last CI run and can be merged, should get the Android CI green again.

…x bridging test Also, make the analogous change to apple/swift-driver#1372, which gets the sanitizer tests working on Android again, and remove the lld_lto feature in the tests, which is now unused.

finagolfin · 2023-08-12T13:41:36Z

Rebased and added a fix for a newly enabled C++ Interop test that fails on Android.

finagolfin · 2023-08-12T13:47:52Z

test/Interop/Cxx/stdlib/use-cxxstdlib-types-in-module-interface.swift

-// CHECK: func getFRTItems() -> std{{\.__1\.|\.}}vector<FRTType, allocator<FRTType>>
-// CHECK: func getPODPtrItems() -> std{{\.__1\.|\.}}vector<UnsafePointer<SimplePOD>, allocator<UnsafePointer<SimplePOD>>>
-// CHECK: func getMutPODPtrItems() -> std{{\.__1\.|\.}}vector<UnsafeMutablePointer<SimplePOD>, allocator<UnsafeMutablePointer<SimplePOD>>>
+// CHECK: func getPODItems() -> std{{\.__(ndk)?1\.|\.}}vector<SimplePOD, allocator<SimplePOD>>


Otherwise fails on Android with this error:

/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/stdlib/use-cxxstdlib-types-in-module-interface.swift:43:11: error: CHECK: expected string not found in input // CHECK: func getPODItems() -> std{{\.__1\.|\.}}vector<SimplePOD, allocator<SimplePOD>> ^ <stdin>:1:1: note: scanning from here ^ <stdin>:16:2: note: possible intended match here func getPODItems() -> std.__ndk1.vector<SimplePOD, allocator<SimplePOD>> ^

finagolfin · 2023-08-16T03:45:15Z

@bnbarham, please run the CI on this.

bnbarham · 2023-08-16T17:37:10Z

@swift-ci please test

finagolfin · 2023-08-17T04:34:44Z

@compnerd, please merge if you're okay with the two changes I made since you approved, which are called out above.

compnerd

Trying to simplify getClangLibraryPath might be a nice follow up change.

compnerd · 2023-08-17T15:45:58Z

lib/Driver/ToolChains.cpp

+ T.isOSDarwin() ? "darwin" : getPlatformNameForTriple(Triple);
+ if (platformName == "android")
+ platformName = "linux";
+ llvm::sys::path::append(LibPath, "clang", "lib", platformName);


I wonder if we can use llvm::Triple::getOSName instead.

finagolfin · 2023-08-17T17:10:50Z

Yay, the community Android CI is green again.

finagolfin requested review from zoecarver, hyp and egorzhdan as code owners May 17, 2023 07:27

finagolfin force-pushed the droid branch from a3c546a to 43dbbcd Compare May 27, 2023 19:57

kubamracek reviewed Jun 2, 2023

View reviewed changes

drodriguez reviewed Jun 2, 2023

View reviewed changes

finagolfin force-pushed the droid branch from 43dbbcd to 81662fe Compare June 8, 2023 07:30

finagolfin requested a review from artemcm as a code owner June 8, 2023 07:30

finagolfin force-pushed the droid branch from 81662fe to a000b26 Compare June 8, 2023 10:43

finagolfin commented Jun 8, 2023

View reviewed changes

finagolfin mentioned this pull request Jun 21, 2023

[Docs][Android] Fix minimal Android API level #66775

Closed

finagolfin force-pushed the droid branch from a000b26 to b309476 Compare July 4, 2023 22:11

finagolfin force-pushed the droid branch from b309476 to 6d1a714 Compare July 16, 2023 19:28

finagolfin changed the title ~~[android][test] Fix new C++ interop test on Android armv7 and new IRGen test, and disable CxxToSwiftToCxx bridging test that now fails on Android~~ [android][test] Fix a handful of tests and disable one CxxToSwiftToCxx bridging test Jul 16, 2023

finagolfin commented Jul 16, 2023

View reviewed changes

finagolfin force-pushed the droid branch from 6d1a714 to 8c1dd08 Compare July 25, 2023 13:39

finagolfin requested a review from MaxDesiatov as a code owner July 25, 2023 13:39

finagolfin requested a review from etcwilde as a code owner July 25, 2023 13:39

finagolfin commented Jul 25, 2023

View reviewed changes

compnerd approved these changes Aug 1, 2023

View reviewed changes

finagolfin force-pushed the droid branch 2 times, most recently from 9c20155 to 1bc774b Compare August 3, 2023 09:10

[android][test] Fix a handful of tests and disable one CxxToSwiftToCx…

30ba49e

…x bridging test Also, make the analogous change to apple/swift-driver#1372, which gets the sanitizer tests working on Android again, and remove the lld_lto feature in the tests, which is now unused.

finagolfin force-pushed the droid branch from 1bc774b to 30ba49e Compare August 12, 2023 11:08

finagolfin commented Aug 12, 2023

View reviewed changes

compnerd approved these changes Aug 17, 2023

View reviewed changes

compnerd merged commit 39a47a0 into apple:main Aug 17, 2023
5 checks passed

finagolfin deleted the droid branch August 17, 2023 17:10

[android][test] Fix a handful of tests and disable one CxxToSwiftToCxx bridging test #65968

[android][test] Fix a handful of tests and disable one CxxToSwiftToCxx bridging test #65968

Conversation

finagolfin commented May 17, 2023 • edited

drodriguez commented May 17, 2023

drodriguez commented May 17, 2023

finagolfin commented May 18, 2023

finagolfin commented May 23, 2023

finagolfin commented May 27, 2023

finagolfin commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin commented Jun 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin commented Jun 16, 2023

finagolfin commented Jul 4, 2023

finagolfin commented Jul 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin commented Jul 24, 2023

finagolfin commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin Jul 25, 2023 • edited

Choose a reason for hiding this comment

finagolfin commented Jul 26, 2023

egorzhdan commented Jul 26, 2023

egorzhdan commented Jul 26, 2023

finagolfin commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin commented Aug 5, 2023

finagolfin commented Aug 12, 2023

Choose a reason for hiding this comment

finagolfin commented Aug 16, 2023

bnbarham commented Aug 16, 2023

finagolfin commented Aug 17, 2023

compnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin commented Aug 17, 2023

finagolfin commented May 17, 2023 •

edited

finagolfin Jul 25, 2023 •

edited