Backport parent validity check for the GC mark loop #53
Conversation
…ng#49741) * Attempting to add debug logs for ENQUEUING an invalid object Check for the object's validity _before enqueuing_ so that we can hopefully give a more useful error message (which object's pointer was corrupted). --------- Co-authored-by: Diogo Netto <diogonetto.dcn@gmail.com> show mark-queue on GC critical error (JuliaLang#49902)
| @@ -1856,14 +1856,37 @@ STATIC_INLINE uintptr_t gc_read_stack(void *_addr, uintptr_t offset, | |||
| return *(uintptr_t*)real_addr; | |||
| } | |||
|
|
|||
| JL_NORETURN NOINLINE void gc_assert_datatype_fail(jl_ptls_t ptls, jl_datatype_t *vt, | |||
| jl_gc_markqueue_t *mq) JL_NOTSAFEPOINT | |||
| #define GC_ASSERT_PARENT_VALIDITY | |||
There was a problem hiding this comment.
This is not defined upstream I think? So this is not an exact backport? If this is added only for us, then it should be called out explicitly and maybe be a separate commit?
There was a problem hiding this comment.
If this is added only for us, then it should be called out explicitly and maybe be a separate commit?
Sounds good. I left it defined for now since I want to run a few benchmarks to study what is performance penalty of leaving this defined.
Though I should have been more explicit about it. Will add a note on that. Thanks for catching it.
|
NOTE: |
| @@ -1856,14 +1856,37 @@ STATIC_INLINE uintptr_t gc_read_stack(void *_addr, uintptr_t offset, | |||
| return *(uintptr_t*)real_addr; | |||
| } | |||
|
|
|||
| JL_NORETURN NOINLINE void gc_assert_datatype_fail(jl_ptls_t ptls, jl_datatype_t *vt, | |||
| jl_gc_markqueue_t *mq) JL_NOTSAFEPOINT | |||
| #define GC_ASSERT_PARENT_VALIDITY | |||
There was a problem hiding this comment.
@d-netto this is you setting this option to enable the flag, right?
Please separate that out, so that this commit is only the backport of the option, and then we should have a separate commit that actually enables the option. That way, we could drop this patch when upgrading to 1.10, but keep the patch that enables the feature.
Can you put this in a separate commit, and also in a separate file? I think this should be in our Make.user or something like that, right? Or in a nix file? Is there a place where we aggregate these build flags?
There was a problem hiding this comment.
Yes. This option was set by me: I just want to run some benchmarks to see what is the performance penalty of doing that, though I plan to disable it before merging. See note.
|
This PR is stale because it has been open 30 days with no activity. Comment or remove stale label, or this PR will be closed in 5 days. |
This should be squashed with #53 when that is merged.
Backports JuliaLang#49741 and JuliaLang#49902.
I found more convenient to bundle these two upstream PRs into a single backport PR since one is a follow-up for the other, but I can split into separate backports if it makes reviewing easier.
Example of how it works in practice:
gives us the following error message: