Skip to content

Add a naked pointers dynamic checker, continued#9956

Merged
xavierleroy merged 16 commits intoocaml:trunkfrom
xavierleroy:nnp_checker
Oct 5, 2020
Merged

Add a naked pointers dynamic checker, continued#9956
xavierleroy merged 16 commits intoocaml:trunkfrom
xavierleroy:nnp_checker

Conversation

@xavierleroy
Copy link
Copy Markdown
Contributor

This is a variant of #9947 where the check for "naked pointers" (dangerous out-of-heap pointers) is performed in a runtime system that supports naked pointers and will not crash early when it encounters one.

Also, the check is disabled in bytecode, as we don't catch SEGV signals in this case.

Tests were added to the test suite that exercise the three modes: naked pointers / naked pointers + dynamic checker / no naked pointers.

/* For the pointer to be considered safe, either the given pointer is in heap
* or the (out of heap) pointer has a black header and its size is < 2 ** 40
* words (128 GB). If not, we report a warning. */
if (Is_in_heap (v) ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC check_pointer_safe is only invoked when !Is_in_heap(v) && !Is_young(v). check_pointer_safe should not be called on a pointer in the minor heap since OCaml does not define what the colour of minor objects is.

Would it be useful to rename this function to caml_is_naked_pointer_safe, with an assertion CAMLassert (!Is_in_heap(v) && !Is_young(v)) to make these assumptions explicit?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're absolutely right! I renamed to is_naked_pointer_safe (it's a static function, so no need for caml_), added the assertion, and removed the Is_in_heap test within.

@kayceesrk
Copy link
Copy Markdown
Contributor

I've reviewed the changes and it looks good to me. In particular, having the test in naked pointers mode makes sense to me.

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Oct 5, 2020

@kayceesrk Just to make sure I understand, in this comment it is written that safe_load is no longer needed, but the function is still present in the code. Did I misunderstand the comment?

@kayceesrk
Copy link
Copy Markdown
Contributor

I should have clarified that safe_load was no longer needed in mark_stack_push since we no longer load the header since #9951. See https://github.com/ocaml/ocaml/pull/9951/files#diff-554731eb45f6f98a2ad012dfbe66f9caL244.

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Oct 5, 2020

Thanks for the explanation, I understand now.

- Rename `check_pointer_safe` -> `is_naked_pointer_safe`
- Make it clear that it is called only on out-of-heap pointers
  (neither Is_young nor Is_in_heap), as checked by the caller.
@kayceesrk
Copy link
Copy Markdown
Contributor

Lgtm!

@xavierleroy xavierleroy merged commit af48d9f into ocaml:trunk Oct 5, 2020
@xavierleroy xavierleroy deleted the nnp_checker branch October 5, 2020 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants