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

Implementation of repr struct alignment RFC 1358. #39999

Merged
merged 4 commits into from Apr 22, 2017

Conversation

@bitshifter
Copy link
Contributor

bitshifter commented Feb 21, 2017

The main changes around rustc::ty::Layout::struct:

  • Added abi_align field which stores abi alignment before repr align is applied
  • align field contains transitive repr alignment
  • Added padding vec which stores padding required after fields

The main user of this information is rustc_trans::adt::struct_llfields
which determines the LLVM fields to be used by LLVM, including padding
fields.

A possible future optimisation would be to put the padding Vec in an Option, since it will be unused unless you are using repr align.

@rust-highfive
Copy link
Collaborator

rust-highfive commented Feb 21, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Copy link
Member

eddyb left a comment

Overall this looks pretty solid, I mostly have an issue with naming of abi_align that conflicts with the existing semantics.

pub align: Align,

/// Alignment of struct fields, for comparison with LLVM type.
pub abi_align: Align,

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

This name is confusing IMO, since each Align tracks two values ("preferred" and "ABI-required") - fields_align is probably better (if at all needed?).

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 21, 2017

Author Contributor

Naming is hard :) The purpose off this field is to keep track of what LLVM thinks the struct's alignment is. This is used in a few different ways:

Originally I called this field base_align which you also suggested in another comment, so perhaps I should go back to that. Other options might be llvm_align, default_align, orig_align.

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

We shouldn't be talking about LLVM in librustc, ideally. How about primitive_align? That is, the alignment obtained from primitives' alignment, without any user annotations.

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 21, 2017

Author Contributor

primitive_align works for me.

@@ -541,6 +545,9 @@ pub struct Struct {
pub memory_index: Vec<u32>,

pub min_size: Size,

/// Size of padding needed by the struct due to requested repr alignment
pub padding: Vec<Size>,

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Can't this be computed on-demand based on the sizes of the fields and the alignments we can get out of LLVM types? Keep in mind constant enum variant values already have to deal with recomputing padding based on the values at hand, I'd rather generalize that algorithm than have this vector.

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 21, 2017

Author Contributor

You comment on this later so I guess you are OK with it?

It could be done on demand, but I thought it would be more overhead as it's needed in a couple of places (https://github.com/bitshifter/rust/blob/6b9c5d1ff6df1e0d6d181fb5049c3108359b47ee/src/librustc_trans/adt.rs#L265-L266 and https://github.com/bitshifter/rust/blob/6b9c5d1ff6df1e0d6d181fb5049c3108359b47ee/src/librustc/ty/layout.rs#L775-L776).

If we keep the vector I was thinking of putting it in an Option, since I think most people won't need repr(align) so it would generally be all zeros.

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Yeah it's fine if an on-demand iterator happens to use it.

@@ -560,13 +567,27 @@ impl<'a, 'gcx, 'tcx> Struct {
repr: &ReprOptions, kind: StructKind,
scapegoat: Ty<'gcx>) -> Result<Struct, LayoutError<'gcx>> {
let packed = repr.packed;
let repr_align = repr.align;

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

IMO copying fields out of the struct like that is confusing, both should be referred as repr.foo.

pub fn memory_index_with_padding_fields(&self, index: usize) -> usize {
let index = self.memory_index[index] as usize;
self.padding.iter().take(index).fold(index, |acc, &pad|
acc + if pad.bytes() > 0 { 1 } else { 0 })

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Okay, this seems like an okay use of the padding vector.

}

/// An untagged union.
#[derive(PartialEq, Eq, Hash, Debug)]
pub struct Union {
pub align: Align,
pub abi_align: Align,

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Same, although field_align might not work as well here. member_align might work for both.

@@ -1513,6 +1603,30 @@ impl<'a, 'gcx, 'tcx> Layout {
}
}
}

