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

Dynamically select alignment of a dyn Trait local value #71695

Closed
pnkfelix opened this issue Apr 29, 2020 · 4 comments · Fixed by #111374
Closed

Dynamically select alignment of a dyn Trait local value #71695

pnkfelix opened this issue Apr 29, 2020 · 4 comments · Fixed by #111374
Labels
F-unsized_locals `#![feature(unsized_locals)]` P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

Spawned off of #71416

Specifically this comment: #71416 (comment)

quoted here:

Unlike the other unsound features, this is one where we don't even have a plan for how to soundly implement it. Not sure how much I like keeping that around.

Can't we do something dynamic where we allocate size + (align - 1) and align it ourselves?
Also, this has been discussed before, hasn't it?

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. F-unsized_locals `#![feature(unsized_locals)]` requires-nightly This issue requires a nightly compiler in some way. labels Apr 29, 2020
@pnkfelix
Copy link
Member Author

@rustbot prioritize

@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 29, 2020
@RalfJung
Copy link
Member

@pnkfelix (in #71694)

which actually I guess would have to be done via alloca if we cannot put an upper-bound on align

size is also statically unknown here, so yes this needs alloca, that is indeed the entire point. ;)

@spastorino
Copy link
Member

spastorino commented Apr 30, 2020

Assigning P-medium as discussed as part of the Prioritization Working Group process and removing I-prioritize.

Assigning to myself as a followup of #68304.

@spastorino spastorino added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 30, 2020
@spastorino spastorino self-assigned this May 5, 2020
@ldr709
Copy link
Contributor

ldr709 commented Dec 21, 2020

Can't we do something dynamic where we allocate size + (align - 1) and align it ourselves?

Here's a small optimization: pick some default alignment, maybe 16, then allocate size + max(align - default_align, 0) bytes. If align >= default_align then align % default_align == 0 because they are both powers of two, so the offset required to align to align bytes is i * default_align for i < align / default_align, which is at most (align / default_align - 1) * default_align = align - default_align. If align < default_align then the allocation will already be properly aligned, no offset required.

@spastorino spastorino removed their assignment Aug 30, 2022
@bors bors closed this as completed in 2c41369 May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-unsized_locals `#![feature(unsized_locals)]` P-medium Medium priority requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants