Skip to content

[android] Add more changes to build the compiler #69538

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

Merged
merged 1 commit into from
Dec 15, 2023
Merged

[android] Add more changes to build the compiler #69538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,9 +405,9 @@ set(SWIFT_STDLIB_MSVC_RUNTIME_LIBRARY


if(BRIDGING_MODE STREQUAL "DEFAULT" OR NOT BRIDGING_MODE)
if(CMAKE_BUILD_TYPE STREQUAL "Debug" OR "${CMAKE_SYSTEM_NAME}" STREQUAL "Windows" OR (CMAKE_Swift_COMPILER AND CMAKE_Swift_COMPILER_VERSION VERSION_LESS 5.8))
if(CMAKE_BUILD_TYPE STREQUAL "Debug" OR "${SWIFT_HOST_VARIANT_SDK}" MATCHES "WINDOWS|ANDROID" OR (CMAKE_Swift_COMPILER AND CMAKE_Swift_COMPILER_VERSION VERSION_LESS 5.8))
Copy link
Member Author

Choose a reason for hiding this comment

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

CMAKE_SYSTEM_NAME is not set by build-script when cross-compiling, this repo uses SWIFT_HOST_VARIANT_SDK instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the time CMAKE_SYSTEM_NAME is set by CMake. Scripts only need to provide a value explicitly when cross-compiling (see https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_NAME.html).

I do not think the change here is correct. Before it was checking if the compiler was being build for Windows, and after the changes it is checking if the machine building the compiler is Windows (or Android). This is changing behaviour for some cross-compiling scenarios.

There's at least one check for CMAKE_SYSTEM_NAME = Android. Isn't that something that can be used?

I will recommend recovering the previous line, and adding the match for ANDROID in a different clause to not modify the previously working code:

if(CMAKE_BUILD_TYPE STREQUAL "Debug" OR "${CMAKE_SYSTEM_NAME}" STREQUAL "Windows" OR "${SWIFT_HOST_VARIANT_SDK}" STREQUAL "ANDROID" OR (CMAKE_Swift_COMPILER AND CMAKE_Swift_COMPILER_VERSION VERSION_LESS 5.8))

Copy link
Member Author

Choose a reason for hiding this comment

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

Most of the time CMAKE_SYSTEM_NAME is set by CMake. Scripts only need to provide a value explicitly when cross-compiling

Yep, that's the point I'm making. Suppose you want to cross-compile the Swift compiler from linux to Windows: the current check using CMAKE_SYSTEM_NAME would not work, because we do not set CMAKE_SYSTEM_NAME anywhere in build-script.

I do not think the change here is correct. Before it was checking if the compiler was being build for Windows, and after the changes it is checking if the machine building the compiler is Windows (or Android). This is changing behaviour for some cross-compiling scenarios.

No, that's not how this repo works. This repo only sets SWIFT_HOST_VARIANT_SDK and uses it extensively to implement its own cross-compilation system, without using CMake's built-in cross-compilation support that you refer to.

SWIFT_HOST_VARIANT_SDK does not refer to "the machine building the compiler," but replaces CMAKE_SYSTEM_NAME as the host the compiler should be built for, at least in this repo.

There's at least one check for CMAKE_SYSTEM_NAME = Android. Isn't that something that can be used?

It can be used when natively compiling, because as you noted, the native host can be inferred by CMake then. However, the few places that use CMAKE_SYSTEM_NAME in this repo all break when cross-compiling, for the reasons given above.

Copy link
Contributor

Choose a reason for hiding this comment

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

build-script is just the way the Darwin/Linux CI uses to build Swift, but Windows CI builds, in particular, do not use build-script (it is impossible). Other people use unified builds with LLVM and other ways of kick starting the build and try to avoid build-script.

I would strongly recommend not changing anything related to other systems like Windows, specially when it is really easy to avoid it, like in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

build-script is just the way the Darwin/Linux CI uses to build Swift, but Windows CI builds, in particular, do not use build-script (it is impossible). Other people use unified builds with LLVM and other ways of kick starting the build and try to avoid build-script.

I'm well aware, as I have merged changes to those Windows batch files in this repo before.

I would strongly recommend not changing anything related to other systems like Windows, specially when it is really easy to avoid it, like in this case.

Trust me, I know what I'm doing in this repo. Even those other build setups that use CMAKE_SYSTEM_NAME have to translate that to the right SWIFT_HOST_VARIANT_SDK for this repo, or this repo's build does not work.

I had checked to make sure that translation was done above this line before changing this. I know what I'm doing by now. 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Can I ask you please to not change this?

No, I'm confident in how this compiler build works. If you are not, please take a look at my links to the source to see what I'm talking about.

One can provide different values to CMAKE_SYSTEM_NAME and SWIFT_HOST_VARIANT_SDK.

Sure, and then this compiler build will not work.

The reason that "override" you mention is there is because build-script passes in SWIFT_HOST_VARIANT_SDK to this CMake build, as I linked above. It does not use CMAKE_SYSTEM_NAME. For those who insist on using CMAKE_SYSTEM_NAME instead outside of build-script, the translation to SWIFT_HOST_VARIANT_SDK that I linked you above is used.

Now, I can see how the use of both CMAKE_SYSTEM_NAME and SWIFT_HOST_VARIANT_SDK in this repo can be confusing, as I remarked that both were being used in this repo from my first pull almost five years ago, though most heavy usage is of SWIFT_HOST_VARIANT_SDK alone.

That inconsistent usage happens to work when natively compiling because CMake can easily infer the native host and set CMAKE_SYSTEM_NAME, but it can break when cross-compiling if you don't set both variables correctly.

Now, let's look at the two scenarios, with build-script or without. build-script does not set a CMAKE_SYSTEM_NAME when cross-compiling, so it is safer to check SWIFT_HOST_VARIANT_SDK on this line then. Without build-script, one can do all kinds of nonsense, as you noted, but if you set CMAKE_SYSTEM_NAME alone, the translation above ensures that SWIFT_HOST_VARIANT_SDK is properly defined, so again it is safer to check SWIFT_HOST_VARIANT_SDK here.

I have worked through this logic multiple times, and I'm telling you, this is the right check for this repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not going to insist more on this topic. I am sorry we cannot agree in a solution that works for both cases. If nobody else have problems with this and I am the only one having this problem, feel free to merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

@drodriguez, I'm telling you, this does work for both cases. If you just look elsewhere in the same file, there are other SWIFT_HOST_VARIANT_SDK checks just like this, indicating at least that both styles are used.

If you are worried about breaking something, please just run the CI now, which as we discussed, runs both with and without build-script. That will show that this doesn't break anything.

Copy link
Member

Choose a reason for hiding this comment

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

I think that @drodriguez has a valid point here. Consider the builds for the macOS and the fat runtimes on Apple platforms. These can be different (at least currently). I think that we should prefer the CMake handling for the cross-compilation mechanism and is something that might be a possibility in the future. We shouldn't make this harder for such a future refactoring.

Copy link
Member Author

@finagolfin finagolfin Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Consider the builds for the macOS and the fat runtimes on Apple platforms. These can be different (at least currently).

Every Darwin OS sets SWIFT_HOST_VARIANT_SDK as I'm checking here- see the next 100 lines after that link too- none set CMAKE_SYSTEM_NAME.

I think that we should prefer the CMake handling for the cross-compilation mechanism

I know: you, me, and Daniel have had this argument since my first pull building the compiler for Android almost five years ago. The fact remains that after all these years, most of this main compiler/stdlib repo primarily uses SWIFT_HOST_VARIANT_SDK and with good reason, as we discussed before and here.

and is something that might be a possibility in the future.

It hasn't happened in five years, I don't think it will happen anytime soon.

We shouldn't make this harder for such a future refactoring.

It's a simple search-and-replace, it doesn't make it hard at all. On the contrary, checking CMAKE_SYSTEM_NAME as you did here breaks cross-compilation for every platform that doesn't set CMAKE_SYSTEM_NAME differently when cross-compiling, which is every platform using build-script, ie almost every Swift platform.

As I emphasized from my first pull, you can't check either CMAKE_SYSTEM_NAME or SWIFT_HOST_VARIANT_SDK randomly throughout this compiler codebase, because it will happen to work when natively compiling the compiler but breaks cross-compiling the Swift compiler and stdlib. Sneaking in CMAKE_SYSTEM_NAME checks like this piecemeal actually breaks things, it is not helping matters.

Since I'm probably the only person consistly cross-compiling the Swift compiler from one OS to another (CMake simply uses Darwinfor all Apple platforms, so that wouldn't be considered cross-OS by CMake, I guess), as the Termux package CI cross-compiles from linux x86_64 for Android, I'm well aware of the problems with CMAKE_SYSTEM_NAME, which is why I removed its use for this check.

Now, all that said, I said from my first pull that I don't care whether CMAKE_SYSTEM_NAME or SWIFT_HOST_VARIANT_SDK is used in this repo, but one has to be chosen and used consistently and exclusively. Since this repo is currently a muddled mess, using SWIFT_HOST_VARIANT_SDK heavily but also using CMAKE_SYSTEM_NAME for a minority of checks, I'm switching this to the more well-supported SWIFT_HOST_VARIANT_SDK, which is the only one supported by build-script. As I detailed above, checking SWIFT_HOST_VARIANT_SDK here also works for platforms like Windows that only set CMAKE_SYSTEM_NAME, as the Windows CI run for this pull passing demonstrated.

# In debug builds, to workaround a problem with LLDB's `po` command (rdar://115770255).
# On windows to workaround a build problem.
# On Windows and Android, to workaround a build problem.
Copy link
Member Author

Choose a reason for hiding this comment

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

Pure bridging allows me to work around #60272 for Android.

# If the host Swift version is less than 5.8, use pure mode to workaround a C++ interop compiler crash.
set(BRIDGING_MODE "PURE")
else()
Expand Down Expand Up @@ -864,7 +864,7 @@ endif()

if(SWIFT_BUILD_SWIFT_SYNTAX)
# Only "HOSTTOOLS" is supported in Linux when Swift parser integration is enabled.
if(SWIFT_HOST_VARIANT_SDK MATCHES "LINUX|ANDROID|OPENBSD|FREEBSD" AND NOT BOOTSTRAPPING_MODE STREQUAL "HOSTTOOLS")
if(SWIFT_HOST_VARIANT_SDK MATCHES "LINUX|OPENBSD|FREEBSD" AND NOT BOOTSTRAPPING_MODE STREQUAL "HOSTTOOLS")
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 does not need this override, for example, I use the CROSSCOMPILE mode when cross-compiling the compiler from linux to Android.

message(WARNING "Force setting BOOTSTRAPPING=HOSTTOOLS because Swift parser integration is enabled")
set(BOOTSTRAPPING_MODE "HOSTTOOLS")
endif()
Expand Down Expand Up @@ -1194,6 +1194,7 @@ if(SWIFT_INCLUDE_TOOLS)
message(STATUS " Assertions: ${LLVM_ENABLE_ASSERTIONS}")
message(STATUS " LTO: ${SWIFT_TOOLS_ENABLE_LTO}")
message(STATUS " Bootstrapping: ${BOOTSTRAPPING_MODE}")
message(STATUS " C++ Bridging: ${BRIDGING_MODE}")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

message(STATUS " Swift parser: ${SWIFT_BUILD_SWIFT_SYNTAX}")
message(STATUS "")
else()
Expand Down
8 changes: 7 additions & 1 deletion SwiftCompilerSources/Sources/Basic/Utils.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,14 @@ public extension NoReflectionChildren {

public var standardError = CFileStream(fp: stderr)

#if os(Android) || canImport(Musl)
public typealias FILEPointer = OpaquePointer
#else
public typealias FILEPointer = UnsafeMutablePointer<FILE>
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

I took this from my similar pull for swift-tools-support-core, which Max later used for Musl also.


public struct CFileStream: TextOutputStream {
var fp: UnsafeMutablePointer<FILE>
var fp: FILEPointer

public func write(_ string: String) {
fputs(string, fp)
Expand Down
3 changes: 3 additions & 0 deletions cmake/modules/AddSwift.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,9 @@ function(_add_swift_runtime_link_flags target relpath_to_lib_dir bootstrapping)
endif()
endif()
endif()
if(SWIFT_HOST_VARIANT_SDK MATCHES "LINUX|ANDROID|OPENBSD|FREEBSD" AND SWIFT_USE_LINKER STREQUAL "lld")
target_link_options(${target} PRIVATE "SHELL:-Xlinker -z -Xlinker nostart-stop-gc")
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 is needed since lld 13 on ELF platforms a couple years ago, #60406, which linking the new SwiftSyntax library here triggers.

endif()
endif()

set_property(TARGET ${target} PROPERTY BUILD_WITH_INSTALL_RPATH YES)
Expand Down
2 changes: 1 addition & 1 deletion lib/Demangling/Errors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ static void reportNow(uint32_t flags, const char *message) {
#endif
#if SWIFT_STDLIB_HAS_ASL
asl_log(nullptr, nullptr, ASL_LEVEL_ERR, "%s", message);
#elif defined(__ANDROID__)
#elif defined(__ANDROID__) && !defined(__TERMUX__)
Copy link
Member Author

Choose a reason for hiding this comment

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

Pushing these compilation errors to the system log is not useful, certainly not on Termux.

__android_log_print(ANDROID_LOG_FATAL, "SwiftDemangle", "%s", message);
#endif
}
Expand Down
11 changes: 8 additions & 3 deletions lib/Driver/UnixToolChains.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,15 @@ bool toolchains::GenericUnix::addRuntimeRPath(const llvm::Triple &T,

// Honour the user's request to add a rpath to the binary. This defaults to
// `true` on non-android and `false` on android since the library must be
// copied into the bundle.
// copied into the bundle. An exception is made for the Termux app as it
// builds and runs natively like a Unix environment on Android.
#if defined(__TERMUX__)
bool apply_rpath = true;
#else
bool apply_rpath = !T.isAndroid();
#endif
return Args.hasFlag(options::OPT_toolchain_stdlib_rpath,
options::OPT_no_toolchain_stdlib_rpath,
!T.isAndroid());
options::OPT_no_toolchain_stdlib_rpath, apply_rpath);
}

ToolChain::InvocationInfo
Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/LLVMSupport/ErrorHandling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ void error(const char *prefix, const char *msg, const char *file = nullptr, unsi

#if SWIFT_STDLIB_HAS_ASL
asl_log(nullptr, nullptr, ASL_LEVEL_ERR, "%s", buffer);
#elif defined(__ANDROID__)
#elif defined(__ANDROID__) && !defined(__TERMUX__)
__android_log_print(ANDROID_LOG_FATAL, "SwiftRuntime", "%s", buffer);
#elif defined(_WIN32)
#define STDERR_FILENO 2
Expand Down