Skip to content

[test] New test mode to execute all tests with C++ interop enabled. #59120

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.

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

Merged
merged 1 commit into from
May 31, 2022

Conversation

drodriguez
Copy link
Contributor

@drodriguez drodriguez commented May 27, 2022

To improve the reliability of the C++ interop, a new test mode is added
to the existing ones. The new mode enables the usage of
--enable-cxx-interop in every test invocation of the frontend and the
driver.

This can be used after a build-script or similar had been successful, and then one can do ninja -C <path/to/build/dir> -- check-swift-validation-with_cxx_interop to execute all tests with C++ interop enabled.

The ideal situation would be that with and without the C++ interop the
test should not fail. Currently around 283 tests seems to still fail.

@drodriguez
Copy link
Contributor Author

@swift-ci please smoke test

@plotfi
Copy link
Contributor

plotfi commented May 27, 2022

Nice work, thanks for posting this @drodriguez

@finagolfin
Copy link
Member

Btw, @drodriguez, any idea why 11 C++ Interoperability tests fail on the Android CI? Seems like it's looking in the wrong place, ie the host linux headers, wonder if you've looked into it:

/usr/include/stdlib.h:25:10: error: 'bits/libc-header-start.h' file not found

I have fixes ready for two other tests that were failing recently on the Android CI that I will submit, but I see 18 more tests that are failing today.

Also, could you get my pull #59001 in? I asked Karoy, but it looks like he's off github this week.

@plotfi
Copy link
Contributor

plotfi commented May 30, 2022

LGTM, @zoecarver @hyp what do you think?

@plotfi plotfi added the c++ interop Feature: Interoperability with C++ label May 31, 2022
test/lit.cfg Outdated
@@ -708,6 +708,10 @@ elif swift_test_mode == 'only_executable':
config.limit_to_features.add("executable_test")
elif swift_test_mode == 'only_non_executable':
config.available_features.add("nonexecutable_test")
elif swift_test_mode == 'with_cxx_interop':
config.available_features.add("with_cxx_interop")
config.swift_frontend_test_options += ' -enable-cxx-interop'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use -enable-expiremental-cxx-interop here and below? I'm trying to get rid of enable-cxx-interop.

Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

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

I don't know much about lit, but this lgtm. Thanks, Daniel!

@plotfi
Copy link
Contributor

plotfi commented May 31, 2022

Probably going to merge this one after @zoecarver provides feedback.

@buttaface can you surface issues with Android CI in the forums or at the next C++-Interop meeting? I think the next one is for covering Windows C++-Interop, so might be a good place to surface cross-platform issues generally.

@zoecarver
Copy link
Contributor

Feel free to merge and fix the "experimental" thing as a nfc after if you don't want to re-run the tests or whatever.

@drodriguez
Copy link
Contributor Author

Btw, @drodriguez, any idea why 11 C++ Interoperability tests fail on the Android CI? Seems like it's looking in the wrong place, ie the host linux headers, wonder if you've looked into it:

/usr/include/stdlib.h:25:10: error: 'bits/libc-header-start.h' file not found

It seems that it is using the system headers instead of the NDK headers. This probably might work in Termux and similar setups, but cannot work cross-compiling from a Linux machine to Android. I do not know exactly how to fix it or what's going on, but I would start looking at those tests, which replacement they use (target-swift-ide-test?), look at their expansions (https://github.com/apple/swift/blob/d775ec8d49a8ae7d82be484da2d172e91837128c/test/lit.cfg#L1640-L1643) and check why they don't use similar tricks as swift_target_frontend to pass the NDK path.

I have fixes ready for two other tests that were failing recently on the Android CI that I will submit, but I see 18 more tests that are failing today.

Also, could you get my pull #59001 in? I asked Karoy, but it looks like he's off github this week.

Done.

To improve the reliability of the C++ interop, a new test mode is added
to the existing ones. The new mode enables the usage of
`--enable-experimental-cxx-interop` in every test invocation of the frontend and the
driver.

The ideal situation would be that with and without the C++ interop the
test should not fail. Currently around 283 tests seems to still fail.
@drodriguez drodriguez force-pushed the test-with-cxx-interop branch from b63d4c8 to ae1f707 Compare May 31, 2022 18:08
@drodriguez
Copy link
Contributor Author

@swift-ci please smoke test and merge

@finagolfin
Copy link
Member

This probably might work in Termux and similar setups

While Termux doesn't have that problem, the same test then fails with errors like these, taken from other failing C++ Interop tests on the Android CI:

