Skip to content

Add support for logging in SwiftWasm (via WASILibc)#177

Merged
ktoso merged 9 commits intoapple:mainfrom
helje5:features/hh-wasm-1
Jan 25, 2021
Merged

Add support for logging in SwiftWasm (via WASILibc)#177
ktoso merged 9 commits intoapple:mainfrom
helje5:features/hh-wasm-1

Conversation

@helje5
Copy link
Copy Markdown
Contributor

@helje5 helje5 commented Jan 11, 2021

This is a port of Logging to the SwiftWasm WASILibc.

Motivation:

I'd like to log things in Wasm as well.

Modifications:

  1. The library was using a lot of import Glibc as a fallback (you might want to consider to move to canImport). Added additional checks for WASILibc and emit an #error when no known runtime is available.
  2. WASILibc (well, WASI) doesn't support threading and hence locks. #if guarded the Locks.swift file to not build the Lock classes w/ WASILibc. Then adjust the few invocations on the callsites to not use locks on WASILibc.
  3. WASILibc doesn't have stdio, but it does have Foundation print. Defines a TextOutputStream using that when on WASILibc, and do not use the Stdio variant. (implies using the same stream for both stdout/err)
  4. WASILibc doesn't have strftime, but it does have a working Foundation DateFormatter/Date. Use that instead of timestamp formatting.

Result:

swift-log can be used in Wasm environments.

Source:

import Logging
let logger = Logger(label: "zz.log")
logger.info("Hello World!")
helge@M1ni cowsa $ swift build --triple wasm32-unknown-wasi
[3/3] Linking cowsa.wasm
helge@M1ni cowsa $ wasmer .build/debug/cowsa.wasm 
2021-01-11 17:44:06.926 info zz.log : Hello World!

There is no threading on WASI (yet? ever?).
The WASILibc doesn't provide much C yet, apparently. Not
even FILE.
WASILibc doesn't have stdio, e.g. no `FILE`. But it does have
`print`, so let's just use that instead for both, regular and
error logging.
WASILibc doesn't have `strftime`, but it does come
w/ a working `DateFormatter`/`Date`. Use that instead.
Instead do not compile the locks on WASILibc and comment
them out on the calling site.
@swift-server-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

5 similar comments
@swift-server-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@ktoso
Copy link
Copy Markdown
Member

ktoso commented Jan 11, 2021

@swift-server-bot test this please

Copy link
Copy Markdown
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

I'm mostly happy with this but it does seem to add a Foundation dependency.

Comment thread Sources/Logging/Logging.swift Outdated
Comment thread Sources/Logging/Logging.swift Outdated
Comment thread Sources/Logging/Locks.swift
Comment thread Sources/Logging/Locks.swift
Comment thread Sources/Logging/Locks.swift Outdated
Comment thread Sources/Logging/Locks.swift Outdated
Comment thread Sources/Logging/Logging.swift
Comment thread Sources/Logging/Logging.swift
Comment thread Sources/Logging/Logging.swift
Comment thread Sources/Logging/Logging.swift
Comment thread Sources/Logging/Logging.swift
Comment thread Sources/Logging/Logging.swift
Comment thread Sources/Logging/Logging.swift
Comment thread Sources/Logging/Logging.swift
Comment thread Sources/Logging/Logging.swift Outdated
Comment thread Sources/Logging/Logging.swift Outdated
@helje5
Copy link
Copy Markdown
Contributor Author

helje5 commented Jan 12, 2021

OK, lets put that on hold. Of course I tried w/ importing WASILibc first (obviously). As mentioned it didn't carry either FILE nor strftime. That was w/ the 5.3.1 release.

If those do work, 95% of my modifications do not make any sense.

Not sure what went wrong in my first try.
@helje5
Copy link
Copy Markdown
Contributor Author

helje5 commented Jan 12, 2021

FILE isn't available though, file'd: swiftwasm/swift#2480

For WASI the Lock is not used at all, drop the local modifications
I had in place before.
As per swiftwasm/swift#2480 (comment)
`FILE` is not available in SwiftWasm, but `stdio` is. FILE pointers
are exposed as OpaquePointer's in that environment.

Also import the whole libc, like it is done with the other runtimes.
@helje5
Copy link
Copy Markdown
Contributor Author

helje5 commented Jan 22, 2021

OK, I had another look at this, this is much simpler now:

  • no more PrintOutputStream, instead modified the StdioOutputStream to use an OpaquePointer on WASI (instead of a proper UnsafeMutablePointer<FILE>)
  • use strftime, it is available
  • also reverted the changes within Lock, for WASI we just don't use it at all

I didn't do the os(WASI) checks suggested (though I agree they are more correct), as I'm not sure whether those are available on Swift 5.0 and canImport(WASILibc) should be OK.

This should be worth another review, I tried it and it works w/ the SwiftWasm 5.3.1 toolchain.

@helje5 helje5 requested a review from weissi January 22, 2021 12:05
@weissi
Copy link
Copy Markdown
Member

weissi commented Jan 22, 2021

@swift-server-bot test this please

@weissi
Copy link
Copy Markdown
Member

weissi commented Jan 22, 2021

@swift-server-bot add to allowlist

@weissi
Copy link
Copy Markdown
Member

weissi commented Jan 22, 2021

I didn't do the os(WASI) checks suggested (though I agree they are more correct), as I'm not sure whether those are available on Swift 5.0 and canImport(WASILibc) should be OK.

You can do

#if compiler(>=5.3)
#if os(WASIlibc)

@helje5
Copy link
Copy Markdown
Contributor Author

helje5 commented Jan 22, 2021

I didn't do the os(WASI) checks suggested (though I agree they are more correct), as I'm not sure whether those are available on Swift 5.0 and canImport(WASILibc) should be OK.

You can do

#if compiler(>=5.3)
#if os(WASIlibc)

Yes, but do you really want to add that much #if cruft everywhere? I think we better stick to the simpler #if canImport then?

@weissi
Copy link
Copy Markdown
Member

weissi commented Jan 22, 2021

Yes, but do you really want to add that much #if cruft everywhere? I think we better stick to the simpler #if canImport then?

fine by me

@helje5
Copy link
Copy Markdown
Contributor Author

helje5 commented Jan 25, 2021

@MaxDesiatov Is this good now from your perspective? (if so, please mark things as resolved so that the PR can be merged)

Copy link
Copy Markdown
Member

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks!

Copy link
Copy Markdown
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@weissi
Copy link
Copy Markdown
Member

weissi commented Jan 25, 2021

@ktoso / @tomerd want to have a look here or good to merge?

Copy link
Copy Markdown
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Went through and it's looking good, thanks @helje5 -- let's merge.

@ktoso
Copy link
Copy Markdown
Member

ktoso commented Jan 25, 2021

// Had some ancient review comment from the time of the initial commit still there; deleted, ignore that :)

@ktoso ktoso merged commit b544051 into apple:main Jan 25, 2021
@ktoso ktoso added this to the 1.4.1 milestone Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants