[AVR] Correctly set the pointer address space when constructing pointers to functions #73270
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
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
ea88e97 to
eba247c
Compare
dylanmckay
commented
Jun 12, 2020
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
eba247c to
70b3f99
Compare
jfrimmel
reviewed
Jun 12, 2020
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
70b3f99 to
da62082
Compare
|
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 |
This was referenced Jun 14, 2020
shepmaster
reviewed
Jun 17, 2020
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
4 times, most recently
from
5a05b41 to
da4e901
Compare
|
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 |
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
2 times, most recently
from
326b9d8 to
c44ea2c
Compare
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
c44ea2c to
9df636e
Compare
|
☔ The latest upstream changes (presumably #73504) made this pull request unmergeable. Please resolve the merge conflicts. |
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
9df636e to
336322c
Compare
|
☔ The latest upstream changes (presumably #73528) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Can we remove the hacks we made to the formatters with this PR? |
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
a3ff396 to
43d0a3a
Compare
I reverted 91bff8c from Rust master locally on top of this patch but then it breaks libcore So it looks like we cannot remove the hacks we made to the formatters in 91bff8c with this PR. |
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
075eb4a to
d348eca
Compare
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
d348eca to
5a3f398
Compare
|
NOTE: I'm keeping tags of all of the iterations of this patch so we can compare and/or pull anything back in. https://github.com/dylanmckay/rust/releases/tag/avr-use-correct-addrspace.0 |
|
@dylanmckay is this still a draft? The implementation looks good to me… on a quick skim, anyway. |
|
@nagisa I haven't got around to writing it up properly, but yeah the implementation is no longer a draft - this is about as minimal as I could get the patch using the "stop using pointercast, always use bitcast" approach. I prefer this current approach to the boilerplate-heavy one located at the The only thing left before sending this PR back to review is... writing tests for it. I will flip this out of draft once that is done. |
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
5a3f398 to
97b6b9b
Compare
…ers to functions This patch extends the existing `type_i8p` method so that it requires an explicit address space to be specified. Before this patch, the `type_i8p` method implcitily assumed the default address space, which is not a safe transformation on all targets, namely AVR. The Rust compiler already has support for tracking the "instruction address space" on a per-target basis. This patch extends the code generation routines so that an address space must always be specified. In my estimation, around 15% of the callers of `type_i8p` produced invalid code on AVR due to the loss of address space prior to LLVM final code generation. This would lead to unavoidable assertion errors relating to invalid bitcasts. With this patch, the address space is always either 1) explicitly set to the instruction address space because the logic is dealing with functions which must be placed there, or 2) explicitly set to the default address space 0 because the logic can only operate on data space pointers and thus we keep the existing semantics of assuming the default, "data" address space.
…tated with the correct space Before this patch, a function pointer stored within an aggregate like a struct or an enum would always have the default address space `0`. This patch removes this assumption and instead, introspects the inner type being pointed at, storing the target address space in the PointeeInfo struct so that downstream users may query it.
dylanmckay
force-pushed
the
avr-use-correct-addrspace
branch
from
97b6b9b to
5581ce6
Compare
|
I've added a few tests. Whilst writing the tests, I noticed that I no longer needed to apply @eddyb's suggestion that changed all Back to review. |
|
@bors r+ |
|
📌 Commit 5581ce6 has been approved by |
bors
added
S-waiting-on-bors
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jul 22, 2020
…e, r=nagisa [AVR] Correctly set the pointer address space when constructing pointers to functions NOTE: Pull request iterations: * https://github.com/dylanmckay/rust/releases/tag/avr-use-correct-addrspace.0 * https://github.com/dylanmckay/rust/releases/tag/avr-use-correct-addrspace.1 * https://github.com/dylanmckay/rust/releases/tag/avr-use-correct-addrspace.2 This patch extends the existing `type_i8p` method so that it requires an explicit address space to be specified. Before this patch, the `type_i8p` method implcitily assumed the default address space, which is not a safe transformation on all targets, namely AVR. The Rust compiler already has support for tracking the "instruction address space" on a per-target basis. This patch extends the code generation routines so that an address space must always be specified. In my estimation, around 15% of the callers of `type_i8p` produced invalid code on AVR due to the loss of address space prior to LLVM final code generation. This would lead to unavoidable assertion errors relating to invalid bitcasts. With this patch, the address space is always either 1) explicitly preserved from the input type, or 2) explicitly set to the instruction address space because the logic is dealing with functions which must be placed there, or 3) explicitly set to the default address space 0 because the logic can only operate on data space pointers and thus we keep the existing semantics of assuming the default, "data" address space.
|
☀️ Test successful - checks-actions, checks-azure |
RalfJung
reviewed
Dec 28, 2022
Comment on lines
+2185
to
+2186
| size: layout.size, | ||
| align: layout.align.abi, |
There was a problem hiding this comment.
FWIW this makes very little sense. This sets the pointee size and alignment of function pointers to be the pointer size and alignment, i.e. on x86-64 this will be size 8 alignment 8, which is just completely wrong -- function pointers do not point to 8-aligned data.
Luckily these two fields are, from what I can tell, completely unused if safe is None (which does make me wonder why they are not inside the same Option).
There was a problem hiding this comment.
Actually sounds like the entire approach of using PointeeInfo to carry address space information is fundamentally broken; see this Zulip discussion.
RalfJung
reviewed
Dec 28, 2022
| unsafe { | ||
| STORAGE_FOO(&1, &mut buf); | ||
| } | ||
| } |
There was a problem hiding this comment.
What happens when you store a function pointer in a type like Result<&'static u8, fn()>? (Example due to @eddyb)
In LLVM this type gets translated as { i64, ptr }, where the first field indicates Ok vs Err and the 2nd field contains the data (&u8 or fn(), depending on the first field). So it is fundamentally impossible to have a dedicated function pointer address space here. Doesn't sound like the approach taken in this PR can possibly work here?
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.
NOTE: Pull request iterations:
This patch extends the existing
type_i8pmethod so that it requires anexplicit address space to be specified. Before this patch, the
type_i8pmethod implcitily assumed the default address space, which isnot a safe transformation on all targets, namely AVR.
The Rust compiler already has support for tracking the "instruction
address space" on a per-target basis. This patch extends the code
generation routines so that an address space must always be specified.
In my estimation, around 15% of the callers of
type_i8pproducedinvalid code on AVR due to the loss of address space prior to LLVM final
code generation. This would lead to unavoidable assertion errors
relating to invalid bitcasts.
With this patch, the address space is always either 1) explicitly preserved
from the input type, or 2) explicitly set to the instruction address
space because the logic is dealing with functions which must be placed
there, or 3) explicitly set to the default address space 0 because the
logic can only operate on data space pointers and thus we keep the
existing semantics of assuming the default, "data" address space.