Skip to content

[test] Fix or disable tests for 32-bit platforms #82501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[test] Fix or disable tests for 32-bit platforms #82501

wants to merge 1 commit into from

Conversation

finagolfin
Copy link
Member

Fix one PCM and two IRGen tests that are failing on Android armv7 and disable two SILOptimizer tests that were already failing on other 32-bit platforms.

As part of the upcoming official Android SDK bundle, #80788, we're running the non-executable compiler validation suite for Android armv7 again before including the armv7 Swift stdlib, and I see 10 tests failing in both the trunk 6.3 and 6.2 branches.

This pull fixes or disables half of them, whereas the other half are all C++ Interop constructor tests that fail because they cannot build that C module with size_t issues:

<unknown>:0: error: fatal error encountered while in -verify mode
/home/finagolfin/swift/test/Interop/Cxx/class/constructors-typechecker.swift:3:8: error: unexpected error produced: could not build C module 'Constructors'
import Constructors
       ^
/home/finagolfin/swift/test/Interop/Cxx/class/constructors-typechecker.swift:11:51: error: expected warning not produced
let deletedImplicitly = ConstructorWithParam() // expected-warning {{'init()' is deprecated}}
                                              ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<--- snip irrelevant errors --->
/home/finagolfin/swift/test/Interop/Cxx/class
/home/finagolfin/swift/test/Interop/Cxx/class/Inputs/constructors.h:253:9: error: diagnostic produced elsewhere: 'operator new' takes type size_t ('unsigned int') as first parameter
  void *operator new(size_t, void *ptr) { return ptr; }
        ^
<module-includes>:1:10: note: diagnostic produced elsewhere: in file included from <module-includes>:1:
#include "constructors.h"
         ^
/home/finagolfin/swift/test/Interop/Cxx/class/Inputs/constructors.h:263:8: warning: diagnostic produced elsewhere: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)
  void *operator new(size_t);
       ^
/home/finagolfin/swift/test/Interop/Cxx/class/Inputs/constructors.h:263:8: note: diagnostic produced elsewhere: insert '_Nullable' if the pointer may be null
  void *operator new(size_t);
       ^
/home/finagolfin/swift/test/Interop/Cxx/class/Inputs/constructors.h:263:8: note: diagnostic produced elsewhere: insert '_Nonnull' if the pointer should never be null
  void *operator new(size_t);
       ^
<module-includes>:1:10: note: diagnostic produced elsewhere: in file included from <module-includes>:1:
#include "constructors.h"
         ^
/home/finagolfin/swift/test/Interop/Cxx/class/Inputs/constructors.h:263:9: error: diagnostic produced elsewhere: 'operator new' takes type size_t ('unsigned int') as first parameter
  void *operator new(size_t);
        ^

@egorzhdan or @fahadnayyar, any idea what that 32-bit C++ Interop issue is or should I just disable those five tests for Android armv7?

finagolfin
finagolfin commented Jun 25, 2025
View reviewed changes
test/ClangImporter/pcm-emit-direct-cc1-mode.swift Outdated
@@ -9,7 +9,7 @@
// CHECK-DUMP: Module map file: {{.*[/\\]}}Inputs{{/|\\}}custom-modules{{/|\\}}module.modulemap

// Verify that the clang command-line used is cc1
// RUN: %FileCheck -check-prefix CHECK-CLANG -DTRIPLE=%target-triple %s < %t.diags.txt
// RUN: %FileCheck -check-prefix CHECK-CLANG -DTRIPLE=%module-target-triple %s < %t.diags.txt
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that the target triple is armv7-unknown-linux-androideabi, while the Swift compiler passes along the slightly different module triple armv7-unknown-linux-android to the ClangImporter. I think some other platforms have similar differences, so I don't know why this would be failing on Android armv7 alone, unless it really is the only one on the official CI that varies like this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix failed on macOS, as it passes the full x86_64-apple-macosx13.0 triple to the ClangImporter, not the abbreviated module triple x86_64-apple-macos like Android armv7 does. Since these platforms are doing different things, simply disabled this test for Android armv7 for now.

