Annotate (most) Julia reference accesses as atomic unordered #36507
Conversation
|
What's the thing that |
|
Correct: this mostly just ensures it doesn't turn into a memcpy. There's also a monotonicity promise with relaxed (once you observe a new value, it can't un-happen) that's absent from llvm::Unordered (and present in Java, but absent from C). That can also make hoisting difficult (it's a stronger conditions on aliasing)—but our UndefVar check depends on that being prohibited already. |
|
Alright, makes sense to me. Presumably x86 memcpy could still be |
src/datatype.c
Outdated
| @@ -1057,11 +1057,12 @@ JL_DLLEXPORT jl_value_t *jl_get_nth_field_checked(jl_value_t *v, size_t i) | |||
|
|
|||
| void set_nth_field(jl_datatype_t *st, void *v, size_t i, jl_value_t *rhs) JL_NOTSAFEPOINT | |||
| { | |||
| if (rhs == NULL) // TODO: this should be invalid, but it happens frequently in ircode.c | |||
There was a problem hiding this comment.
What's the purpose of this behavior change in the context of this change (don't really have an independent opinion on the change, but seems odd here).
There was a problem hiding this comment.
We're going to potentially attempt to read the type tag (for a write barrier or to figure out how to copy the data). I suppose we should actually store the NULL, but the use case (ircode) doesn't care. It's mostly here because I had added an assert here in April, and it started failing when I tried to rebase this.
There was a problem hiding this comment.
In the ircode usage is the previous field always NULL? If so maybe assert that here?
There was a problem hiding this comment.
Yeah, it's a new object (GC hasn't run yet since allocation), otherwise we'd sometimes segfault on the write barrier a couple lines later.
There was a problem hiding this comment.
Alright, I'd say let's add that assert then, since that way there's at least no silent behavior change if anybody else thought they'd be clever by using this to unset a field.
There was a problem hiding this comment.
Might as well. We should probably also look into avoiding it in that file also, but it's not causing problems yet.
| return mark_julia_type(ctx, tbaa_decorate(tbaa_binding, ctx.builder.CreateLoad(bp)), true, (jl_value_t*)jl_any_type); | ||
| LoadInst *v = ctx.builder.CreateAlignedLoad(T_prjlvalue, bp, sizeof(void*)); | ||
| v->setOrdering(AtomicOrdering::Unordered); | ||
| tbaa_decorate(tbaa_binding, v); |
There was a problem hiding this comment.
This is technically fine since it mutates in place, but I think traditionally we've used the return value of this call, in case it ever needs to rewrite the instruction or allocate anew one.
There was a problem hiding this comment.
I think we just did that for the convenience of chaining calls, since TBAA shouldn't need to replace the instruction.
There was a problem hiding this comment.
I agree it shouldn't need to, as currently designed, but it's just the kind of thing that's bound to cause a use after free if we ever do need to change this. Maybe not worth defending against until we decide to rewrite the whole C++ part in Rust ;).
|
I'm not sure why it'd ever pick |
|
|
|
Ah, I leave it entirely up to LLVM to decide then. There's usually no chunks though, just single loads (followed by UndefVar checks). |
This lets us say that anytime you observe a pointer in memory, it's certain to not be from thin-air. It may require fences to be valid to examine the type of the object however. This is free on most hardware, though much weaker than what x86 promises.
And add load/store alignment annotations, because LLVM now prefers that we try to specify those explicitly, even though it's not required. This does not yet include correct load/store behaviors for objects with inlined references (the recent #34126 PR).
vtjnash
changed the title
vtjnash
changed the title
Followup to #36507; see the discussion there. Also slightly weakens non-atomic pointer modification, since we generally don't need DRF swap guarantees at all (even monotonic), only the atomic-release property. This would correspond to atomic-unordered failure order in many cases, but the LLVM LangRef says that this case is "uninteresting", and thus declares it is invalid. n.b. this still does not cover embedded references inside inlined structs
Followup to #36507; see the discussion there. Also slightly weakens non-atomic pointer modification, since we generally don't need DRF swap guarantees at all (even monotonic), only the atomic-release property. This would correspond to atomic-unordered failure order in many cases, but the LLVM LangRef says that this case is "uninteresting", and thus declares it is invalid. n.b. this still does not cover embedded references inside inlined structs
Per discussion in #35535 (though I made a new PR as I wanted to preserve the current state of that branch for comparison), this works towards guaranteeing that references stored in memory are valid and cannot corrupt the GC. It's free on essentially all processors, and just requests the compiler to respect our expectations of that.