Restore "Protect against repeated initialization (PR#3532)" #11473
Conversation
| @@ -437,7 +437,8 @@ void caml_thread_interrupt_hook(void) | |||
| system. */ | |||
| CAMLprim value caml_thread_initialize(value unit) | |||
| { | |||
| CAMLparam0(); | |||
| /* Protect against repeated initialization (PR#3532) */ | |||
| if (Active_thread != NULL) return Val_unit; | |||
There was a problem hiding this comment.
Why can we assume that Active_thread is NULL before initialization, and not some uninitialized value whose read is undefined behavior? thread_table does not seem 0-initialized.
There was a problem hiding this comment.
The “thread_table table” is actually 0-initialized, since it is a non-local static and an array of values.
There was a problem hiding this comment.
To me all fields should be indeed zero initialized as @gadmm mentioned, so I think this bit is ok. (thread_table[n].active_thread should indeed be 0)
There was a problem hiding this comment.
Hello, thanks you for this re-import!
I think this change is good and make sense, but I fail to link it to the Rust usecase you are talking about.
Why would one want to call caml_thread_initialize right after calling caml_startup and not let it be initialized in the "organic" way through entering the Thread module?
I may be missing a subtlety here.
Nonetheless this change looks good to me and seems correct.
|
It is quite specific and I am not sure I explained it well. Boxroot (cf. our ML workshop paper) installs various callbacks using runtime hooks. In multicore this goes well with systhread (I made sure of it), but for existing OCaml (4.x) versions we need to guarantee that systhread does not overwrite hooks. Hence some initialization order issue. Now it would be very nice if the advice I want to give for OCaml 4.x (calling caml_thread_initialize immediately) does not result in broken programs for OCaml 5. Do you know if the justification mentioned in the documentation comment (cf. #3532) is still valid? Otherwise I must adapt the comment. |
|
I see, looking at Boxroot I think I understand the issue here. It does make sense to me to reinstate it then.
You mean the erroneous linking scenario causing a second entry into |
I suspect it is still valid, in that the bytecode linker still accepts to link threads.cma twice (but with copious warnings). |
(cherry picked from commit 1960635)
|
Cherry-picked to 5.0: c5cda47 |
This restores some bit left out from the previous systhreads implementation.
I would like to have this in 5.0. The ability to call
caml_thread_initializefrom Rust directly after callingcaml_startuppotentially avoids some initialization order pains. I give this advice to Rust users for OCaml 4 and it would be nice if my advice worked consistently between major OCaml versions (without this PR, it would crash when re-enteringcaml_thread_initializefrom theThreadmodule). Since this functionality was there before, this is fixing an API discrepancy, so I think it is appropriate for 5.0.No change entry needed.
(cc @Engil, @gasche)