Skip to content
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

Phase1 #1

Merged
merged 23 commits into from
Sep 22, 2019
Merged

Phase1 #1

merged 23 commits into from
Sep 22, 2019

Conversation

Lokathor
Copy link
Owner

@Lokathor Lokathor commented Sep 20, 2019

TODO:

  • ?

Copy link
Contributor

@MaulingMonkey MaulingMonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the advantage of (a ...).as_ref().unwrap_or_else(...) over &*(a ...)?

@Lokathor
Copy link
Owner Author

Ah, right, none. I just literally forget that you can do that

src/allocation.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MaulingMonkey MaulingMonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and I guess Box casts are maybe probably sound due to A and B having compatible Layouts?

While all the unsafe frightens me and threatens to make me spontaniously combust, I'm not seeing any more soundness concerns 👍

@Lokathor
Copy link
Owner Author

@MaulingMonkey there was a request for zeroed boxes without using the stack, so i put that in.

src/pod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MaulingMonkey MaulingMonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed new commits... approved pending fix for ZSTs in try_zeroed_box

We could update this to just not allocate in the future but I don't believe that the docs give us assurances in that area.
@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Sep 21, 2019

What about returning Ok(Box::new(T::zeroed())) ?

@Lokathor
Copy link
Owner Author

That puts T::zeroed on the stack, which is exactly what the goal was to avoid.

You're the second person to ask that one. Do yall read the doc comments?

@danielhenrymantilla
Copy link
Contributor

Sorry, I posted a global comment instead of answering to the thread about ZST in try_zeroed_box. In that case instead of erroring an Ok(Box::new(T::zeroed())) is acceptable since T is zero-sized so there is nothing in the stack.

@Lokathor
Copy link
Owner Author

ahh, hmm, that sounds like a good fix

@MaulingMonkey
Copy link
Contributor

#[inline(never)] fn try_zeroed_box_on_stack<T>() -> Box<T> {
    Ok(Box::new(T::zeroed()))
}

Would help ensure it never gets inlined onto the stack for size_of::<T>() != 0... although I imagine the branch would get optimized out in release builds, so maybe just a regular fn would be fine?

@Lokathor
Copy link
Owner Author

If we only use that for zero sized types it doesn't matter if it is "on the stack" since the size is zero anyway

@MaulingMonkey
Copy link
Contributor

MaulingMonkey commented Sep 21, 2019

Right, I'm just being paranoid about the case where non-executing code isn't fully eliminated (debug builds?), which can still contribute to stack size as I've found out before (last time I overflowed the stack was thanks to conditional logging macros, that had fixed length char buffers for formatting strings... apparently that's enough to blow the stack even when they're not executing....)

Or to put it a little more concretely, I'd worry:

if size_of::<T>() != 0 {
    Ok(Box::new(T::zeroed()))
} else {
    ...
}

Could still put size_of::<T> bytes on the stack when it's nonzero, despite the branch never executing.

@Lokathor Lokathor merged commit 7603a0f into master Sep 22, 2019
@Lokathor Lokathor deleted the phase1 branch September 22, 2019 19:07
leod pushed a commit to leod/bytemuck that referenced this pull request Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants