Add a naked pointers dynamic checker by abbysmal · Pull Request #9947 · ocaml/ocaml

abbysmal · 2020-09-29T14:42:46Z

This pull requests is a rebased version of the naked pointer checker, as discussed in #9534, for introduction in the runtime.
It remains mostly the same as the code that was developed in the RFC, with a few adjustments:

Introduction of a configure time option to enable it (--enable-naked-pointers-checker)
The major_gc trigger at the end of the program (to force a gc and run the checker even on programs that may terminate before having the opportunity to do so) was reworked as well.
checking_pointer_pc is now hidden behind an ifdef when the checker is not built.

While the checker operates mostly the same, heavy changes were brought to the GC, so I could observe different behaviors between this branch and the previous implementation.

One particular thing I observed (and I'm unsure about the correct resolution of this issue, if it qualifies as such), is that now, a few test programs now segfaults in the no-naked-pointers mode, and as such as well in the naked-pointer-checker mode, where they used not to.
Two simple examples are

The test case provided by @kayceesrk on [RFC] Dynamic check for naked pointers #9534, now leads to a segfault further down the path during marking because the invariant for Caml_gray in invert_pointer_at (https://github.com/ocaml/ocaml/blob/trunk/runtime/compact.c#L80) is not respected, leading to further mayhem.
https://github.com/dra27/naked also leads to this exact observation, with the (likely platform dependent funkiness) program only segfaulting on msvc64.

I'm unsure what is the correct course of action with this observation, and would be happy if anyone had an opinion on the matter.
Maybe there should be a specific adjustment in no-naked-pointer mode I am missing, related to this recent change.
I can provided further data on these specific points if it helps.

I added a temporary Changes entry, I quoted the names and reviewers that contributed to the RFC at #9534, but since this PR definitely could use more proofreading (with the recent changes to the GC), I did not meant this was reviewed already. :)

Have a nice day,
Enguerrand.

…gure additions

xavierleroy · 2020-09-29T15:03:06Z

Thanks for getting the ball rolling. Just to make sure we're on the same page:

One particular thing I observed (and I'm unsure about the correct resolution of this issue, if it qualifies as such), is that now, a few test programs now segfaults in the no-naked-pointers mode, and as such as well in the naked-pointer-checker mode, where they used not to.

I would expect the naked pointers detector to run in a naked-pointer-tolerant run-time system, so that naked pointers won't crash the runtime immediately and can be flagged and reported at the next major GC. And, yes, no-naked-pointers mode is getting less and less tolerant in the working sources.

abbysmal · 2020-09-29T16:04:45Z

I would expect the naked pointers detector to run in a naked-pointer-tolerant run-time system, so that naked pointers won't crash the runtime immediately and can be flagged and reported at the next major GC. And, yes, no-naked-pointers mode is getting less and less tolerant in the working sources.

The previous code was running inside the no-naked-pointer runtime, it would make sense however to have the checks performed outside indeed, I can change this if this is the right thing to do.

nojb · 2020-09-29T18:19:20Z

runtime/domain.c

+
+  #ifdef NAKED_POINTERS_CHECKER
+  Caml_state->checking_pointer_pc = NULL;
+  #endif


Is this used under Windows?

Right, it is not, I will guard it for _WIN32 as well, thank you.

Same for the definition in domain_state.tbl.

kayceesrk · 2020-09-30T07:34:19Z

Changes

+- #9534: Introduce a naked pointers checker mode to the runtime,
+  to print information on relevant values when they are
+  inspected by the garbage collector.
+  (KC Sivaramakrishnan, with contributions by Stephen Dolan and David Allsopp,


You should add yourself here @Engil. And to be consistent with the other entries in the file, perhaps do:

Enguerrand Decorne, KC Sivaramakrishnan, Xavier Leroy, Stephen Dolan, David Allsopp, Nicolás Ojeda Bär, review by ...

Xavier contributed the inline assembly and Nicolás kick-started the work on Windows support.

abbysmal · 2020-09-30T13:28:36Z

While implementing the move of the checker out of the no-naked-pointer mode, I noticed that the example program provided on #9534 does segfault currently on trunk even with the "default" runtime (so with naked pointers on.).
This program used to work on 4.11, 4.10 and 4.09.

Program received signal SIGSEGV, Segmentation fault.
0x00005562cf2e74e9 in mark_stack_push (work=0x0, offset=0, block=140569830424568, stk=<optimized out>) at major_gc.c:244
warning: Source file is more recent than executable.
244	    if (Is_block(v) && !Is_black_val(v))
(rr) bt
#0  0x00005562cf2e74e9 in mark_stack_push (work=0x0, offset=0, block=140569830424568, stk=<optimized out>) at major_gc.c:244
#1  caml_darken (v=140569830424568, p=<optimized out>) at major_gc.c:301
#2  0x00005562cf2e5291 in caml_darken_all_roots_slice (work=work@entry=9223372036854775804) at roots_nat.c:375
#3  0x00005562cf2e6df0 in mark_slice (work=9223372036854775804, work@entry=9223372036854775807) at major_gc.c:609
#4  0x00005562cf2e7e40 in caml_finish_major_cycle () at major_gc.c:967
#5  0x00005562cf2f7240 in caml_gc_full_major (v=<optimized out>) at gc_ctrl.c:590
#6  0x00005562cf2c6d51 in camlTest__do_gc_216 () at roots_nat.c:159
#7  0x00005562cf2c6fc4 in camlTest__entry () at roots_nat.c:159
#8  0x00005562cf2c4869 in caml_program () at roots_nat.c:159
#9  0x00005562cf30160c in caml_start_program ()
#10 0x00005562cf301e44 in caml_startup_common (argv=0x7ffc345c8258, pooling=<optimized out>, pooling@entry=0) at startup_nat.c:164
#11 0x00005562cf301e8f in caml_startup_exn (argv=<optimized out>) at startup_nat.c:174
#12 caml_startup (argv=<optimized out>) at startup_nat.c:174
#13 0x00005562cf2c4662 in main (argc=<optimized out>, argv=<optimized out>) at main.c:41

Should I fill in a separate issue for this?

xavierleroy · 2020-09-30T17:20:33Z

Should I fill in a separate issue for this?

Yes,please. It's entirely possible that the new two-color marking phase is missing a case or two for naked pointers. Should be easy to fix.

kayceesrk · 2020-10-01T06:04:37Z

@Engil I've made #9951 to fix #9950. When that is approved, you'll need a safe_load loading the header to check if it is already marked here https://github.com/kayceesrk/ocaml/blob/fix_mark_stack_push_opt/runtime/major_gc.c#L246.

kayceesrk · 2020-10-03T04:43:01Z

#9951 is merged. We do not load the header anymore and hence, safe_load is not necessary.

xavierleroy · 2020-10-03T16:54:59Z

In order to make progress faster, I "forked" this PR at #9956 to get something that seems to work and has a test suite.

abbysmal · 2020-10-05T08:21:03Z

Sure, thank you very much for this. This PR can be closed I believe?

xavierleroy · 2020-10-05T12:45:16Z

This was merged as part of #9956, commit af48d9f.

kayceesrk and others added 9 commits September 22, 2020 15:21

Initial commit of naked pointer checker

d1d352a

Atoms have black header. Objects must be reasonable sized.

e1ee13a

Disable optimisations for is_pointer_safe

c25163d

Convince clang to generate the correct code

e943895

Use inline assembly to circumvent obstinate clang

9cb9b61

Stephen's safe_load code for msvc64

a1dcf8c

SIZEOF_PTR != SIZEOF_LONG on Windows

9758bd7

nnp checker: cherry-pick to trunk, update to support 4.12.0 and confi…

44f3f2d

…gure additions

Changes entry for naked pointers checker

ae766ac

nojb reviewed Sep 29, 2020

View reviewed changes

kayceesrk reviewed Sep 30, 2020

View reviewed changes

nnp checker: checking_pointer_pc not used on _WIN32

2beb8e6

abbysmal mentioned this pull request Sep 30, 2020

Naked pointers crash on trunk #9950

Closed

update Changes entry

671dc0b

xavierleroy mentioned this pull request Oct 3, 2020

Add a naked pointers dynamic checker, continued #9956

Merged

xavierleroy closed this Oct 5, 2020

Conversation

abbysmal commented Sep 29, 2020

Uh oh!

xavierleroy commented Sep 29, 2020

Uh oh!

abbysmal commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

abbysmal Sep 30, 2020

Choose a reason for hiding this comment

Uh oh!

nojb Sep 30, 2020

Choose a reason for hiding this comment

Uh oh!

kayceesrk Sep 30, 2020

Choose a reason for hiding this comment

Uh oh!

abbysmal commented Sep 30, 2020

Uh oh!

xavierleroy commented Sep 30, 2020

Uh oh!

kayceesrk commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kayceesrk commented Oct 3, 2020

Uh oh!

xavierleroy commented Oct 3, 2020

Uh oh!

abbysmal commented Oct 5, 2020

Uh oh!

xavierleroy commented Oct 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

abbysmal commented Sep 29, 2020 •

edited

Loading

kayceesrk commented Oct 1, 2020 •

edited

Loading