Emit nofree attribute#156281
Conversation
|
rustbot has assigned @jdonszelmann. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
r? @RalfJung |
|
|
There was a problem hiding this comment.
Generally 👍 but I have two questions.
@rustbot author
| if deref != 0 { | ||
| // dereferenceable in LLVM currently implies nofree, so only emit dereferenceable if nofree | ||
| // is also set. | ||
| if deref != 0 && regular.contains(ArgAttribute::NoFree) { |
There was a problem hiding this comment.
We also don't want LLVM dereferenceable on return values (see the comment in compiler/rustc_ty_utils/src/abi.rs that you removed). Where is that handled now?
There was a problem hiding this comment.
It's handled here: https://github.com/rust-lang/rust/pull/156281/changes#diff-eb5b0fc5c9734679907fdb908c3a6b424ff723c4dceb364eef2588c996366708R404 That is, we never set NoFree on return values (and thus also not dereferenceable).
There was a problem hiding this comment.
That's a subtle non-local invariant and you removed the comment that explains it. Please add that comment back.
| // <https://github.com/rust-lang/miri/issues/3341> for why.) The second argument is the allocator, | ||
| // which is a reference here that still carries `noalias` as usual. | ||
| // CHECK: @_box_custom(ptr noundef nonnull align 4 %x.0, ptr noalias noundef nonnull readonly{{( captures\(address, read_provenance\))?}} %x.1) | ||
| // CHECK: @_box_custom(ptr noundef nonnull align 4 %x.0, ptr noalias nofree noundef nonnull readonly{{( captures\(address, read_provenance\))?}} %x.1) |
There was a problem hiding this comment.
Is this a nofree without a dereferenceable? I guess this is the &Global field and the pointee size is 0. The comment currently says that's not a valid combination. Given that we allow ZST references to even have dangling provenance, I don't think marking them nofree makes sense.
There was a problem hiding this comment.
So I tried out dropping nofree if pointee_size == 0, but this means that we also no longer emit nofree for slices. That seems undesirable to me.
I don't see a reason why we shouldn't use nofree for ZSTs, those are trivially nofree.
There was a problem hiding this comment.
They aren't nofree in the Rust sense. This is totally allowed even though the allocation x points to gets freed:
fn fun(x: &(), ptr: *mut u8) {
drop(Box::from_raw(ptr));
}
let ptr = Box::into_raw(Box::new(0u8));
fun(unsafe { &* ptr.cast::<()>() }, ptr);I wasn't able to produce an example that violates the LLVM notion of nofree. I'll have to look more into that.
There was a problem hiding this comment.
That sounds like an argument that we should use the same nofree definition as LLVM :)
There was a problem hiding this comment.
Just to expand a bit on why I think having nofree on slices can be useful even if they're not dereferenceable. There are basically two ways in which LLVM is interested in nofree:
- One is for speculation once we have dereferenceable-at-point semantics -- this is of course only relevant if we have dereferenceable.
- The case where an access is sunk across a call. I think the only case where this is currently done based on nofree is loop load PRE, where basically a load in the loop header gets moved to the pre-header plus after a clobbering call in the loop. This requires knowing that the clobbering call can't free the pointer. This case is useful without dereferenceable.
That said, my primary interest right now is the first case, so I'd be fine to restore the pointee_size > 0 requirement if you think using the LLVM nofree semantics is not a good idea.
There was a problem hiding this comment.
I'm entirely fine with saying that programs like this are UB:
use std::alloc::Layout;
fn inner(x: &mut (), f: fn(&mut ())) {
// `f` may mutate, but it may not deallocate!
f(x)
}
fn main() {
let ptr = Box::leak(Box::new(0i32)) as *mut i32;
inner(unsafe { &mut *(ptr as *mut ()) }, |x| unsafe {
let raw = x as *mut _ as *mut i32;
// Avoid ever creating a `Box`, we don't want any implicit accesses.
std::alloc::dealloc(raw.cast(), Layout::new::<i32>());
});
}We just have to ensure Miri agrees. I made a PR which adds tests for that.
It's kind of an accident that this gives us the LLVM nofree property (and we get a bit more than that still), but also it's a good sign IMO if this is both useful for LLVM and naturally becomes UB in Tree Borrows.
There was a problem hiding this comment.
The one case like this for which TB would not trigger UB is deallocating a zero-sized allocation this way. I don't know if LLVM has such a concept. Rust does, zero-sized statics and stack variables are a thing.
There was a problem hiding this comment.
Hm, LLVM does have zero-size allocations (e.g. alloca [0 x i8]). Though I'm not sure if "freeing" a zero-size allocation is observable in any meaningful way? (I think what we really care about here from an LLVM perspective is that "no bytes are rendered non-dereferenceable", which is true for zero-size allocations even if they are "freed".)
There was a problem hiding this comment.
Though I'm not sure if "freeing" a zero-size allocation is observable in any meaningful way?
I don't think it does -- even in Rust. An allocation being live only matters for dereferenceability and memory accesses and in both cases, the 0-size case is allowed no matter the pointer value and the other cases can't apply to a 0-sized allocation. Rust also cares about liveness for "inbounds" requirements but again a 0-sized allocation can't actually make a difference here.
I think what we really care about here from an LLVM perspective is that "no bytes are rendered non-dereferenceable", which is true for zero-size allocations even if they are "freed".
Yeah that sounds plausible.
|
Reminder, once the PR becomes ready for a review, use |
| const NoUndef = 1 << 7; | ||
| const Writable = 1 << 8; | ||
| /// It is UB for the allocation that this pointer points to to be freed | ||
| /// while the function is executing. Only valid if pointee_size > 0. |
There was a problem hiding this comment.
| /// while the function is executing. Only valid if pointee_size > 0. | |
| /// while the function is executing. Only valid if pointee_size > 0, | |
| /// and only valid on arguments (not return types). |
| const NoUndef = 1 << 7; | ||
| const Writable = 1 << 8; | ||
| /// It is UB for the allocation that this pointer points to to be freed | ||
| /// while the function is executing. Only valid if pointee_size > 0. |
There was a problem hiding this comment.
Is there some good place to assert! that NoFree implies "not a return type" and "pointee_size > 0"? I guess that should be in compiler/rustc_codegen_llvm/src/abi.rs where we consume such values.
There was a problem hiding this comment.
I ended up putting a check inside fn_abi_sanity_check(). That already verifies other ABI invariants, so I think it's also a good place to verify invariants for the attributes stored in the same structure.
|
@rustbot ready |
| // We can map our internal NoFree attribute (which forbids freeing the underlying allocation) | ||
| // to LLVM's weaker attribute (which forbids freeing it through this specific pointer). |
There was a problem hiding this comment.
| // We can map our internal NoFree attribute (which forbids freeing the underlying allocation) | |
| // to LLVM's weaker attribute (which forbids freeing it through this specific pointer). | |
| // LLVM doesn't care about zero-sized deallocation so we can map | |
| // our attribute to LLVM's. |
Maybe?
|
LGTM, thanks. :) |
Treat the semantics of pointee.size as "dereferenceable-at-point" and always specify the size. Instead, use a separate NoFree attribute to determine whether dereferenceability extends to the whole function. Then in the LLVM backend, only actually emit dereferenceable if nofree is also set, as dereferenceable currently implies nofree. In addition, explicitly emit the nofree attribute, which will help when LLVM switches dereferenceable to be at-point.
|
@bors r=RalfJung |
…uwer Rollup of 8 pull requests Successful merges: - #156281 (Emit nofree attribute) - #157305 (Eagerly decide whether relaxed bounds are allowed or not) - #148713 (rustc_borrowck: fix async closure error note to report AsyncFn rather than Fn) - #156266 (Don't ICE in has_self_borrows when coroutine captures-by-ref ty is still inferred) - #156417 (Fix an ICE in the vtable iteration for a trait reference in const eval when a supertrait is not implemented) - #156956 (Support generic params in `Lift_Generic`) - #157140 ( rustc_target: Use rustc_abi instead of cfg_abi to detect powerpcspe ) - #157423 (Refactor/expand rustc_attr_parsing docs)
Rollup merge of #156281 - nikic:nofree, r=RalfJung Emit nofree attribute Treat the semantics of pointee.size as "dereferenceable-at-point" and always specify the size. Instead, use a separate NoFree attribute to determine whether dereferenceability extends to the whole function. Then in the LLVM backend, only actually emit dereferenceable if nofree is also set, as dereferenceable currently implies nofree. In addition, explicitly emit the nofree attribute, which will help when LLVM switches dereferenceable to be at-point. Relevant recent LLVM PR: llvm/llvm-project#195658
|
@rust-timer build c770949 |
This comment has been minimized.
This comment has been minimized.
|
Oh yeah this will probably be a slowdown because adding more attributes in LLVM is expensive.
|
|
Finished benchmarking commit (c770949): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.7%, secondary 3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.5%, secondary -3.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 512.944s -> 510.113s (-0.55%) |
|
@rustbot label: +perf-regression-triaged |
View all comments
Treat the semantics of pointee.size as "dereferenceable-at-point" and always specify the size. Instead, use a separate NoFree attribute to determine whether dereferenceability extends to the whole function.
Then in the LLVM backend, only actually emit dereferenceable if nofree is also set, as dereferenceable currently implies nofree.
In addition, explicitly emit the nofree attribute, which will help when LLVM switches dereferenceable to be at-point.
Relevant recent LLVM PR: llvm/llvm-project#195658