This issue aims to record and document outstanding comments in the Multicore PR (see #10831), in the case a merge makes it harder for the teams to dig them out from the Github UI.

This list should be exhaustive, however there may be some bits missing. Hopefully the PR remains usable after our last push and subsequent merge.
Anyone mentioned here, feel free to take a look at the list including your names and see if I missed anything in the process.

• runtime/startup_aux.c:123 Multicore OCaml #10831 (review) (fixed in Fixup Filename.check_suffix; remove duplicate ',' fix for OCAMLRUNPARAM #10873)
  @damiendoligez : I don't understand this change. It seems to me that empty clauses are handled by line 121.

• stdlib/camlinternalAtomic.ml:5 Multicore OCaml #10831 (review)
  @damiendoligez @gadmm : Should probably add the copyright of Cambridge and/or Stephen Dolan. cc @stedolan

• stdlib/filename.ml:103 Multicore OCaml #10831 (review)
  @damiendoligez @xavierleroy : This is another recent change in 4.x that was not picked in Multicore.

• runtime/domain.c:1455 Multicore OCaml #10831 (review)
  @damiendoligez : That doesn't leave any room for the id added by caml_thread_setname. Not sure if this is important, though.

• runtime/eventlog.c:370 Multicore OCaml #10831 (review) (implemented in Ring-buffer based runtime tracing (eventring) #10964)
  @damiendoligez : These counters give information about the allocation patterns of the OCaml program, not about the allocator itself. So yes, we do want them in Multicore, eventually.

• runtime/fail_nat.c:207 Multicore OCaml #10831 (review)
  @damiendoligez : To do: change this into a call to caml_fatal_error.

• runtime/fiber.c:101 Multicore OCaml #10831 (review) (transferred to stack handler alignment in fiber.c #11779)
  @damiendoligez : Assumes 64-bit words.

• runtime/fiber.c:168 Multicore OCaml #10831 (review) (transferred to stack handler alignment in fiber.c #11779)
  @damiendoligez : This assumes a 64-bit word size, right?

• runtime/io.c:552 Multicore OCaml #10831 (review)
  @kayceesrk : This condition is missing in multicore, possibly lost in the rebase. Looks like the changes from this PR were not included in multicore: https://github.com/ocaml/ocaml/pull/10136/files.

• Multicore OCaml #10831 (comment)
  @shindere Naked pointers are not allowed in OCaml Multicore. Now that we are about to merge, shoulnd't it be updated to juss say that naked pointers are no longer supported, if I understand correctly? (fixed in Ignore compatible --disable options in configure #10962)

• asmcomp/cmm_helpers.ml:2206 Multicore OCaml #10831 (review)
  @kayceesrk @ctk21 Related discussion: https://github.com/ocaml/ocaml/pull/10831/#discussion_r777185977. (Github UI renders the URL wrong).

• testsuite/tests/c-api/alloc_async.ml:1 Multicore OCaml #10831 (review) (re-enabled in Turn tests/c-api/alloc_async.ml back on #11596)
  @kayceesrk Are we sure this is meant to be skipped?

• .gitattributes:193 Multicore OCaml #10831 (comment)
  @dra27 Is there a task to tidy this up? We should either generate this in the build (which I think is hard and possibly unnecessary) or the script which generates it ought to satisfy check-typo. If it is a committed generated file, CI should check its consistency (I'm volunteering, if necessary...)

• configure.ac:304 Multicore OCaml #10831 (comment)
  @dra27 This doesn't quite do what's intended (in passing, I see the spacetime one is wrong too, which is embarrassing given that I wrote it!): (see Ignore compatible --disable options in configure #10962)

• runtime/caml/memory.h:21 Multicore OCaml #10831 (comment)
  @dra27 Is this correct? (addressed in Get rid of <caml/compatibility.h> #10863)

• runtime/caml/ui.h: Multicore OCaml #10831 (comment)
  @dra27 @xavierleroy The whole runtime/caml/ui.h should be removed, as well as its inclusion in runtime/startup_nat.h. It's a leftover from very old times.

• otherlibs/unix/fork.c:41 Multicore OCaml #10831 (comment)
  @xavierleroy Should it be failwith or invalid_argument or a Unix error code like ENOTSUPP ? I don't know yet, just making a note to think about it later.

• otherlibs/unix/unixsupport.c:296 Multicore OCaml #10831 (comment)
  @xavierleroy That's OK for the time being. Later, either check that caml_named_value is fast enough (currently it uses a rather primitive hash table implementation) or consider caching the result in thread-local storage or in global storage but with atomic update. (Minor improvements to "named values" handling #11056)

• runtime/eventlog.c:199 Multicore OCaml #10831 (comment) (eventlog.c removed in Ring-buffer based runtime tracing (eventring) #10964)
  @avsm isn't caml_stat_alloc and raising Out_of_memory shorter and a more accurate exception here? (the backtrace will reveal where the mem alloc failed)

• runtime/caml/sync.h:82 Multicore OCaml #10831 (comment)
  @edwintorok If this is called in a multithreaded program it should call strerror_r: the value returned by strerror could change if another thread calls strerror meanwhile, man 3p strerror says: (Alternative approach to using strerror_r for reentrant error string conversion #11010).

• runtime/fiber.c:77 Multicore OCaml #10831 (comment)
  @avsm For OpenBSD support, we're going to need to allocate this stack memory with mmap(..., MAP_STACK|MAP_ANON|MAP_PRIVATE) or else the process is killed by the syscall entry function. I've got a patch for this locally on OpenBSD but am not sure of the performance impact of switching to mmap for every OS or if we should just do that for OpenBSD.

• runtime/addrmap.c:27 Multicore OCaml #10831 (comment) (addressed in Comment hash function in pos_initial #11650)
  @xavierleroy Where does this hash function comes from? The "multiply by 0xcc9e2d51" is also in Murmurhash 3, but not the "shift and xor". It would be nice to give a reference.

• runtime/caml/camlatomic.h:54 Multicore OCaml #10831 (comment)
  @xavierleroy Do we really need to support GCC 4.8? (last released in 2015...)It would be nice to rely on <stdatomic.h> unless absolutely impossible (i.e. unless MSVC).

• runtime/io.c:201 Multicore OCaml #10831 (comment)
  @xavierleroy I don't understand the logic of this code, and it makes me nervous. Yes, there can be a race between caml_close_channel and caml_ml_out_channels_list. How does this code resolve it?

• runtime/io.c:502 Multicore OCaml #10831 (comment)
  @xavierleroy Same comment as above. Rather than "let's unlink and ooops let's link again", could we just unlink only when it's safe?

• runtime/obj.c:188 Multicore OCaml #10831 (comment)
  @xavierleroy I'd rather remove caml_obj_truncate and Obj.truncate entirely, so that we get clean compile-time errors for code that uses it. Note that Obj.truncate has been marked deprecated since 4.09.

• runtime/platform.c:244 Multicore OCaml #10831 (comment)
  @xavierleroy @gadmm @edwintorok @gadmm: The original comment has been there unchanged for a while and I propose to remove it. I do not think there is much to gain in trying to fight the overcommitting behaviour on systems that have it enabled. Not failing immediately is overcommitting working as intended (even if it does not produce any interesting difference in this case), and one should instead focus on reliable ways to handle OOM if there is interest in this topic.

• runtime/roots.c:34 Multicore OCaml #10831 (comment)
  @xavierleroy @kayceesrk We may revisit this design choice after the merge. The change is fairly easy to implement and benchmark.

• runtime/callback.c:346 Multicore OCaml #10831 (comment)
  @kayceesrk Not having a lock seems like an omission. We don't have a test for caml_iterate_named_values in the testsuite, which we should. (Minor improvements to "named values" handling #11056)

• runtime/startup_byt.c:381 Multicore OCaml #10831 (comment)
  @kayceesrk Is there a reason why caml_global_data needs to be handled differently in multicore? (Fix leftover TODO in runtime/startup_byt.c #11051)

• runtime/obj.ml:32 Multicore OCaml #10831 (comment)
  @kayceesrk Consider for removal.

• runtime/caml/stack.h:71 Multicore OCaml #10831 (comment) (removed in Remove unused #define Context_needs_padding from stack.h #11621; arm64 backend re-enabled without it in Arm64 multicore support #10972 and the struct itself which used the macro was removed in Documentation improvements for native stack switching functions #11059)
  @kayceesrk Unused currently. It was needed earlier for the arm64 backend. May need it again when arm64 is revived.

• runtime/stdlib.mli Multicore OCaml #10831 (comment)
  @kayceesrk consider renaming EffectsHandlers to Effects

• runtime/meta.c:157 Multicore OCaml #10831 (comment)
  @kayceesrk GC can't be triggered in the code; Field no longer includes a read barrier. Make it similar to trunk.