Fix GCExt test #47699
Fix GCExt test #47699
Conversation
|
Ugh! Thank you! |
brenhinkeller
added
multithreading
| @@ -307,6 +307,7 @@ static size_t gc_alloc_size(jl_value_t *val) | |||
|
|
|||
| int internal_obj_scan(jl_value_t *val) | |||
| { | |||
| // FIXME: `jl_gc_internal_obj_base_ptr` is not allowed to be called from outside GC | |||
There was a problem hiding this comment.
@rbehrends we already register a task scanner and a root scanner callback in gcext, which exercise jl_gc_internal_obj_base_ptr. So could we just remove internal_obj_scan? (I guess if we did that, a major part of gcext.c and LocalTest.jl would end up dead code and could be removed...)
Or do you think it's still valuable? If so, perhaps the code @vchuravy added here in a previous revision of the PR (and which is still visible in https://github.com/JuliaLang/julia/pull/47407/files) which sets/restores gc_n_threads and gc_all_tls_states, could simply be inserted into internal_obj_scan ?
There was a problem hiding this comment.
Well, the purpose of that part of the test was that all internal object references are properly recognized, so ideally we'd want to keep it around. It's to protect against some future evolution of the GC where the logic might not work for some reason or another.
As I said, I haven't had an opportunity yet to look at exactly what the problem was, but if we could just add code to internal_obj_scan() to make it possible to call jl_gc_internal_obj_base_ptr() from there, that would indeed be the ideal solution.
An alternative would be to check internal_obj_scan() inside mark_stack() and mark_stack_data(). This would require maintaining the failure counter at the C level, but might ultimately be the less hacky solution.
There was a problem hiding this comment.
The problem is more that you are looking at data that is managed by the GC without holding the GC "lock". So the data might be mutated underneath you. So if we could move this to inside the mark_stack that would be better.
|
@rbehrends when you have time it would be good to take a look at the test and re-engineer it so that it does what you need to test. But at least we are now running the tests as art of CI instead of ignoring them |
* Add test/gcext to out-of-tree * Disable gcext test that uses jl_gc_internal_obj_base_ptr (cherry picked from commit 5495b8d)
* Add test/gcext to out-of-tree * Disable gcext test that uses jl_gc_internal_obj_base_ptr (cherry picked from commit 5495b8d)
Noticed while working on #47407
This makes me wonder if
gcextis run on CI.