Skip to content
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

Use platform shims for clock_gettime to support wasi-libc #781

Conversation

kateinoigakukun
Copy link
Member

This change adds platform shims for clock_gettime so that we can use it in Swift code even when the clock id macros can't be imported through ClangImporter like wasi-libc's definitions.

Also wasi-libc's timespec.tv_sec and timespec.tv_nsec are not imported as Int in Swift, so we need to cast them to UInt64 before doing arithmetic operations on them.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Does this really require the C shim? Why can we not just add an OS conditional?

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Jul 27, 2024

CLOCK_MONOTONIC and CLOCK_REALTIME are defined in wasi-libc as follows, and we can't use them directly from Swift. Thus we need C shim at least for WASI, and I think it makes sense to use the shims for all unixy platforms consistently to share the same semantics code.

extern const struct __clockid _CLOCK_MONOTONIC;
#define CLOCK_MONOTONIC (&_CLOCK_MONOTONIC)
extern const struct __clockid _CLOCK_REALTIME;
#define CLOCK_REALTIME (&_CLOCK_REALTIME)

https://github.com/WebAssembly/wasi-libc/blob/b9ef79d7dbd47c6c5bafdae760823467c2f60b70/libc-bottom-half/headers/public/__header_time.h#L17-L20

@jmschonfeld
Copy link
Contributor

@swift-ci please test

@kateinoigakukun kateinoigakukun force-pushed the pr-e051ba5406b3773ba8a95dcba35b477df0f4b273 branch from 665353d to f8b423d Compare July 30, 2024 03:54
@kateinoigakukun
Copy link
Member Author

@swift-ci test

// Define clock_gettime shims for each clock type so that we can use them in Swift
// even if clock id macros can't be imported through ClangImporter.

#if !defined(_WIN32)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use TARGET_OS_WINDOWS here to try to be consistent with our platform checks (I know some places might not, I plan to come and fix up a few but it'd be great to standardize on the target conditionals with new additions)

@@ -67,3 +67,20 @@ mach_port_t _platform_mach_task_self() {
}
#endif

#if !defined(_WIN32)
int _platform_shims_clock_monotonic_raw_gettime(struct timespec * _Nonnull ts) {
#if __wasi__ // WASI does not support CLOCK_MONOTONIC_RAW. Fall back to CLOCK_MONOTONIC.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here as above, could we use TARGET_OS_WASI?

@kateinoigakukun
Copy link
Member Author

@swift-ci test

@jmschonfeld
Copy link
Contributor

Just confirming - is this change still necessary for the WASI build? One thought that occurred to me - if the problem is CLOCK_MONOTONIC, I wonder if we could just shim that and limit the shim to WASI instead of shimming the whole function, does that seem plausible / do you think that'd be any better? Not sure if it's inherently better, just a thought

This change adds platform shims for clock ids so that we can use
them in Swift code because the clock id macro definitions in wasi-libc
can't be imported through ClangImporter.

Also wasi-libc's `timespec.tv_sec` and `timespec.tv_nsec` are not
imported as `Int` but as `Int64` and `Int32` respectively, so we need to
cast them to `UInt64` before doing arithmetic operations on them.
@kateinoigakukun kateinoigakukun force-pushed the pr-e051ba5406b3773ba8a95dcba35b477df0f4b273 branch from 284487d to 4e06bcd Compare August 7, 2024 02:47
@kateinoigakukun
Copy link
Member Author

@jmschonfeld That totally makes sense to me. Now shims are isolated to only WASI.

@kateinoigakukun
Copy link
Member Author

@swift-ci test

1 similar comment
@kateinoigakukun
Copy link
Member Author

@swift-ci test

@kateinoigakukun kateinoigakukun merged commit 4440638 into swiftlang:main Aug 8, 2024
3 checks passed
cthielen pushed a commit to cthielen/swift-foundation that referenced this pull request Nov 8, 2024
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.

3 participants