/// Returns alignment before repr alignment is applied
pub fn abi_align(&self, dl: &TargetDataLayout) -> Align {

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Maybe content_align? Or, given that it seems to be recursive, base_align? (user-specified alignment is still ABI alignment, in the sense that it affects padding, whereas preferred alignment is for allocations, e.g. to speed up SSE).

@@ -1363,6 +1364,7 @@ pub struct ReprOptions {
pub packed: bool,
pub simd: bool,
pub int: Option<attr::IntType>,
pub align: Option<u64>,

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

I'd use an u16 and default it to 0 (we don't support larger alignments anyway - if you want to, you can use a pair of bytes Align instead of just a byte, bringing the max alignment from 2^15 to 2^255).

@@ -1294,6 +1298,33 @@ pub fn check_simd<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, sp: Span, def_id: DefId
}
}

fn has_align<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, root_def_id: DefId, def_id: DefId) -> bool {

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

This looks like it could easily go into infinite recursion.

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 21, 2017

Author Contributor

I was assuming that structs couldn't reference themselves but probably that's assuming too much :)

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Wait, this only applies to nested structs? That explains why no regressions, however, you probably want to handle it so you don't infinitely recurse in the compiler when some other part of the compiler wants to report a non-fatal error about it (see AdtDef::calculate_sized_constraint_inner, you probably don't need the cache though, just the stack to check for cycles).

