Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upCompute LLVM-agnostic type layouts in rustc. #32939
Conversation
rust-highfive
assigned
nrc
Apr 13, 2016
This comment has been minimized.
This comment has been minimized.
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
eddyb
force-pushed the
eddyb:layout
branch
2 times, most recently
from
c6564a1
to
b9022d2
Apr 13, 2016
eddyb
added
the
S-waiting-on-crater
label
Apr 13, 2016
arielb1
reviewed
Apr 13, 2016
| } | ||
| } | ||
|
|
||
| // Odd unit types. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
eddyb
Apr 13, 2016
Author
Member
I don't really see the need for a dedicated unit layout, but Layout::Empty would probably be useful in refining the set of variants, to make sizeof(Result<T, Void>) just sizeof(T).
However, just like invalid value range reusal (i.e. extending non-zero optimizations), it cannot be done before moving trans to use only ty::Layout.
I was hoping we can test the worst-case transmute approach before the beta, but there were complications I had to address.
eddyb
changed the title
[WIP] Compute LLVM-agnostic type layouts in rustc.
Compute LLVM-agnostic type layouts in rustc.
Apr 13, 2016
solson
reviewed
Apr 13, 2016
| } | ||
|
|
||
| impl Default for TargetDataLayout { | ||
| fn default() -> TargetDataLayout { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
eddyb
Apr 14, 2016
Author
Member
They're LLVM's defaults: http://llvm.org/docs/LangRef.html#data-layout.
This comment has been minimized.
This comment has been minimized.
Aatch
Apr 14, 2016
Contributor
Looks like the defaults match LLVMs defaults for the data layout string. The data layout string isn't required to specify every value, so the defaults are used to fill in the gaps.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
My main worry on these data layouts was just that no one actually understands them and they're just opaque blobs we copy around without any thought, and then once that caused a bug one day and LLVM had reasonable defaults for everything it seemed "why not just use that?" If we validate that our data layout is the same as LLVM, however, then this seems fine to me. I'd be curious if LLVM actually changes anything here between releases (wouldn't that be a huge breaking change for C as well?), but doing this ourselves also seems fine to me. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton They're documented, though: http://llvm.org/docs/LangRef.html#data-layout. |
This comment has been minimized.
This comment has been minimized.
|
I've taken a look at the preliminary list of regressions, and even though almost half of them were build failures (timeouts?), the rest displayed a clear pattern:
That is, in the latter case, you might have Assuming we want to allow the pattern of casting between maybe-newtyped pointers to maybe-unsized maybe-wrapped type parameters, how should we go about doing it? The cleanest solution I have in mind, although not the most principled one, is to have two checks:
While it may be possible to integrate unknown sizes into the regular EDIT: I should mention that if breaking all of those crates is an option, most of them can use pointer casts and dereferences instead - even when a newtype or |
eddyb
added
I-needs-decision
and removed
S-waiting-on-crater
labels
Apr 14, 2016
This comment has been minimized.
This comment has been minimized.
I think regions should be ignored/substituted here. Makes more general sense, since the regions shouldn't affect the layout of the type. |
This comment has been minimized.
This comment has been minimized.
rust-highfive
assigned
nikomatsakis
and unassigned
nrc
Apr 15, 2016
This comment has been minimized.
This comment has been minimized.
I think we do.
I don't have an informed enough opinion yet. I'll try to take a look at your code but it may not happen until Monday, since I'm kind of busy today with other (non-Rust-related) things. But it seems to me like we could probably use a similar trick to what we do today, where for things where |
This comment has been minimized.
This comment has been minimized.
Oh, and clearly regions shouldn't affect the result, but I would expect this equivalency to fallout in a more general way than strict type equality? |
This comment has been minimized.
This comment has been minimized.
|
@nikomatsakis Just trying out combinations is pretty fragile, if we ever want to add more kinds of fat pointer metadata (or if we make it fully custom). The "perfect" solution IMO is to enforce static sizes because that would allow a That's because we could end up simplifying, e.g. |
This comment has been minimized.
This comment has been minimized.
I see. Good point. |
nikomatsakis
reviewed
Apr 15, 2016
| pub struct Size { | ||
| raw: u64 | ||
| pub bytes: u64 |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Apr 15, 2016
Contributor
I don't really care, but I imagine the use of privacy here was to help ensure people don't do stupid things like let mut size = ...; size.bytes *= 2;, but rather encourage them to go through these (presumably more careful) APIs.
This comment has been minimized.
This comment has been minimized.
eddyb
Apr 16, 2016
Author
Member
I actually initially wanted to use the highest bit to indicate that the rest of the size is the base size of an unsized type, instead of an exact size, but I ended up dealing with unsized types differently.
I could go back to private raw and add more checks for 2^61 overflow if you want to.
eddyb
added some commits
Apr 18, 2016
eddyb
force-pushed the
eddyb:layout
branch
from
b9022d2
to
c7d564d
Apr 19, 2016
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Apr 20, 2016
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@bors retry |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Apr 20, 2016
GuillaumeGomez
reviewed
Apr 20, 2016
| @@ -1410,6 +1410,32 @@ It is not possible to use stability attributes outside of the standard library. | |||
| Also, for now, it is not possible to write deprecation messages either. | |||
| "##, | |||
|
|
|||
| E0512: r##" | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
merged commit c7d564d
into
rust-lang:master
Apr 20, 2016
eddyb
deleted the
eddyb:layout
branch
Apr 20, 2016
eddyb
added
the
beta-nominated
label
Apr 20, 2016
This comment has been minimized.
This comment has been minimized.
|
Nominated for backporting to beta to fix #32377. |
Aatch
referenced this pull request
Apr 20, 2016
Merged
Various improvements to MIR and LLVM IR Construction #32980
solson
reviewed
Apr 22, 2016
| } | ||
|
|
||
| /// Helper function for normalizing associated types in an inference context. | ||
| fn normalize_associated_type<'a, 'tcx>(infcx: &InferCtxt<'a, 'tcx>, |
This comment has been minimized.
This comment has been minimized.
solson
Apr 22, 2016
•
Member
@eddyb In Miri I use rustc::infer::normalize_associated_type. Its source code is slightly different and has a warning about only being callable from trans, but most of it is duplicated. Could they be unified somehow?
This comment has been minimized.
This comment has been minimized.
eddyb
Apr 22, 2016
Author
Member
See how I call ty.layout(&infcx) from trans. The effect is the same in that case, it's only different elsewhere (intrinsicck).
NilSet
referenced this pull request
Apr 23, 2016
Merged
Add data-layout fields to build targets. #622
hannobraun
pushed a commit
to hannobraun/embedded
that referenced
this pull request
Apr 26, 2016
arielb1
referenced this pull request
Apr 27, 2016
Closed
tuples with unsized tails cause an error when compiled #33241
This comment has been minimized.
This comment has been minimized.
|
after a week of exercising in the nightly release, the compiler team has decided to accept this for beta, largely because it fixes a real regression (#32377), and that outweighed the relative risk of backporting such a largish change to beta. |
eddyb commentedApr 13, 2016
•
edited
Layout for monomorphic types, and some polymorphic ones (e.g.
&TwhereT: Sized),can now be computed by rustc without involving LLVM in the actual process.
This gives rustc the ability to evaluate
size_oforalign_of, as well as obtain field offsets.MIR-based CTFE will eventually make use of these layouts, as will MIR trans, shortly.
Layout computation also comes with a
[breaking-change], or two:"data-layout"is now mandatory in custom target specifications, reverting the decision from #27076.This string is needed because it describes endianness, pointer size and alignments for various types.
We have the first two and we could allow tweaking alignments in target specifications.
Or we could also extract the data layout from LLVM and feed it back into rustc.
However, that can vary with the LLVM version, which is fragile and undermines stability.
For built-in targets, I've added a check that the hardcoded data-layout matches LLVM defaults.
transmutecalls are checked in a stricter fashion, which fixes #32377To expand on
transmute, there are only 2 allowed patterns: between types with statically known sizes and between pointers with the same potentially-unsized "tail" (which determines the type of unsized metadata they use, if any).If you're affected, my suggestions are:
transmutewhere possibletransmute::<RefCell<Box<T>>, RefCell<*mut T>>(x)doesn't work, butRefCell::new(Box::into_raw(x.into_inner()))does (andBox::into_rawis just atransmute)