build: Repair the build on WASI platform (take 3) #5057

kateinoigakukun · 2024-08-07T06:15:53Z

I forgot to tell @MaxDesiatov we need several additional changes other than #5052

kateinoigakukun · 2024-08-07T07:14:39Z

MaxDesiatov · 2024-08-07T12:14:35Z

I merged that one, but that introduced conflicts unfortunately 😢

…bxml2 `find_package(LibXml2 REQUIRED)` sets `LIBXML2_INCLUDE_DIR` to the correct include directory for the libxml2 headers. Use this variable instead of hardcoding `/usr/include/libxml2`. This allows the build to work with custom libxml2 builds on WASI.

This commit disables libdispatch and threads on WASI, and enables wasi-libc emulation features.

Because networking is not a part of WASI Preview 1. We can add it back when it is available.

…uilds Those headers has been imported by Swift side through `CoreFoundation/Base.subproj/SwiftRuntime/CoreFoundation.h` since 44031b5 but we switched to use CoreFoundation.h directly after the recore, and the header was not updated in 44031b5

Treat WASI as an usual Unix-like system

We had been using the vendored BlocksRuntime on WASI, but the build configuration was removed during the recore. This change restores the vendored BlocksRuntime build configuration on WASI.

Mark them available on WASI. Otherwise, the `static inline` implementations are activated and the build fails with multiple definitions.

kateinoigakukun · 2024-08-07T12:25:00Z

Sources/CoreFoundation/CFBundle.c

@@ -600,7 +600,7 @@ CFBundleRef CFBundleGetBundleWithIdentifier(CFStringRef bundleID) {
 #if TARGET_OS_WASI
    hint = NULL;
 #else
-    hint = __builtin_frame_address(0);
+    hint = __builtin_return_address(0);


It was accidentally changed in #5052 We should continue using __builtin_return_address on non-WASI platforms.

kateinoigakukun · 2024-08-07T12:26:13Z

@swift-ci test

CMakeLists.txt

Sources/CoreFoundation/BlockRuntime/CMakeLists.txt

jmschonfeld · 2024-08-07T15:42:38Z

Sources/FoundationXML/XMLParser.swift

@@ -471,7 +471,7 @@ open class XMLParser : NSObject {
    open var allowedExternalEntityURLs: Set<URL>?

 #if os(WASI)
-    private static var _currentParser: XMLParser?
+    private nonisolated(unsafe) static var _currentParser: XMLParser?


This seems unsafe at first look, since any thread can call setCurrentParser and effectively traffic XMLParser between actors. Can you elaborate a bit more on why this is necessary?

WASI preview1 ABI is always single-threaded, so memory access to the same storage should not happen simultaneously. So any data storage should be safe to access from any isolation context on WASI, but 6 lang mode requires explicit marking here.

Perhaps the better question is what does actor isolation mean on WASI where everything is single threaded? This will allow this mutable state to pass between actor isolations on WASI, is that ok because we know it's single threaded? Or should this instead be some form of task-local storage to prevent it from traveling across actors?

We don't need task local storage at all here because there is no suspension point during XMLParser.parse call. No suspension point means no Task switching, thus it's guaranteed that XMLParser.setCurrentParser(self) is not called twice before resetting the storage by XMLParser.setCurrentParser(nil).
The only exception is the case where user provided XMLParserDelegate methods call XMLParser.parse(), but the reentrancy issue is not specific to os(WASI) branch.

Thanks for the explanation. The reentrancy possibility isn't unique to WASI, but its ability to manipulate the underlying nonisolated(unsafe) variable across actor isolations is unique to WASI with this change (on other platforms, that variable is thread-specific - which should probably be marked as unavailable from async for this reason).

The example I'm thinking of is: someone calls XMLParser.parse(), in a delegate callout they create a new detached task on a different actor calling XMLParser.parse() again. At this point, we have two actors manipulating the same underlying nonisolated(unsafe) variable. Typically, that'd be a concurrency violation because two actors are mutating the same shared state. Since WASI is single-threaded, we know that this can't data race in practice, but it's still a violation in the Swift 6 concurrency design and I'm not sure how to rationalize that with the thread model of WASI at the moment (cc @hborla in case you have any insight here)

The example I'm thinking of is: someone calls XMLParser.parse(), in a delegate callout they create a new detached task on a different actor calling XMLParser.parse() again.

Having a closer look, I noticed the problem exists even in non-WASI code path since jobs associated with two different Tasks can be scheduled on the same thread, and in that case two different tasks can manipulate the same thread local variable.

So we had two issues in the existing code, 1. reentrancy issue, 2. sharing mutable storage across Tasks. I'll send a fix for them in another PR and exclude the commit from this PR. Does it make sense?

@jmschonfeld Opened another PR to fix the issues #5061

Sources/_CFXMLInterface/CMakeLists.txt

Sources/CoreFoundation/BlockRuntime/CMakeLists.txt

MaxDesiatov · 2024-08-08T10:26:14Z

@swift-ci test

MaxDesiatov · 2024-08-08T11:11:25Z

@swift-ci test windows

Sources/CoreFoundation/BlockRuntime/CMakeLists.txt

jmschonfeld · 2024-08-08T16:37:03Z

Sources/FoundationXML/XMLParser.swift

@@ -471,7 +471,7 @@ open class XMLParser : NSObject {
    open var allowedExternalEntityURLs: Set<URL>?

 #if os(WASI)
-    private static var _currentParser: XMLParser?
+    private nonisolated(unsafe) static var _currentParser: XMLParser?


Thanks for the explanation. The reentrancy possibility isn't unique to WASI, but its ability to manipulate the underlying nonisolated(unsafe) variable across actor isolations is unique to WASI with this change (on other platforms, that variable is thread-specific - which should probably be marked as unavailable from async for this reason).

The example I'm thinking of is: someone calls XMLParser.parse(), in a delegate callout they create a new detached task on a different actor calling XMLParser.parse() again. At this point, we have two actors manipulating the same underlying nonisolated(unsafe) variable. Typically, that'd be a concurrency violation because two actors are mutating the same shared state. Since WASI is single-threaded, we know that this can't data race in practice, but it's still a violation in the Swift 6 concurrency design and I'm not sure how to rationalize that with the thread model of WASI at the moment (cc @hborla in case you have any insight here)

We accidentally changed it to `__builtin_frame_address` in 7f38264 but we should keep it as `__builtin_return_address` as it was before.

To avoid shippig BlocksRuntime as a separate library, build it as an object library and include it in CoreFoundation static archive.

We ported BlocksRuntime CMakeLists.txt from the state of 02b7d8f but we don't find any reason to set POSITION_INDEPENDENT_CODE=FALSE for BlocksRuntime.

jmschonfeld · 2024-08-08T18:46:04Z

@swift-ci please test

kateinoigakukun · 2024-08-08T21:55:14Z

@swift-ci test Windows

kateinoigakukun mentioned this pull request Aug 7, 2024

Repair swift-corelibs-foundation build swiftwasm/swift#5586

Closed

33 tasks

MaxDesiatov requested review from iCharlesHu and jmschonfeld August 7, 2024 12:13

MaxDesiatov added the WASI WebAssembly System Interface-related changes label Aug 7, 2024

kateinoigakukun added 7 commits August 7, 2024 12:19

[CMake] Disable libdispatch & threads, enable some emulations on WASI

fef1083

This commit disables libdispatch and threads on WASI, and enables wasi-libc emulation features.

[CMake] Exclude FoundationNetworking and _CFURLSessionInterface on WASI

27310f4

Because networking is not a part of WASI Preview 1. We can add it back when it is available.

[wasm] Fix the new CFString.c compilation error on WASI

f91a759

Treat WASI as an usual Unix-like system

[wasm] Build the vendored version of BlocksRuntime on WASI

27efb0c

We had been using the vendored BlocksRuntime on WASI, but the build configuration was removed during the recore. This change restores the vendored BlocksRuntime build configuration on WASI.

[wasm] strlcpy and strlcat are available in wasi-libc

f0a3631

Mark them available on WASI. Otherwise, the `static inline` implementations are activated and the build fails with multiple definitions.

kateinoigakukun force-pushed the yt/repair-wasi-build branch from 916e4bc to 29c01d4 Compare August 7, 2024 12:24

kateinoigakukun commented Aug 7, 2024

View reviewed changes

jmschonfeld reviewed Aug 7, 2024

View reviewed changes

Sources/CoreFoundation/BlockRuntime/CMakeLists.txt Show resolved Hide resolved

MaxDesiatov requested a review from jmschonfeld August 8, 2024 10:26

jmschonfeld reviewed Aug 8, 2024

View reviewed changes

kateinoigakukun added 5 commits August 8, 2024 17:12

[Build] Repair WASI build with SwiftPM

8176d11

Use __builtin_return_address instead of __builtin_frame_address

e5104c3

We accidentally changed it to `__builtin_frame_address` in 7f38264 but we should keep it as `__builtin_return_address` as it was before.

[CMake] Not even try to find libdispatch on WASI

a1b9c39

[CMake] Build BlocksRuntime as object library

965246e

To avoid shippig BlocksRuntime as a separate library, build it as an object library and include it in CoreFoundation static archive.

[CMake] Remove POSITION_INDEPENDENT_CODE=FALSE from BlocksRuntime

3b8485a

We ported BlocksRuntime CMakeLists.txt from the state of 02b7d8f but we don't find any reason to set POSITION_INDEPENDENT_CODE=FALSE for BlocksRuntime.

kateinoigakukun force-pushed the yt/repair-wasi-build branch from c5f47e7 to 3b8485a Compare August 8, 2024 17:12

jmschonfeld approved these changes Aug 8, 2024

View reviewed changes

kateinoigakukun mentioned this pull request Aug 9, 2024

[XMLParser] Fix reentrancy issue around currentParser #5061

Merged

kateinoigakukun merged commit 6450f42 into swiftlang:main Aug 9, 2024
2 of 3 checks passed

kateinoigakukun deleted the yt/repair-wasi-build branch August 9, 2024 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Repair the build on WASI platform (take 3) #5057

build: Repair the build on WASI platform (take 3) #5057

kateinoigakukun commented Aug 7, 2024

kateinoigakukun commented Aug 7, 2024

MaxDesiatov commented Aug 7, 2024

kateinoigakukun Aug 7, 2024 •

edited

Loading

kateinoigakukun commented Aug 7, 2024

jmschonfeld Aug 7, 2024

kateinoigakukun Aug 7, 2024

jmschonfeld Aug 7, 2024

kateinoigakukun Aug 7, 2024

jmschonfeld Aug 8, 2024

kateinoigakukun Aug 8, 2024

kateinoigakukun Aug 9, 2024

MaxDesiatov commented Aug 8, 2024

MaxDesiatov commented Aug 8, 2024

jmschonfeld Aug 8, 2024

jmschonfeld commented Aug 8, 2024

kateinoigakukun commented Aug 8, 2024

build: Repair the build on WASI platform (take 3) #5057

build: Repair the build on WASI platform (take 3) #5057

Conversation

kateinoigakukun commented Aug 7, 2024

kateinoigakukun commented Aug 7, 2024

MaxDesiatov commented Aug 7, 2024

kateinoigakukun Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

kateinoigakukun commented Aug 7, 2024

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

MaxDesiatov commented Aug 8, 2024

MaxDesiatov commented Aug 8, 2024

Choose a reason for hiding this comment

jmschonfeld commented Aug 8, 2024

kateinoigakukun commented Aug 8, 2024

kateinoigakukun Aug 7, 2024 •

edited

Loading