finagolfin
finagolfin commented Jun 25, 2025
View reviewed changes
test/IRGen/abitypes_arm.swift
@@ -11,6 +11,6 @@ class Foo {
}
}

// armv7: define internal void @makeOne(ptr noalias sret({{.*}}) align 4 %agg.result, float{{( noundef)?}} %f, float{{( noundef)?}} %s)
// armv7: define internal void @makeOne(ptr{{( dead_on_unwind)?}} noalias{{( writable)?}} sret({{.*}}) align 4 %agg.result, float{{( noundef)?}} %f, float{{( noundef)?}} %s)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android armv7 generated define internal void @makeOne(ptr dead_on_unwind noalias writable sret(%struct.One) align 4 %agg.result, float noundef %f, float noundef %s) in its IR instead, so I added those attributes, optionally since I don't know if they will be there on other armv7 platforms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like Clang adds dead_on_unwind for sret args, so they always appear in a combination. I guess we don't need to make it optional. https://github.com/swiftlang/llvm-project/blob/f1607d165cb61ef9f857d76afc6e8a04345bb41a/clang/lib/CodeGen/CGCall.cpp#L2622

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, do you mind approving this pull as is? Don't want to kick off another CI run just for this, and keeping it optional is more flexible anyway.

finagolfin
finagolfin commented Jun 25, 2025
View reviewed changes
test/Interop/Cxx/class/custom-new-operator-irgen.swift
@@ -5,4 +5,4 @@ import CustomNewOperator
var x = callsCustomNew()

// Make sure the definition of `operator new` is emitted.
// CHECK: define {{.*}} @{{_ZnwmPv15container_new_t|"\?\?2@YAPEAX_KPEAXUcontainer_new_t@@@Z"}}
// CHECK: define {{.*}} @{{_Znw(j|m)Pv15container_new_t|"\?\?2@YAPEAX_KPEAXUcontainer_new_t@@@Z"}}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llvm-cxxfilt _ZnwjPv15container_new_t demangles this to operator new(unsigned int, void*, container_new_t), in addition to the operator new(unsigned long, void*, container_new_t) that was already there for 64-bit platforms.

@finagolfin
Copy link
Member Author

@swift-ci smoke test

@finagolfin
Copy link
Member Author

@swift-ci smoke test macos

@finagolfin
[test] Fix or disable tests for 32-bit platforms
51081f6 
Fix two IRGen tests that are failing on Android armv7 and disable eight
ClangImporter, C++ Interop, and SILOptimizer tests, two of which that were
already failing on other 32-bit platforms.
@finagolfin
Copy link
Member Author

Simply disabled the five failing C++ Interop tests and the ClangImporter test for Android armv7 and rebased.

@finagolfin
Copy link
Member Author

@swift-ci smoke test

@finagolfin
Copy link
Member Author

@swift-ci smoke test macos

@finagolfin
Copy link
Member Author

@kateinoigakukun, mind reviewing the two IRGen changes? The rest are just disabling tests for Android armv7, so should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kateinoigakukun kateinoigakukun kateinoigakukun approved these changes

@eeckstein eeckstein Awaiting requested review from eeckstein eeckstein is a code owner

@zoecarver zoecarver Awaiting requested review from zoecarver zoecarver is a code owner

@egorzhdan egorzhdan Awaiting requested review from egorzhdan egorzhdan is a code owner

@Xazax-hun Xazax-hun Awaiting requested review from Xazax-hun Xazax-hun is a code owner

@j-hui j-hui Awaiting requested review from j-hui j-hui is a code owner

@fahadnayyar fahadnayyar Awaiting requested review from fahadnayyar fahadnayyar is a code owner

@susmonteiro susmonteiro Awaiting requested review from susmonteiro susmonteiro is a code owner

@hnrklssn hnrklssn Awaiting requested review from hnrklssn hnrklssn is a code owner

@AnthonyLatsis AnthonyLatsis Awaiting requested review from AnthonyLatsis AnthonyLatsis is a code owner

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@finagolfin @kateinoigakukun