search

janestreet / core

Jane Street Capital's standard library overlay
MIT License
1.11k stars 111 forks source link

Bigstring channel structure layout assumptions are incompatible with multicore #146

Closed sadiqj closed 2 years ago

sadiqj commented 3 years ago

@emillon and @engil have been attempting to get the Core testsuite passing on 4.12.0+multicore.

They have tracked down the cause of a segfaulting test to the channel structure layout assumption made in https://github.com/janestreet/core/blob/v0.14/bigstring_unix/src/bigstring_unix_stubs.c#L448

@engil has summarised the issue in more depth here: https://github.com/ocaml-multicore/ocaml-multicore/issues/624#issuecomment-891314830

This will probably require an upstream change, we don't think we can retain compatibility with the existing structure layout.

emillon commented 3 years ago

Hi! I tried with 4.13.1 and this is causing problems too. The reason is that the layout has changed in mainline ocaml too in ocaml/ocaml@96568152d98ef240e463ae853e06502b9d52ffc0 (#10136).

pszilagyi commented 3 years ago

It looks at a glance like we can simply switch to <caml/io.h> in Bigstring_unix. I'm running that change by folks internally now.

emillon commented 3 years ago

The definition is guarded under #ifdef CAML_INTERNALS. I don't know if that's an issue with how core deals with includes.

pszilagyi commented 3 years ago

There are a few places where we do #define CAML_INTERNALS to get similar definitions, and that's what I've provisionally done in this case to get struct channel. But, take with a grain of salt for now, until everyone's had a look.

On Thu, Oct 7, 2021 at 11:08 AM Etienne Millon @.***> wrote:

The definition is guarded under #ifdef CAML_INTERNALS. I don't know if that's an issue with how core deals with includes.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/janestreet/core/issues/146#issuecomment-937884998, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU7Y3CSKOC7DNYSKJ5QVGLUFWZWTANCNFSM5DI5Y7JQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- Pete Szilagyi, Jane Street, 212-651-6156

aalekseyev commented 2 years ago

Should be fixed in 185ea93.

let-def commented 2 years ago

Hello, we just rediscovered this bug. Is it possible to add a constraint ocaml < 4.13 on core v0.14 ?