-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP/RFC: stack scrubbing #12317
WIP/RFC: stack scrubbing #12317
Conversation
4ef4fe6
to
f25f437
Compare
// singleton object), this usually returns the same pointer which points to | ||
// the next object but it can also return NULL if the pointer is pointing to | ||
// the end of the page. | ||
DLLEXPORT jl_taggedvalue_t *jl_gc_find_taggedvalue_pool(char *p, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this go into gcdebug.c ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep it here for now since it's potentially useful for the GC itself as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we need it in this form we'll move it back I think, gc.c is already messy enough not to add dead functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k. sure..
sorry forgot to review this. Could you try to see if it works by removing a useful root on purpose somewhere ? other than the minor comment about files, if it works it's good to go |
Instead of trashing pointer-like values on the stack, we fill dead objects that is possibly referred to from the stack with garbage.
Suggestions implemented. It's very hard to come up with a case that only reliably triggers an error with this PR mainly because it doesn't increase GC frequency. I believe it will make debugging GC easier because it make a missing GC root fail fast instead of generating some wierd error much later. The following script at least show that it works. @noinline ptr1() = ccall(:jl_astaggedvalue, Ptr{UInt}, (Any,),
Ref{UInt}(0x12345))
@noinline ptr2() = ccall(:jl_astaggedvalue, Ptr{UInt}, (Any,),
Ref{UInt}(0x12345)) + sizeof(Int) * 2
@noinline function load1(ptr)
i1 = unsafe_load(ptr)
i2 = unsafe_load(ptr + sizeof(Int))
ptr, i1, i2
end
@noinline function load2(ptr)
i1 = unsafe_load(ptr - 2 * sizeof(Int))
i2 = unsafe_load(ptr - sizeof(Int))
ptr, i1, i2
end
function f1()
ptr = ptr1()
gc()
load1(ptr)
end
function f2()
ptr = ptr2()
gc()
load2(ptr)
end
println(f1())
println(f2()) Output on master
Output on this branch with the build option enabled.
The output clearly shows that the scrubing only happens when the pointer is stored on the stack. Will merge when the CI is green again. |
@@ -259,6 +259,20 @@ static int check_timeout = 0; | |||
static gcpage_t *page_metadata(void *data); | |||
static void pre_mark(void); | |||
static void post_mark(arraylist_t *list, int dryrun); | |||
static region_t *find_region(void *ptr, int maybe); | |||
jl_taggedvalue_t *jl_gc_find_taggedvalue_pool(char *p, size_t *osize_p); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For MSVC, you need DLLEXPORT
to be consistent between the prototype and the implementation. It should either be on both or neither. Which should this be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Export for debugging purpose THX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not by my computer now, will fix it later. Feel free to do it if you need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's okay, just came up in #12056. We don't do the MSVC build on CI right now, so just trying to remind people who touch the C code of some of its constraints so hopefully it breaks a little less often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tkelman Merged. Please let me know if there's still an issue.
This is a rewrite of @carnaval 's previous work in #10725. The main differences are,
(As suggested by Oscar) Instead of messing with stack data, we fill the dead object that might be referenced by the stack with garbage (
0xff
ATM) so that we won't have any data corruption.Split out the logic for conservative pool scan (i.e. figure out if a pointer might be refering a GC object in the pool)
This function is useful for a number of other things including conservative stack scan (either for GC root conservative stack scanning? #11714, or for pinning conservative stack scanning? #11714 (comment)), ccall sanitizer RFC:
ccall
sanitizer #12173 and gc debugging in general.