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 base CClockId as ClockId type #209

Merged
merged 2 commits into from
May 30, 2022

Conversation

TerrorJack
Copy link
Contributor

We used to define ClockId as hsc2hs-detected ${type clockid_t} type. Unfortunately, certain libcs (e.g. wasi-libc) define clockid_t as a pointer type, and hsc2hs only supports detecting C integral/floating-point types, so clockid_t will mistakenly be detected as a floating-point type, resulting in incorrect code generation.

We should really just use the base CClockId type here. base handles C pointer types correctly, since it uses its own custom autoconf logic instead of hsc2hs, and similar issues have been reported & fixed before, see https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6896 and https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6912.

@AshleyYakeley
Copy link
Member

Is there some way this can be properly tested with WebAssembly? I'd prefer not to take a sequence of fixes.

@TerrorJack
Copy link
Contributor Author

I'd prefer not to take a sequence of fixes.

Sorry, this PR was meant to be the last one related to wasm!

Is there some way this can be properly tested with WebAssembly?

If you don't mind, I can add a CI job to test it; just FYI the fork including this change is already used/tested in the wasm32-wasi ghc head bindist.

@AshleyYakeley
Copy link
Member

Yes, if you can please add a CI job to run the test suites with wasm.

@TerrorJack TerrorJack force-pushed the fix-clockid-ctype branch from c0c8bda to 514d0c9 Compare May 27, 2022 10:38
@TerrorJack
Copy link
Contributor Author

Hi @AshleyYakeley, I've added a CI pipeline that builds and runs a simple test case.

Due to tasty introducing clock as transitive dependency, and clock needing similar fixes at the moment, the full test suite can't be built at the moment; do you intend to hold this PR until test-main/test-unix is built?

@AshleyYakeley
Copy link
Member

Is there a parallel github issue for the clock package?

What do you need right now? I can merge this PR, though ideally I'd like to see test-main & test-unix pass. However, I'm not planning on doing another release as I just did one a few days ago.

@TerrorJack
Copy link
Contributor Author

Is there a parallel github issue for the clock package?

Not yet; the clock package involves more work, and I'm holding off the issue before I have a patch yet.

What do you need right now?

It would be helpful to merge it as it currently is :) Not doing another release immediately is totally fine, the wasm work is targeting 9.6 anyway so we still got quite some time.

@AshleyYakeley
Copy link
Member

The build gives this warning, can you clarify? Is there a way of fixing it?

/tmp/ghc4503_0/ghc_48.c:9:122: error:
     warning: incompatible pointer to integer conversion returning 'const struct __clockid *' from a function with result type 'HsWord32' (aka 'unsigned int') [-Wint-conversion]
  |
9 | HsWord32 ghczuwrapperZC0ZCtimezm1zi12zi2zminplaceZCDataziTimeziClockziInternalziCTimespecZCCLOCKzuREALTIME(void) {return CLOCK_REALTIME;}
  |                                                                                                                          ^
HsWord32 ghczuwrapperZC0ZCtimezm1zi12zi2zminplaceZCDataziTimeziClockziInternalziCTimespecZCCLOCKzuREALTIME(void) {return CLOCK_REALTIME;}
                                                                                                                         ^~~~~~~~~~~~~~

/home/runner/.ghc-wasm32-wasi/wasi-sdk/bin/../share/wasi-sysroot/include/__header_time.h:22:24: error:
     note: expanded from macro 'CLOCK_REALTIME'
   |
22 | #define CLOCK_REALTIME (&_CLOCK_REALTIME)
   |                        ^
#define CLOCK_REALTIME (&_CLOCK_REALTIME)
                       ^~~~~~~~~~~~~~~~~~
1 warning generated.

We used to define ClockId as hsc2hs-detected ${type clockid_t} type.
Unfortunately, certain libcs (e.g. wasi-libc) define clockid_t as a
pointer type, and hsc2hs only supports detecting C
integral/floating-point types, so clockid_t will mistakenly be
detected as a floating-point type, resulting in incorrect code
generation.

We should really just use the base CClockId type here. base handles C
pointer types correctly, since it uses its own custom autoconf logic
instead of hsc2hs, and similar issues have been reported & fixed
before, see https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6896
and https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6912.

Also make tzset conditional.
@TerrorJack TerrorJack force-pushed the fix-clockid-ctype branch from 514d0c9 to b35924e Compare May 30, 2022 15:38
@TerrorJack
Copy link
Contributor Author

@AshleyYakeley The warning is harmless, since CLOCK_REALTIME is either a word-sized integer, or a pointer that can be casted from/to a word-sized integer without loss. For glibc it's an integer literal, therefore no warning, while for wasi-libc it's the latter.

Neverthless, I added an explicit cast to handle this warning. Also, now we run ShowTime case for wasm32 CI, which at run-time touches the relevant code path to make sure it works.

@AshleyYakeley AshleyYakeley merged commit 8cf54fe into haskell:master May 30, 2022
@TerrorJack TerrorJack deleted the fix-clockid-ctype branch May 30, 2022 18:44
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.

2 participants