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.
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.
|
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.
|
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.
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