if let Ok(align) = u64::from_str(&*value.as_str()) {
if align.is_power_of_two() {
// rustc::ty::layout::Align restricts align to <= 32768
if align <= 32768 {

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Oh there it is 😄.

}
} else {
span_err!(diagnostic, item.span, E0584,
"alignment representation hint is not a u64");

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Oh, we could probably use a single error message for an invalid #[repr(align)] (also, "hint" is probably the wrong thing to call it?).

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 21, 2017

Author Contributor

I wasn't sure what the done thing was, lots of specific errors versus one general one. The "hint" part was just from copying the language used in other error messages. I can change it.

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 22, 2017

Author Contributor

Rebased with master and got conflicts on these error numbers, so this all got a bit messed up. Still working on the other feedback.

if unrecognised {
// Not a word we recognize
span_err!(diagnostic, item.span, E0552,
"unrecognized representation hint");

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

This and the one above it seem like they serve the same function?

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 21, 2017

Author Contributor

Yes, I think it's because there are different code paths for name and named value and for a while I had different error messages. I'll collapse to a single message.

@@ -877,10 +886,38 @@ pub fn find_repr_attrs(diagnostic: &Handler, attr: &Attribute) -> Vec<ReprAttr>
if let Some(h) = hint {
acc.push(h);
}
} else if let Some((name, value)) = item.name_value_str() {

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Seems like you want to support only #[repr(align = "16")] but not #[repr(align(16))]?
@alexcrichton used to think otherwise, but I hope he can be persuaded to only allow the latter form, as it's already implemented and thus likelier to be stabilized first. In fact, we can stabilize it for specific attributes (like this one) and leave the general case unstable.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Feb 21, 2017

Member

My historical position was that #[repr(align(16))] is grammatically invalid (e.g. would simply not parse), but if we've changed the grammar since then then that seems fine by me.

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 21, 2017

Author Contributor

Sure, [repr(align(16))] is much nicer. Do you know what else is using that form so I can see how it's implemented?

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 21, 2017

Member

Not sure anything else does that already, but for the record, it's a MetaItemKind::List (.meta_item_list() on Attribute) with a single NestedMetaItemKind::Literal element (.literal() on NestedMetaItem), that's a LitKind::Int(16, LitIntType::Unsuffixed).

@bors
Copy link
Contributor

bors commented Feb 21, 2017

The latest upstream changes (presumably #39765) made this pull request unmergeable. Please resolve the merge conflicts.

@bitshifter
Copy link
Contributor Author

bitshifter commented Feb 24, 2017

@eddyb hopefully I've address all your feedback. I made a couple of additional changes:

  • Made the Struct padding vector empty unless it's actually used, since the common case won't need it
  • added a print-type-sizes test
@@ -546,7 +546,8 @@ pub struct Struct {

pub min_size: Size,

/// Size of padding needed by the struct due to requested repr alignment
/// Size of padding needed by the struct due to requested repr alignment.
/// This is created as required and will be empty if the structs fields require no padding.
pub padding: Vec<Size>,

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 24, 2017

Member

I'm tempted to say that we should have offsets and field sizes, which is more general, and then creating a LLVM type can compute how much padding to use between each two consecutive fields. Check out rust-lang/miri#146 (comment), where I have a potential use for this: removing the implied discriminant field of enum variant Structs, which in an Vec<(field_offset, field_size)> encoding just be the first field starting e.g. 4 bytes in and thus the LLVM type having a padding field there.

We might also be able to always emit padding fields (sometimes zero-sized) and get LLVM to play along.
cc @rust-lang/compiler Anyone think this is terrible idea? IMO it might be better than memory_index_with_padding_fields which makes field access O(n) instead of O(1).

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 24, 2017

Author Contributor

You mean replacing the Struct offset Vec with (field_offset, field_size) tuple and calculating padding on demand when the LLVM type is created? This on it's own wouldn't remove the need for memory_index_with_padding though and translating the index to include padding fields would get more expensive. If you stored the adjusted memory index also then that could be avoided.

I did think about always emitting padding even for primitive aligned types but I wasn't sure what impact that might have on LLVM internals, does that mean a lot more work for it to do? Is it possible to emit a zero sized field? Generally I've taken a cautious approach and only changed behaviour if repr align is being used. It would simplify some things if you could assume that every field has a padding field though.

BTW, the memory_index_with_padding_fields is now only O(n) if repr align is being used.

This comment has been minimized.

Copy link
@eddyb

eddyb Feb 24, 2017

Member

Sure, [0 x i8] or {} (LLVM's 0-tuple, our ()) both work to that effect (LLVM ZST).

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 24, 2017

Author Contributor

Ok, so if we emit all padding fields we can make memory_index_with_padding_fields always O(1) (basically index * 2) and calculate padding on demand. Will wait and see what @rust-lang/compiler says.

This comment has been minimized.

Copy link
@bitshifter

bitshifter Mar 1, 2017

Author Contributor

I did have a couple of unsuccessful attempts at always emitting padding fields. I basically wasn't able to get to the point where I could actually build the compiler, I think it was libcore that I was failing on. I'm not sure how to best debug that. In any case it seemed like it might be a non-trivial change, it seems somewhat independent to the repr align changes, but would make the repr align stuff cleaner. Was there any comment from the compiler team about it?

#[inline]
pub fn memory_index_with_padding_fields(&self, index: usize) -> usize {
if self.padding.is_empty() {
index

This comment has been minimized.

Copy link
@bitshifter

bitshifter Feb 24, 2017

Author Contributor

Whoops. This should be self.memory_index[index].

@bors
Copy link
Contributor

bors commented Feb 28, 2017

The latest upstream changes (presumably #40008) made this pull request unmergeable. Please resolve the merge conflicts.

@bitshifter bitshifter force-pushed the bitshifter:struct_align branch from 158465a to c006f5a Mar 1, 2017
#![feature(attr_literals)]
#![allow(dead_code)]

#[repr(align(16))]

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 2, 2017

Contributor

Should these attributes be feature-gated?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 2, 2017

Contributor

To be clear: I know there is the attr_literals feature gate, but I mean should align specifically be feature-gated?

This comment has been minimized.

Copy link
@bitshifter

bitshifter Mar 2, 2017

Author Contributor

I don't think I have the rustc experience to answer that question :) Happy to add a feature gate if it's needed.

@eddyb
Copy link
Member

eddyb commented Mar 2, 2017

Continuing #39999 (comment): at the compiler teem meeting this week @nikomatsakis seemed fine with the change to always emit padding fields, as long as it had no impact on ABI (and AFAICT, it can't).

@bitshifter Can you push the branch that you couldn't get working somewhere? Maybe I can help.

@bitshifter
Copy link
Contributor Author

bitshifter commented Mar 2, 2017

@eddyb sure, I haven't spent a huge amount of time on it so probably I'm just doing something silly. I've committed my current WIP to bitshifter@00eb421. What I'm hitting at the moment is some of my asserts around expected offsets and sizes in struct_llfields are being hit when building stage 1 and I haven't worked out why yet. The stride check at the end of that function is commented because it was also failing, which was counter to my expectations too. I haven't worked out yet what struct is being created that triggers these. Hopefully it's not all structs, it's been a week since I tried so no guarantees it's not just totally broken :)

@bitshifter
Copy link
Contributor Author

bitshifter commented Mar 5, 2017

@eddyb I've pushed some changes to that branch so the padding fields are hopefully correct now. I've added a lot of asserts and debug output to rustc_trans::adt::struct_llfields and it seemed to be working OK. Now I'm back to getting a segfault when building stage 1 libcore (my original attempt was hitting this). The tricky thing is how to debug what's going on.

Just as I was writing this comment I was trying to attach rust-gdb to the compiler's pid, which seemed to have worked, and I have a stack trace http://pastebin.com/3b8E8Xez. I might take a break from this for now but that trace looks like it should point me in the right direction. If you have any insights or tips for debugging this let me now.

I guess in particular I don't have a good understanding of the different phases of building the compiler. Stage 0 is just downloaded right? Stage 1 appears to start self hosting? At this point it must have built some of the compiler and has started building other libs with that I assume.

@bitshifter
Copy link
Contributor Author

bitshifter commented Mar 6, 2017

More invesitgation, log output for rustc_trans where the crash occurs:

DEBUG:rustc_trans::mir::statement: trans_statement(statement=_14 = ((_11 as Some).0: <Self as iter::iterator::Iterator>::Item))
DEBUG:rustc_trans::debuginfo::source_loc: set_source_location: src/libcore/iter/iterator.rs:254:13: 254:14
DEBUG:rustc_trans::debuginfo::source_loc: setting debug location to 254 0
DEBUG:rustc_trans::mir::rvalue: trans_rvalue(dest.llval=({ i64, [0 x i8], i32, [4 x i8] }*:  %x = alloca { i64, [0 x i8], i32, [4 x i8] }), rvalue=((_11 as Some).0: <Self as iter::iterator::Iterator>::Item))
DEBUG:rustc_trans::mir::operand: trans_operand(operand=((_11 as Some).0: <Self as iter::iterator::Iterator>::Item))
DEBUG:rustc_trans::mir::operand: trans_consume(lvalue=((_11 as Some).0: <Self as iter::iterator::Iterator>::Item))
DEBUG:rustc_trans::mir::lvalue: trans_lvalue(lvalue=((_11 as Some).0: <Self as iter::iterator::Iterator>::Item))
DEBUG:rustc_trans::mir::lvalue: trans_lvalue(lvalue=(_11 as Some))
DEBUG:rustc_trans::mir::lvalue: trans_lvalue(lvalue=_11)
DEBUG:rustc_trans::monomorphize: apply_param_substs(param_substs=Slice([str::CharIndices]), value=Downcast { adt_def: option::Option, substs: Slice([(usize, char)]), variant_index: 1 })
DEBUG:rustc_trans::mir::lvalue: trans_lvalue(lvalue=(_11 as Some)) => LvalueRef { llval: 0x7f059d15cb78, llextra: 0x0, ty: Downcast { adt_def: option::Option, substs: Slice([(usize, char)]), variant_index: 1 }, alignment: AbiAligned }
DEBUG:rustc_trans::monomorphize: apply_param_substs(param_substs=Slice([str::CharIndices]), value=Ty { ty: <Self as iter::iterator::Iterator>::Item })
DEBUG:rustc_trans::adt: struct_llfields: variant: Struct { align: Align { raw: 51 }, primitive_align: Align { raw: 51 }, packed: false, sized: true, offsets: [Size { raw: 0 }, Size { raw: 8 }], memory_index: [0, 1], min_size: Size { raw: 24 } }
DEBUG:rustc_trans::adt: struct_llfields: 0 ty: u64 min_offset: 0 target_offset: 0
DEBUG:rustc_trans::adt: struct_llfields: 1 ty: (usize, char) pad_bytes: 0 min_offset: 8 target_offset: 8
DEBUG:rustc_trans::adt: struct_llfields: pad_bytes: 0 min_offset: 24 min_size: 24 stride: 24

DEBUG:rustc_trans::mir::lvalue: trans_lvalue(lvalue=((_11 as Some).0: <Self as iter::iterator::Iterator>::Item)) => LvalueRef { llval: 0x7f05a0a17f78, llextra: 0x0, ty: Ty { ty: (usize, char) }, alignment: AbiAligned }
DEBUG:rustc_trans::mir::operand: trans_load: ({ i64, [0 x i8], i32, [4 x i8] }*:  %10 = getelementptr inbounds { i64, [0 x i8], { i64, [0 x i8], i32, [4 x i8] }, [0 x i8] }, { i64, [0 x i8], { i64, [0 x i8], i32, [4 x i8] }, [0 x i8] }* %9, i32 0, i32 2, !dbg !73) @ (usize, char)

That source line is in Iterator::nth which looks innocuous enough, but it's inlined so I'm not sure what usage is causing the problem.

The crash is inside LLVMMDNodeInContext, I need to build that with debug symbols...

@eddyb
Copy link
Member

eddyb commented Mar 6, 2017

@bitshifter Oh I haven't gotten a chance to look at it, I still need to bring my own branch to a point where I can rebase yours on it to remove any suspicions of ABI confusion. But if it's debuginfo, that could be missing changes in rustc_trans::debuginfo, where it computes metadata for various types.

@bitshifter
Copy link
Contributor Author

bitshifter commented Mar 10, 2017

The pointer for min in Builder::load_range_assert looks like a garbage value (varies, was 0xff..fe one time). So I'm not sure it's debuginfo that's the problem, but getting a bad value out of LLVM somewhere. Do you have any tips for debugging LLVM, it's just a bunch of opaque pointers that I can't introspect.

@eddyb
Copy link
Member

eddyb commented Mar 10, 2017

@bitshifter Can you log (just before this) Value(ptr)?
It might not be a pointer to an integer type.

@bitshifter
Copy link
Contributor Author

bitshifter commented Mar 10, 2017

@eddyb log the LLVMTypeOf? Got to head out, will take a look when I get back.

@eddyb
Copy link
Member

eddyb commented Mar 10, 2017

@bitshifter debug!("{:?}", Value(ptr)) should show both the type and the instruction.

@bitshifter
Copy link
Contributor Author

bitshifter commented Mar 11, 2017

@eddyb Ah, that's helpful. I've pushed a fix which seems to get me past that problem. The next issue is:

DEBUG:rustc_trans::back::symbol_export: EXPORTED SYMBOL (local): _ZN4core3num7flt2dec8strategy6dragon12format_exact17h154ec7e8cc7d61b2E (Rust)
DEBUG:rustc_trans::back::write: llvm-optimizing "core.cgu-0"
Invalid operand types for ICmp instruction
  %30 = icmp ne [0 x i8] %29, zeroinitializer, !dbg !21846
Invalid operand types for ICmp instruction
  %55 = icmp ne [0 x i8] %54, zeroinitializer, !dbg !21862
LLVM ERROR: Broken function found, compilation aborted!

Don't know if the EXPORTED SYMBOL is related or just the previous phase in the log. Looks like an internal assert from LLVM, not sure how to trace the cause.

@bitshifter
Copy link
Contributor Author

bitshifter commented Mar 11, 2017

It seems that zeroinitializer is used in constant initialization, maybe that's where this is coming from...

@eddyb
Copy link
Member

eddyb commented Mar 11, 2017

Constants are cast so you wouldn't see that. Seems like more field index confusion?

@bitshifter
Copy link
Contributor Author

bitshifter commented Apr 21, 2017

@eddyb Maybe something to do with rustc_trans::adt::union_fill. I think in this instance, the else branch of the below code would be hit

    if let Some(ity) = layout::Integer::for_abi_align(cx, layout_align) {
        Type::array(&Type::from_integer(cx, ity), align_units)
    } else {
        Type::array(&Type::vector(&Type::i32(cx), align/4),
                    align_units)
    }

Perhaps the Type::vector behaves differently on this arch, or it doesn't support wide enough vectors?

The code it's failing to compile is most likely the enum test in src/tests/run-pass/align-struct:

#[repr(align(16))]
struct Align16(i32);

enum Enum {
    A(i32),
    B(Align16)
}
@eddyb
Copy link
Member

eddyb commented Apr 21, 2017

@bitshifter We should not be emitting vector types, they're useless for this 😞

@bitshifter
Copy link
Contributor Author

bitshifter commented Apr 21, 2017

@eddyb I guess this code is trying to not add extra fields for padding. I wonder what the motivation for using a vector here was. Maybe I could just remove the vector branch.

@bitshifter
Copy link
Contributor Author

bitshifter commented Apr 21, 2017

@eddyb I think I understand what this code is trying to do - to create space for the tagged enum variants using a type that has the same alignment as the max enum variant alignment. I'm not sure why this is necessary though? This code is really old, I think the vector code path was added 3 years ago in 8658a43. The layout should store the maximum variant's alignment and I think this is the alignment that gets used. That's the case for repr aligned variants anyway.

Can you see any problem with just emitting i8's for the enum contents? I'm trying it now, so far it appears to work, but perhaps there are arch differences with this approach.

This is all assuming that this is the cause of the problem on ARM of course.

@eddyb
Copy link
Member

eddyb commented Apr 21, 2017

@bitshifter It's just a belief that larger elements are better, and I think vectors were used to get 16-byte elements but I don't know more. You can only really do it for primitive_align though, not the possibly-user-specified align.

@bitshifter
Copy link
Contributor Author

bitshifter commented Apr 21, 2017

@eddyb If the alignment doesn't matter, perhaps the easiest thing to do is just remove that assert.

@eddyb
Copy link
Member

eddyb commented Apr 21, 2017

@bitshifter Hmm now that I think more about it, I believe the problem is comparing an alignment from LLVM with align instead of primitive_align. LLVM can't return anything other than primitive_align.

@bitshifter
Copy link
Contributor Author

bitshifter commented Apr 21, 2017

@eddyb that would make sense, except for some reason this assert isn't firing on x86_64. I really don't understand why not.

@eddyb
Copy link
Member

eddyb commented Apr 21, 2017

@bitshifter Just give an enum the alignment of 32 or more. x64 has 16-byte alignment SSE.

@bitshifter
Copy link
Contributor Author

bitshifter commented Apr 21, 2017

@eddyb I did try that, it still didn't assert... I'll try changing the assert to primitive align anyway and see what happens.

@eddyb
Copy link
Member

eddyb commented Apr 21, 2017

@bitshifter Sorry, I meant something like #[repr(align(64))] struct Foo(u8); and then trying to use Option<Foo>.

Hopefully will fix assert on ARM where vector types are being used as
the fill type for enums containing repr aligned types greater than the
largest possible native type, thus don't match the Layout's alignment
and triggers an assert.
@eddyb
Copy link
Member

eddyb commented Apr 21, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Apr 21, 2017

📌 Commit 946f8e6 has been approved by eddyb

@bitshifter
Copy link
Contributor Author

bitshifter commented Apr 22, 2017

@eddyb weirdly, the 64 bit aligned Option<Foo> doesn't assert. I added some print statements and LLVM is reporting the alignment as 64. I think perhaps it's because of this https://github.com/rust-lang/llvm/blob/2e951c3ae354bcbd2e50b30798e232949a926b75/lib/IR/DataLayout.cpp#L494. LLVM must let you create arbitrary vector types, I think it's not a problem because there's no code generated that tries to access the data as that type. Still not sure why this would be different on ARM though.

Anyway, I don't see any problem with the change I made, so hopefully it passes homu tests. I don't think alignment should be important here, as it should be handled by the Layout.

@eddyb
Copy link
Member

eddyb commented Apr 22, 2017

@bitshifter I am dumb, 64 bytes is AVX512. I should've said 1024 bytes or something.

@bitshifter
Copy link
Contributor Author

bitshifter commented Apr 22, 2017

@bors
Copy link
Contributor

bors commented Apr 22, 2017

Testing commit 946f8e6 with merge 6d841da...

bors added a commit that referenced this pull request Apr 22, 2017
Implementation of repr struct alignment RFC 1358.

The main changes around rustc::ty::Layout::struct:
* Added abi_align field which stores abi alignment before repr align is applied
* align field contains transitive repr alignment
* Added padding vec which stores padding required after fields

The main user of this information is rustc_trans::adt::struct_llfields
which determines the LLVM fields to be used by LLVM, including padding
fields.

A possible future optimisation would be to put the padding Vec in an Option, since it will be unused unless you are using repr align.
@bors
Copy link
Contributor

bors commented Apr 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 6d841da to master...

@bors bors merged commit 946f8e6 into rust-lang:master Apr 22, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors bors mentioned this pull request Apr 22, 2017
1 of 3 tasks complete
@Vtec234 Vtec234 mentioned this pull request Apr 22, 2017
@bitshifter
Copy link
Contributor Author

bitshifter commented Apr 22, 2017

I should have edited the PR description before it got merged, it's pretty out of date :)

There's some follow up things that should probably be done in the future:

  • write a codegen test - the test/run-pass/align-struct.rs doesn't catch everything - alloca's missing the alignment can just work depended on how they're used (e.g. address might happen to have correct alignment, only fails if usage requires alignment like vector instruction load/stores)
  • specify alignment for over aligned types to stores and loads - this is an optimisation. it's always safe to under specify alignment, it's just slower, but over specifying is undefined behaviour so care is required. I thought it would be good to see how repr(align) is being used before tackling this.

What would the process be for tracking these, create some new issues?

@eddyb
Copy link
Member

eddyb commented Apr 22, 2017

@bitshifter I'd put them in the tracking issue, if there is one for #[repr(align)].

@bitshifter bitshifter deleted the bitshifter:struct_align branch Apr 28, 2017
bors added a commit that referenced this pull request Feb 6, 2019
Allow #[repr(align(x))] on enums (#57996)

Tracking issue: #57996

Implements an extension of [RFC 1358](https://github.com/rust-lang/rfcs/blob/master/text/1358-repr-align.md) behind a feature flag (`repr_align_enum`). Originally introduced here for structs: #39999.

It seems like only HIR-level changes are required, since enums are already aware of their alignment (due to alignment of their limbs).

cc @bitshifter
bors added a commit that referenced this pull request Feb 6, 2019
Allow #[repr(align(x))] on enums (#57996)

Tracking issue: #57996

Implements an extension of [RFC 1358](https://github.com/rust-lang/rfcs/blob/master/text/1358-repr-align.md) behind a feature flag (`repr_align_enum`). Originally introduced here for structs: #39999.

It seems like only HIR-level changes are required, since enums are already aware of their alignment (due to alignment of their limbs).

cc @bitshifter
bors added a commit that referenced this pull request Feb 7, 2019
Allow #[repr(align(x))] on enums (#57996)

Tracking issue: #57996

Implements an extension of [RFC 1358](https://github.com/rust-lang/rfcs/blob/master/text/1358-repr-align.md) behind a feature flag (`repr_align_enum`). Originally introduced here for structs: #39999.

It seems like only HIR-level changes are required, since enums are already aware of their alignment (due to alignment of their limbs).

cc @bitshifter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.