Limit maximum alignment in #[repr(align)] to 4K #70193
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
|
⌛ Trying commit 290ac8d with merge 140461fb7e517404cf26f5c951fd8629cc232828... |
Centril
reviewed
Mar 20, 2020
| Ok(*literal as u32) | ||
| } else { | ||
| Err("larger than 2^29") | ||
| Err("larger than 4096") |
There was a problem hiding this comment.
if we land this, then it would be good to mention what this magic number is about
Centril
reviewed
Mar 20, 2020
| Ok(*literal as u32) | ||
| } else { | ||
| Err("larger than 2^29") | ||
| Err("larger than 4096") |
There was a problem hiding this comment.
Also, let's add a regression test at (4096 + 1).next_power_of_two()
| @@ -67,9 +67,9 @@ struct AlignContainsPacked4C { | |||
|
|
|||
| // The align limit was originally smaller (2^15). | |||
| // Check that it works with big numbers. | |||
There was a problem hiding this comment.
Irrelevant for crater run but this comment is now wrong.
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-author
|
@bors retry |
|
⌛ Trying commit 290ac8d with merge e236cb20f9a6d9a97f7e915fabdff9ef9fdcafc6... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors try |
|
⌛ Trying commit 3f1b797 with merge 8a2b22251fe99d18beab8fdca0e241231799a8b3... |
|
☀️ Try build successful - checks-azure |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
Centril
added
I-nominated
T-lang
|
Limiting to 4k alignment could hurt specialized structures in an embedded context. For example, I haved used align(32k) (or rather the equivalent) to place objects in a half-bank on 65816. Half banks are specifically important, as that is conventionally the mapping division point, and the largest page size used by the mapper. It also makes it easier to place something at a precise address with fewer linker hacks. |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🤣 I support this change but I am not a lang team member and I'm mostly unfamiliar with the code involved... I'll see if I can find someone. |
|
Quoting @joshtriplett from Zulip:
If targets can override this, we need to find a good way to make sure people do not think that unsafe code can rely on 4k being the maximum alignment. (Some people seem to already think that now, for some reason.) |
|
If you look at the list of pages sizes per archtecture, you'll see that all architectures have 4KB as their smallest page size. So I don't think it really makes sense to make this a per-target attribute. |
|
With respect to 65816, I specify for C/++ that the maximum alignment of any
type is 32k. It doesn't really make sense to impose something stricter on
statics in rust. I would want a per-target attribute, if only so I can
support the same basic limitations in all languages I support.
…On Wed, May 13, 2020 at 15:45 Amanieu ***@***.***> wrote:
If you look at the list
<https://en.wikipedia.org/wiki/Page_(computer_memory)#Multiple_page_sizes>
of pages sizes per archtecture, you'll see that all architectures have 4KB
as their smallest page size. So I don't think it really makes sense to make
this a per-target attribute.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#70193 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGLD263DGJXAGTK6GQY6D3RRL2GHANCNFSM4LQQCBOA>
.
|
|
@Amanieu I believe part of the motivation here was that the limitation of 4K only arises because of interactions with the dynamic linker on some systems (and some LLVM limitations, I believe), and the hypothesis was that on some embedded platforms those things would be non-issues (and there would be legitimate reasons to want higher alignment requirements). It does seem like 'per target' may not be the most direct way to reason about those limits though (e.g., I may be on some target that supports dynamic linking but I may not plan to use it). |
|
How about this: alignments greater than 4K are not allowed when generating PIC (position independent) code. |
|
When compiling an rlib, I don't think we know if that code will be position-independent or not -- or do we? |
|
At least as long as rlibs can contain machine code and not just MIR, we (need to) know. But if this is a monomorphization time check (I think it might needs to be, due to promotion in generic functions?), that won't help all code, nor will it fire in |
|
It is not currently a monomorphization time check, though that was proposed as a possible alternative. |
joelpalmer
added
S-waiting-on-author
Elinvynia
added
S-waiting-on-author
Elinvynia
added
S-waiting-on-author
crlf0710
added
S-waiting-on-author
Muirrum
added
S-waiting-on-author
|
@Amanieu what's the status of this? |
|
Closing this after a discussion with the author. |
Dylan-DPC-zz
added
S-inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Let's try this on crater to see the impact.
cc #70022 #70143 #70144