haskell / time

A time library
http://hackage.haskell.org/package/time
Other
119 stars 79 forks source link

Use base CClockId as ClockId type #209

Closed TerrorJack closed 2 years ago

TerrorJack commented 2 years ago

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 commented 2 years ago

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

TerrorJack commented 2 years ago

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 commented 2 years ago

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

TerrorJack commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.
TerrorJack commented 2 years ago

@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.