-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Do not generate allocations for zero sized allocations #63635
Conversation
Can you add a test, perhaps? Something that fails on master but passes with this change. |
src/librustc_codegen_llvm/common.rs
Outdated
&self.const_usize(offset.bytes()), | ||
1, | ||
)} | ||
}; | ||
let llval = self.const_bitcast(llval, self.type_ptr_to(layout.llvm_type(self))); |
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.
Could you move the bitcast to the non-zero-len arm? And compute this final pointer type only once?
src/librustc_codegen_llvm/common.rs
Outdated
)}; | ||
let llval = if layout.size == Size::ZERO { | ||
let llval = self.const_usize(alloc.align.bytes()); | ||
unsafe { llvm::LLVMConstIntToPtr(llval, self.type_ptr_to(self.type_i8p())) } |
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.
Also this seems like you're creating a i8**
instead of i8*
(but ideally we'd just go directly to the final type).
Some easy, if indirect, tests: assert_eq!(<Vec<i32>>::new().as_ptr(), <&[i32]>::default().as_ptr());
assert_eq!(<Box<[i32]>>::default().as_ptr(), (&[]).as_ptr()); |
Added tests and cleaned up the casting |
@bors r+ |
📌 Commit 1ea88a8 has been approved by |
Do not generate allocations for zero sized allocations Alternative to rust-lang#62487 r? @eddyb There are other places where we could do this, too, but that would cause `static FOO: () = ();` to not have a unique address
☀️ Test successful - checks-azure |
let x: &'static () = &(); | ||
assert_eq!(x as *const () as usize, 1); | ||
let x: &'static Foo = &Foo; | ||
assert_eq!(x as *const Foo as usize, 4); |
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.
Does this constitute a stable guarantee that we will allocate ZSTs at certain addresses? I hope not, that seems quite arbitrary to me. After all we also don't guarantee where non-ZST are allocated.
Codegen ZSTs without an allocation This makes sure that &[] is equivalent to unsafe code (from_raw_parts(dangling, 0)). No new stable guarantee is intended about whether or not we do this, this is just an optimization. This regressed in rust-lang#67000 (no comments I can see about that regression in the PR, though it did change the test modified here). We had previously performed this optimization since rust-lang#63635.
Codegen ZSTs without an allocation This makes sure that &[] is equivalent to unsafe code (from_raw_parts(dangling, 0)). No new stable guarantee is intended about whether or not we do this, this is just an optimization. This regressed in rust-lang#67000 (no comments I can see about that regression in the PR, though it did change the test modified here). We had previously performed this optimization since rust-lang#63635.
Codegen ZSTs without an allocation This makes sure that &[] is equivalent to unsafe code (from_raw_parts(dangling, 0)). No new stable guarantee is intended about whether or not we do this, this is just an optimization. This regressed in rust-lang#67000 (no comments I can see about that regression in the PR, though it did change the test modified here). We had previously performed this optimization since rust-lang#63635.
Alternative to #62487
r? @eddyb
There are other places where we could do this, too, but that would cause
static FOO: () = ();
to not have a unique address