/home/ubuntu/android-ndk-r19c/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/local/include/stdlib.h:31:15: note: in file included from /home/ubuntu/android-ndk-r19c/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/local/include/stdlib.h:31:
#include_next <stdlib.h>
              ^
/home/ubuntu/android-ndk-r19c/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/stdlib.h:113:72: error: missing '#include <xlocale.h>'; 'locale_t' must be declared before it is used
unsigned long strtoul_l(const char* __s, char** __end_ptr, int __base, locale_t __l) __INTRODUCED_IN(26);
                                                                       ^
/home/ubuntu/android-ndk-r19c/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/xlocale.h:49:28: note: declaration here is not visible
typedef struct __locale_t* locale_t;
                           ^
<module-includes>:1:10: note: in file included from <module-includes>:1:
#include "move-only.h"
         ^
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/foreign-reference/Inputs/move-only.h:8:14: error: 'operator new' is missing exception specification 'noexcept'
inline void *operator new(size_t, void *p) { return p; }
             ^
/home/ubuntu/android-ndk-r19c/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/c++/v1/new:220:70: note: previous declaration is here
_LIBCPP_NODISCARD_AFTER_CXX17 inline _LIBCPP_INLINE_VISIBILITY void* operator new  (std::size_t, void* __p) _NOEXCEPT {return __p;}
                                                                     ^
<module-includes>:1:10: note: in file included from <module-includes>:1:
#include "move-only.h"
         ^
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/foreign-reference/Inputs/move-only.h:8:40: warning: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified)
inline void *operator new(size_t, void *p) { return p; }
                                       ^
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/foreign-reference/Inputs/move-only.h:8:40: note: insert '_Nullable' if the pointer may be null
inline void *operator new(size_t, void *p) { return p; }
                                       ^
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/foreign-reference/Inputs/move-only.h:8:40: note: insert '_Nonnull' if the pointer should never be null
inline void *operator new(size_t, void *p) { return p; }
                                       ^
<module-includes>:1:10: note: in file included from <module-includes>:1:
#include "move-only.h"
         ^
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/foreign-reference/Inputs/move-only.h:26:12: error: no matching 'operator new' function for non-allocating placement new expression; include <new>
    return new (malloc(sizeof(MoveOnly))) MoveOnly();
           ^
<module-includes>:1:10: note: in file included from <module-includes>:1:
#include "move-only.h"
         ^
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/foreign-reference/Inputs/move-only.h:36:12: error: no matching 'operator new' function for non-allocating placement new expression; include <new>
    return new (malloc(sizeof(HasMoveOnlyChild))) HasMoveOnlyChild();
           ^
<module-includes>:1:10: note: in file included from <module-includes>:1:
#include "move-only.h"
         ^
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/foreign-reference/Inputs/move-only.h:50:12: error: no matching 'operator new' function for non-allocating placement new expression; include <new>
    return new (malloc(sizeof(PrivateCopyCtor))) PrivateCopyCtor();
           ^
<module-includes>:1:10: note: in file included from <module-includes>:1:
#include "move-only.h"
         ^
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/foreign-reference/Inputs/move-only.h:68:12: error: no matching 'operator new' function for non-allocating placement new expression; include <new>
    return new (malloc(sizeof(BadCopyCtor))) BadCopyCtor();
           ^
/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/test/Interop/Cxx/foreign-reference/move-only-irgen.swift:3:8: error: could not build C module 'MoveOnly'
import MoveOnly
       ^

The first one I'm familiar with, Android's Bionic libc only defines locale_t in a separate header xlocale.h, which isn't in Swift's Glibc modulemap. The rest I'm not familiar with, as I avoid C++.

look at their expansions

Yeah, was going to do that, was just hoping you had done it already. 😉 Anyway, those tests will hit these other Interop errors once that header path is fixed.

@plotfi, sure, I just started looking at them, though these foreign-reference tests have been failing on the Android CI since they were added late last year, so I will try a few things then file a github issue with what I find.

@swift-ci swift-ci merged commit 7521101 into swiftlang:main May 31, 2022
@drodriguez drodriguez deleted the test-with-cxx-interop branch May 31, 2022 22:26
drodriguez added a commit that referenced this pull request Jun 3, 2022
…59216)

In #59120 I did not realize that I was filtering tests that required
executable or non-executable features, so many tests were being skipped.

This change should recover both sets of tests and it allows us to test
C++ interop in a lot more cases.

Sadly this raises the number of failing tests with C++ interop to 425.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants