Fix all the no-naked-pointers problems by mshinwell · Pull Request #1282 · oxcaml/oxcaml

mshinwell · 2023-03-31T14:49:50Z

Having added CI checks for no-naked-pointers and no-naked-pointers+debug-runtime modes, various problems have come to light. After a long struggle, this PR should fix everything, and furthermore ensure the page table is never queried in no-naked-pointers mode (that was not the case before, surprisingly enough):

Fix the new code added in Provide a no-naked-pointers runtime and use it for the compiler #1224 for caml_page_table_lookup; this was erroneously returning values ORed with In_heap in the young and local cases. The comment has also been corrected.
Some assertions which would otherwise trigger when the debug runtime was used in conjunction with a compiler configured with --disable-naked-pointers have been removed; these assertions cannot be checked in that mode.
The changes to make the closure representation allow non-scannable values had bugs in two places: marshalling in extern.c, which was correctly skipping over the non-scannable values but failing to actually marshal them; and the heap check used for the debug runtime.
Some assertions in the GC which prove nothing as a result of the aforementioned PR, because of the new inability to distinguish statically-allocated values from other OCaml values, have been removed.
The best fit collector was failing to set the correct Abstract_tag on "remnant" parts of the heap, which meant that the heap check would see these and fall over.
It became apparent whilst debugging that the memmove operation in the compactor is sometimes a no-op; there's no point in calling memmove in such cases, so I've just put this tiny change in here.
The name of the CI check for ocaml-jst has had nnp removed from the name, since it isn't a no naked pointers build.

We could probably remove the page table code entirely now, but that can wait for another PR (or may not be worth doing as it doesn't exist in OCaml 5).

…and update comment

…ap check

…t_tag tags

ocaml/runtime/compact.c

stedolan · 2023-04-03T08:20:04Z

The best fit collector was failing to set the correct Abstract_tag on "remnant" parts of the heap, which meant that the heap check would see these and fall over.

Can you explain this one a bit more?

ocaml/runtime/extern.c

ocaml/runtime/major_gc.c

mshinwell · 2023-04-03T09:40:21Z

The best fit collector was failing to set the correct Abstract_tag on "remnant" parts of the heap, which meant that the heap check would see these and fall over.

Can you explain this one a bit more?

Here's the situation triggered reliably by tests/regression/pr9853/:

0x108510f68: 0x0000000108510f48 0x0000000000000800
0x108510f78: 0x0000000000000001 0x0000000108510f60 - well-formed value with `0x800` header

Remnant left behind by best fit.  It has tag zero, but cannot be scanned in NNP mode:
0x108510f88: 0x0000000000003800 (14 fields) = [ 0x0000000000000000
0x108510f98: 0xd701d7d7d701d6d7 0xd701d7d7d701d6d7
0x108510fa8: 0xd701d7d7d701d6d7 0xd701d7d7d701d6d7
0x108510fb8: 0xd701d7d7d701d6d7 0xd701d7d7d701d6d7
0x108510fc8: 0xd701d7d7d701d6d7 0xd701d7d7d701d6d7
0x108510fd8: 0xd701d7d7d701d6d7 0xd701d7d7d701d6d7
0x108510fe8: 0x0000000000000800 0x0000000000000001
0x108510ff8: 0x0000000108510f60 ] 0xd785d7d7d785d6d7
0x108511008: 0xd785d7d7d785d6d7 0xd785d7d7d785d6d7
0x108511018: 0xd785d7d7d785d6d7 0xd785d7d7d785d6d7
0x108511028: 0xd785d7d7d785d6d7 0xd785d7d7d785d6d7
Heap contains only one chunk which ends here

0x108511038: 0x0000000000000000 0x0000000000000000
0x108511048: 0x0000000000000000 0x0000000000000000

As far as I can see the existing semantics of the best-fit code is that blocks allocated with Caml_white colour are given tag Abstract_tag; this was just missing for the "remnant" case.

stedolan · 2023-04-03T10:03:22Z

Is the heap checker scanning blocks with tag Caml_white? I thought it never did that (Caml_white blocks do not necessarily contain sensible data)

mshinwell · 2023-04-03T10:05:46Z

Is the heap checker scanning blocks with tag Caml_white? I thought it never did that (Caml_white blocks do not necessarily contain sensible data)

It does in certain situations, it would seem: https://github.com/ocaml-flambda/flambda-backend/blob/main/ocaml/runtime/gc_ctrl.c#L172

ocaml/runtime/intern.c

1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (oxcaml#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (oxcaml#1282) 8fe089e flambda-backend: Unrevert oxcaml#1131 and fix a Cmm unboxing bug (oxcaml#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (oxcaml#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (oxcaml#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (oxcaml#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (oxcaml#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (oxcaml#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (oxcaml#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (oxcaml#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (oxcaml#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (oxcaml#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: 1686928

a7d005a flambda-backend: Lazy deserialization of cmi files (oxcaml#1322) aa83fa3 flambda-backend: Reinstate previous API for Env.lookup_value (oxcaml#1323) e4007a4 flambda-backend: Lazy substitution into value_declaration (oxcaml#1320) 634b607 flambda-backend: Merge Types.* and Subst.Lazy.* types (oxcaml#1312) cf82708 flambda-backend: Bump magic numbers for 4.14.1-7 (oxcaml#1317) 6470400 flambda-backend: zero_alloc attribute payload "assert all" and "ignore" (oxcaml#1296) bba5248 flambda-backend: Teach `ocamldep` about all the language extensions (oxcaml#1303) 33e97b0 flambda-backend: Change Includemod to work on lazy modtypes (oxcaml#1228) 16e5002 flambda-backend: zero_alloc new warning for unchecked functions (oxcaml#1302) 36b4626 flambda-backend: Attribute [@@@zero_alloc check] to turn the check on (oxcaml#1294) 3b524c6 flambda-backend: Cmm.value_kind cleanup (oxcaml#1091) ec99505 flambda-backend: Fix failure of `check_all_arches` on RISCV (oxcaml#1300) 450bc58 flambda-backend: Backend changes for multiple returns (oxcaml#1268) 84a7a26 flambda-backend: Static check for zero_alloc: ignore allocation that lead to exceptional return (oxcaml#1157) 1723728 flambda-backend: Re-enable parallel build of the runtime (oxcaml#1287) 26ea7f3 flambda-backend: Fix closure marshalling when not in NNP mode (oxcaml#1286) 9b91f2e flambda-backend: Reduce number of caml_apply functions taking/returning "I" and "V" (oxcaml#1272) 1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (oxcaml#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (oxcaml#1282) 8fe089e flambda-backend: Unrevert oxcaml#1131 and fix a Cmm unboxing bug (oxcaml#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (oxcaml#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (oxcaml#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (oxcaml#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (oxcaml#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (oxcaml#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (oxcaml#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (oxcaml#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (oxcaml#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (oxcaml#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: a7d005a

mshinwell added 7 commits March 31, 2023 15:24

Fix wrong return values in new NNP version of caml_page_table_lookup …

5347683

…and update comment

Skip some assertions that cannot be checked in NNP mode during the he…

6de80e6

…ap check

Fix marshalling and heap check for new closure representation

cc161ef

Remove assertions that now prove nothing

be2c409

Ensure white remnants are created in bf_make_free_blocks with Abstrac…

7726768

…t_tag tags

Stop pointless memmove operations

10a3528

Fix misleading CI check name for ocaml-jst

b24bf5c

mshinwell added the bug Something isn't working label Mar 31, 2023

mshinwell requested a review from stedolan as a code owner March 31, 2023 14:49

stedolan reviewed Apr 3, 2023

View reviewed changes

ocaml/runtime/compact.c Outdated Show resolved Hide resolved

stedolan reviewed Apr 3, 2023

View reviewed changes

ocaml/runtime/extern.c Show resolved Hide resolved

ocaml/runtime/major_gc.c Show resolved Hide resolved

Remove check prior to memmove

ce62088

mshinwell force-pushed the fix-nnp-closure-marshalling branch from 07ad401 to 32f62cb Compare April 3, 2023 10:30

stedolan reviewed Apr 3, 2023

View reviewed changes

ocaml/runtime/intern.c Show resolved Hide resolved

Fix marshalling of non-scanned environments

920c8c0

mshinwell force-pushed the fix-nnp-closure-marshalling branch from 32f62cb to 920c8c0 Compare April 3, 2023 13:27

stedolan approved these changes Apr 3, 2023

View reviewed changes

stedolan merged commit 36c851d into oxcaml:main Apr 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix all the no-naked-pointers problems#1282

Fix all the no-naked-pointers problems#1282
stedolan merged 9 commits intooxcaml:mainfrom
mshinwell:fix-nnp-closure-marshalling

mshinwell commented Mar 31, 2023

Uh oh!

Uh oh!

stedolan commented Apr 3, 2023

Uh oh!

Uh oh!

Uh oh!

mshinwell commented Apr 3, 2023

Uh oh!

stedolan commented Apr 3, 2023

Uh oh!

mshinwell commented Apr 3, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mshinwell commented Mar 31, 2023

Uh oh!

Uh oh!

stedolan commented Apr 3, 2023

Uh oh!

Uh oh!

Uh oh!

mshinwell commented Apr 3, 2023

Uh oh!

stedolan commented Apr 3, 2023

Uh oh!

mshinwell commented Apr 3, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants