[WebAssembly] Add ifdefs for the WASI target #29530

MaxDesiatov · 2020-01-29T16:55:35Z

Disables/enables stdlib and IRGen code appropriately when targeting WebAssembly/WASI.

This is a part of SR-9307 and #24684.

compnerd

This is a huge change. Please split this up into more focused changes. There can be a single change for the runtime, and a single change for the standard library at least.

include/swift/SwiftRemoteMirror/Platform.h

compnerd · 2020-01-29T17:23:13Z

lib/IRGen/IRGen.cpp

@@ -165,6 +165,12 @@ swift::getIRTargetOptions(const IRGenOptions &Opts, ASTContext &Ctx) {
  TargetOpts.FunctionSections = Opts.FunctionSections;

  auto *Clang = static_cast<ClangImporter *>(Ctx.getClangModuleLoader());
+
+  // WebAssembly doesn't support atomics yet, see https://bugs.swift.org/browse/SR-12097
+  if (Clang->getTargetInfo().getTriple().isOSBinFormatWasm()) {


Remove the extraneous braces. IIRC, we set the ThreadModel for cygwin/MinGW as well. Can we please set the thread model at one site?

As far as I'm aware this is a single place where ThreadModel is used in the whole Swift project, it's not set for Cygwin anywhere. The only somewhat related settings for Cygwin that I see are set in createTargetMachine, but that doesn't give access to TargetOpts:

if (EffectiveTriple.isArch64Bit() && EffectiveTriple.isWindowsCygwinEnvironment()) cmodel = CodeModel::Large;

stdlib/private/StdlibUnittest/InterceptTraps.cpp

stdlib/private/StdlibUnittest/RaceTest.swift

compnerd · 2020-01-29T17:29:45Z

stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift

 import Glibc
 #elseif os(Windows)
 import MSVCRT
 import WinSDK
 #endif

 internal func _signalToString(_ signal: Int) -> String {
+  #if os(WASI)
+  // no signals support on WASI yet, see https://github.com/WebAssembly/WASI/issues/166
+  fatalError("\(#function) is not supported on WebAssembly/WASI")


I don't think that you should stub out _signalToString. Instead, you need to die earlier at the point where the signal is being handled. Just drop the entire function on WASI.

compnerd · 2020-01-29T17:30:06Z

stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift

+// WASI doesn't support child processes
+public func spawnChild(_ args: [String])
+  -> (pid: pid_t, stdinFD: CInt, stdoutFD: CInt, stderrFD: CInt) {
+  preconditionFailure("\(#function) is not supported on WebAssembly/WASI")


This should be fatalError, not preconditionFailure.

compnerd · 2020-01-29T17:30:11Z

stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift

+  preconditionFailure("\(#function) is not supported on WebAssembly/WASI")
+}
+public func posixWaitpid(_ pid: pid_t) -> ProcessTerminationStatus {
+  preconditionFailure("\(#function) is not supported on WebAssembly/WASI")


compnerd · 2020-01-29T17:31:19Z

stdlib/private/SwiftPrivateLibcExtras/SwiftPrivateLibcExtras.swift

@@ -125,6 +125,8 @@ public func _stdlib_pipe() -> (readEnd: CInt, writeEnd: CInt, error: CInt) {
  let ret = fds.withUnsafeMutableBufferPointer { unsafeFds -> CInt in
 #if os(Windows)
    return _pipe(unsafeFds.baseAddress, 0, 0)
+#elseif os(WASI)
+    preconditionFailure("no pipes available on WebAssembly/WASI")


IIRC, this is largely used for the reflection tests. Perhaps we should just disable the reflection tests instead. I really think that given the amount of stuff not available in WASI libc yet, the focus should be improving that first (which is why I stopped and started looking at WASI libc instead).

This is used in RaceTest.swift in the implementation of _InterruptibleSleep on non-Windows platforms. Would _InterruptibleSleep need a dedicated implementation for WASI instead? Could WebAssembly/WASI#77 be somewhat related?

compnerd · 2020-01-29T17:32:07Z

stdlib/private/SwiftPrivateThreadExtras/ThreadBarriers.swift

@@ -12,7 +12,7 @@

 #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
 import Darwin
-#elseif os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Cygwin) || os(Haiku)
+#elseif os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Cygwin) || os(Haiku) || os(WASI)


I think that it might be better to just not build SwiftPrivateThreadExtras on WASI

This would make RaceTest that uses SwiftPrivateThreadExtras disabled, requiring us to disable quite a few tests. Do you think that be worth it?

+1 to disabling the whole module, if it leads to simpler code. Race tests would not work (and are pointless) without threads anyway.

gribozavr · 2020-01-29T17:24:54Z

stdlib/private/StdlibUnittest/InterceptTraps.cpp

-#endif
+#endif // _WIN32
+
+#endif // __wasi__


I think the endif should be before the definition of VectoredCrashHandler.

gribozavr · 2020-01-29T17:26:01Z

stdlib/private/StdlibUnittest/RaceTest.swift

@@ -562,7 +562,12 @@ class _InterruptibleSleep {
      return
    }

+    #if os(WASI)
+    // WebAssembly/WASI on wasm32 is the only 32-bit platform with Int64 time_t


Everywhere throughout this patch, I'd appreciate periods at the end of comments.

gribozavr · 2020-01-29T17:27:22Z

stdlib/private/StdlibUnittest/RaceTest.swift

@@ -562,7 +562,12 @@ class _InterruptibleSleep {
      return
    }

+    #if os(WASI)
+    // WebAssembly/WASI on wasm32 is the only 32-bit platform with Int64 time_t
+    var timeout = timeval(tv_sec: time_t(duration), tv_usec: 0)


I think the code would still compile on other platforms if you the time_t(duration) variant everywhere (no conditional compilation).

gribozavr · 2020-01-29T17:46:14Z

stdlib/private/SwiftPrivateThreadExtras/SwiftPrivateThreadExtras.swift

@@ -98,6 +98,9 @@ public func _stdlib_thread_create_block<Argument, Result>(
  } else {
    return (0, ThreadHandle(bitPattern: threadID))
  }
+#elseif os(WASI)
+  // WASI environment has only a single thread, see https://bugs.swift.org/browse/SR-12097
+  return (0, nil)


pthread_create returns 0 on success. Please return an appropriate errno, like ENOTSUP.

gribozavr · 2020-01-29T17:48:32Z

stdlib/private/SwiftPrivateThreadExtras/SwiftPrivateThreadExtras.swift

@@ -128,6 +131,9 @@ public func _stdlib_thread_join<Result>(
  } else {
    return (CInt(result), nil)
  }
+#elseif os(WASI)
+  // WASI environment has only a single thread, see https://bugs.swift.org/browse/SR-12097
+  return (0, nil)


Ditto, please return EINVAL.

gribozavr · 2020-01-29T17:49:15Z

stdlib/private/SwiftPrivateThreadExtras/ThreadBarriers.swift

@@ -35,7 +35,9 @@ public struct _stdlib_thread_barrier_t {
  var cond: UnsafeMutablePointer<CONDITION_VARIABLE>?
 #elseif os(Cygwin) || os(FreeBSD)
  var mutex: UnsafeMutablePointer<pthread_mutex_t?>?
-  var cond: UnsafeMutablePointer<pthread_cond_t?>?
+  var cond: UnsafeMutablePointer<pthread_cond_t?>


I think we lost a question mark on this line.

gribozavr · 2020-01-29T17:51:59Z

stdlib/private/SwiftPrivateThreadExtras/ThreadBarriers.swift

-  var cond: UnsafeMutablePointer<pthread_cond_t?>?
+  var cond: UnsafeMutablePointer<pthread_cond_t?>
+#elseif os(WASI)
+  // WASI environment has a only single thread, see https://bugs.swift.org/browse/SR-12097


It seems like the code will pretend to succeed, which does not sound great. Can we produce an error?

Does this comment refer to some function? It's currently positioned within a struct property declaration, do you mean that the structs init should invoke fatalError or preconditionFailure when it runs on WASI?

Sorry about sloppy commenting -- yes, I was trying to attach the comment to the init, and invoking fatalError is exactly what I meant.

gribozavr · 2020-01-29T17:57:35Z

stdlib/public/core/Builtin.swift

@@ -379,7 +379,7 @@ internal var _objectPointerLowSpareBitShift: UInt {
 }

 #if arch(i386) || arch(arm) || arch(powerpc64) || arch(powerpc64le) || arch(
-  s390x)
+  s390x) || arch(wasm32)


Maybe group wasm32 together with the other 32-bit platforms? (That is, i386, arm, wasm32, and then everything else.)

kateinoigakukun · 2020-01-30T04:34:50Z

@MaxDesiatov Could you reflect this change also for WASI? #29536
This prevents from building WASI target stdlib

build log

stdlib/private/StdlibUnittest/SymbolLookup.swift

stdlib/private/SwiftPrivateLibcExtras/Subprocess.c

stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift

gribozavr · 2020-01-30T15:41:52Z

stdlib/private/SwiftPrivateThreadExtras/ThreadBarriers.swift

@@ -12,7 +12,7 @@

 #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
 import Darwin
-#elseif os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Cygwin) || os(Haiku)
+#elseif os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Cygwin) || os(Haiku) || os(WASI)


+1 to disabling the whole module, if it leads to simpler code. Race tests would not work (and are pointless) without threads anyway.

gribozavr · 2020-01-30T15:44:03Z

stdlib/public/Platform/Platform.swift

@@ -380,7 +382,7 @@ public var SEM_FAILED: UnsafeMutablePointer<sem_t>? {
 #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
  // The value is ABI.  Value verified to be correct for OS X, iOS, watchOS, tvOS.
  return UnsafeMutablePointer<sem_t>(bitPattern: -1)
-#elseif os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Cygwin) || os(Haiku)
+#elseif os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Cygwin) || os(Haiku) || os(WASI)


Does sem_open() exist and work on WASI? If not, ifdef out the whole variable.

It does exist and whole semaphore.h is available too.

gribozavr · 2020-01-30T15:45:34Z

stdlib/public/core/AtomicInt.swift.gyb

@@ -65,7 +65,7 @@ internal func _swift_stdlib_atomicCompareExchangeStrongInt(
  object target: UnsafeMutablePointer<Int>,
  expected: UnsafeMutablePointer<Int>,
  desired: Int) -> Bool {
-#if arch(i386) || arch(arm)
+#if arch(i386) || arch(arm) || arch(wasm32)


Other comments that you left say that WebAssembly has no atomics right now. So how do these functions get compiled? Regular loads and stores?

Yes, this is set in IRGen.cpp change in this same PR:

if (Clang->getTargetInfo().getTriple().isOSBinFormatWasm()) TargetOpts.ThreadModel = llvm::ThreadModel::Single;

MaxDesiatov · 2020-02-04T21:15:27Z

Hi @gribozavr @compnerd I've addressed your feedback and added comments with clarifications, could you have another look. Thank you!

gribozavr · 2020-02-06T16:58:35Z

I don't see where in the patch you disable building SwiftPrivateThreadExtras on WASI...

If you want to submit this change as an incremental improvement, LGTM, but just noting that it seems that some work is still needed to disable building some modules.

MaxDesiatov · 2020-02-06T18:06:10Z

@gribozavr in stdlib/private/StdlibUnittest/CMakeLists.txt there's no SWIFT_MODULE_DEPENDS_WASI yet, but when it's there it won't specify SwiftPrivateThreadExtras to exclude that dependency. SWIFT_MODULE_DEPENDS_WASI implementation would come in a future PR that updates the build system and CMake code.

Would you mind triggering CI for this PR? Thanks again for your help.

gribozavr · 2020-02-07T16:54:35Z

@MaxDesiatov That will avoid linking the StdlibUnittest against SwiftPrivateThreadExtras, but still won't exclude SwiftPrivateThreadExtras from being built, I believe. PTAL at TARGET_SDKS as used in stdlib/public/Platform/CMakeLists.txt, for example.

gribozavr · 2020-02-07T16:55:07Z

@swift-ci Please test

swift-ci · 2020-02-07T17:45:14Z

Build failed
Swift Test Linux Platform
Git Sha - 1d7e592ae5fe216a342c1ba3b874d126cd61ee1f

swift-ci · 2020-02-07T18:06:20Z

Build failed
Swift Test OS X Platform
Git Sha - 1d7e592ae5fe216a342c1ba3b874d126cd61ee1f

MaxDesiatov · 2020-02-09T14:06:41Z

@gribozavr I've updated CMakeLists.txt to use the TARGET_SDKS argument instead of SWIFT_MODULE_DEPENDS, also rebased the PR on top of master, as the CI issue seems to be caused by the branch being too out of sync:

swiftpm/Sources/PackageLoading/Diagnostics.swift:11:8: error: 
compiled module was created by a newer version of the compiler: 
buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/tsc/swift/TSCBasic.swiftmodule

Could you trigger another CI run? Thanks

gribozavr · 2020-02-12T12:20:00Z

I think we need both TARGET_SDKS (to prevent the module from being built when you build everything that's available), and SWIFT_MODULE_DEPENDS (to prevent other modules from depending on it when it is not available.)

MaxDesiatov · 2020-02-12T13:30:10Z

@gribozavr yeah, it does have both, I've restored SWIFT_MODULE_DEPENDS to its original state. As far as I understand, when SWIFT_MODULE_DEPENDS_WASI is available we'd just add that with a corresponding setting, while TARGET_SDKS should be enough for now to avoid building it for WASI.

gribozavr · 2020-02-13T12:39:08Z

@swift-ci Please test

gribozavr · 2020-02-13T12:42:20Z

@swift-ci Please smoke test

MaxDesiatov · 2020-02-13T15:06:21Z

Thanks for triggering CI! As it has passed and PR got the approval, can it be merged now to avoid potential conflicts appearing and preventing it from being merged as the diff is big enough?

stephentyrone · 2020-02-13T18:46:49Z

In StringObject.swift, you seem to be missing arch(wasm32) in the very first conditional, defining the string layout. If that's the case, am I correct in believing that none of this is actually tested for wasm?

MaxDesiatov · 2020-02-13T18:59:52Z

Do you mean line 80 that precedes internal enum Variant? That one has a corresponding #else which does the job for other platforms in my understanding, but I can't confirm that it works for WASI at the moment. Will need to have a closer look if some of our failing tests have any relation to it, we are slowly enabling the test suite here in swiftwasm#12 and swiftwasm#30. If it does fail, I'd be happy to fix it in a follow-up PR.

stephentyrone · 2020-02-13T19:02:49Z

Yes, that one; the fields of a _StringObject are radically different on 32-bit platforms than they are on 64-bit platforms; if you have the wrong definition (as I think you do), it shouldn't even build.

MaxDesiatov · 2020-02-13T19:31:24Z

Well, it does build it seems, but thanks for having a look, I'll keep that in mind 👍

Disabled by mistake in apple#29530.

MaxDesiatov requested a review from compnerd January 29, 2020 16:55

compnerd requested changes Jan 29, 2020

View reviewed changes

gribozavr reviewed Jan 29, 2020

View reviewed changes

MaxDesiatov force-pushed the swiftwasm-ifdefs branch 2 times, most recently from e3c7637 to 50e5c4a Compare January 29, 2020 22:28

MaxDesiatov force-pushed the swiftwasm-ifdefs branch 2 times, most recently from aa2d6e6 to a71ae92 Compare January 30, 2020 14:50

MaxDesiatov requested review from gribozavr and compnerd January 30, 2020 14:51

MaxDesiatov force-pushed the swiftwasm-ifdefs branch from a71ae92 to 0fa22c4 Compare January 30, 2020 15:42

gribozavr reviewed Jan 30, 2020

View reviewed changes

MaxDesiatov force-pushed the swiftwasm-ifdefs branch 2 times, most recently from b11dec8 to b6e79b0 Compare January 30, 2020 18:01

MaxDesiatov requested a review from gribozavr January 30, 2020 18:26

MaxDesiatov force-pushed the swiftwasm-ifdefs branch from b6e79b0 to 1d7e592 Compare January 30, 2020 20:53

gribozavr approved these changes Feb 6, 2020

View reviewed changes

MaxDesiatov force-pushed the swiftwasm-ifdefs branch from 1d7e592 to 31f38f4 Compare February 8, 2020 07:36

[WebAssembly] Add ifdefs for the WASI target

6729790

MaxDesiatov force-pushed the swiftwasm-ifdefs branch from 31f38f4 to 6729790 Compare February 8, 2020 21:46

gribozavr merged commit 3566e6e into apple:master Feb 13, 2020

MaxDesiatov deleted the swiftwasm-ifdefs branch February 13, 2020 16:46

MaxDesiatov mentioned this pull request Feb 13, 2020

[154] Issue #154 - February 27, 2020 SwiftWeekly/swiftweekly.github.io#500

Closed

drodriguez added a commit to drodriguez/swift that referenced this pull request Feb 13, 2020

[android] Enable SwiftPrivateThreadExtras to build in Android.

5e05ce5

Disabled by mistake in apple#29530.

drodriguez mentioned this pull request Feb 13, 2020

[android] Enable SwiftPrivateThreadExtras to build in Android. #29825

Merged

MaxDesiatov mentioned this pull request Feb 17, 2020

Upstream all the changes 🙌 swiftwasm/swift#187

Closed

78 tasks

[WebAssembly] Add ifdefs for the WASI target #29530

[WebAssembly] Add ifdefs for the WASI target #29530

Conversation

MaxDesiatov commented Jan 29, 2020

compnerd left a 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

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

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

Choose a reason for hiding this comment

kateinoigakukun commented Jan 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov Jan 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov commented Feb 4, 2020

gribozavr commented Feb 6, 2020

MaxDesiatov commented Feb 6, 2020

gribozavr commented Feb 7, 2020

gribozavr commented Feb 7, 2020

swift-ci commented Feb 7, 2020

swift-ci commented Feb 7, 2020

MaxDesiatov commented Feb 9, 2020 • edited

gribozavr commented Feb 12, 2020

MaxDesiatov commented Feb 12, 2020

gribozavr commented Feb 13, 2020

gribozavr commented Feb 13, 2020

MaxDesiatov commented Feb 13, 2020

stephentyrone commented Feb 13, 2020

MaxDesiatov commented Feb 13, 2020

stephentyrone commented Feb 13, 2020

MaxDesiatov commented Feb 13, 2020

kateinoigakukun commented Jan 30, 2020 •

edited

MaxDesiatov Jan 30, 2020 •

edited

MaxDesiatov commented Feb 9, 2020 •

edited