Append missing padding after last field of struct #13271

stepancheg · 2014-04-03T01:25:30Z

This patch fixes issue #13186.

When generating constant expression for enum, it is possible that
alignment of expression may be not equal to alignment of type. In that
case space after last struct field must be padded to match size of value
and size of struct. This commit adds that padding.

See detailed explanation in src/test/run-pass/trans-tag-static-padding.rs

alexcrichton · 2014-04-03T01:29:43Z

cc @jld

flaper87 · 2014-04-04T17:36:14Z

src/test/run-pass/tag-align-static.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Issue #13186


could you please prefix the filename with trans and add a description of what this test does ?

By reading the test case, it's not obvious what the test is since default_instance is never called, nor the value is used, etc.

It'd be nice if you could also explain why this succeeds to compile and how this test verifies the right alignment.

Thanks

stepancheg · 2014-04-04T21:06:53Z

@flaper87 done

stepancheg · 2014-04-09T00:13:26Z

I came up with slighly different and slightly better approach. At startup compiler iterates over known integer types (i8, i16, i32, i64 and i8*) and populates map alignment -> integer_type_with_that_alignment. When aligner is needed, compiler searches that map.

See updated patch.

flaper87 · 2014-04-09T13:39:11Z

src/librustc/middle/trans/context.rs

+                ];
+                for &aligner_base in aligner_bases.iter() {
+                    let align = machine::llalign_of_min(&ccx, aligner_base);
+                    ccx.aligner_by_align.find_or_insert_with(align, |_| {


Unless I'm missing something, you could just do ccx.aligner_by_align.find_or_insert(align, Type::array(&aligner_base, 0)) (or even just insert)?

@flaper87 both insert and find_or_insert compute second parameter which may be unused. For instance, both i8p and i64 might use same alignment, so only one of them can be inserted into map.

It looks like microoptimization, but I did it to emphasizes that not all aligner_bases are used in map.

I think using find_or_insert is fine, TBH. Like you said, current code is a micro-optimization. I'm fine either way but I don find find_or_insert more readable in this case and this micro-optimization is probably not needed here.

@flaper87 OK, changed to find_or_insert. Any change pull requested will be approved or rejected?

flaper87 · 2014-04-09T15:24:08Z

@alexcrichton @jld any final comments here?

alexcrichton · 2014-04-10T14:34:35Z

I am not familiar enough with the layout of enums to r+ this. It makes sense to me, but there's clearly a lot of subtle things going on here.

jld · 2014-04-11T00:24:12Z

I think there's a simpler solution: trans::adt::build_const_struct inserts padding to deal with constants with insufficient alignment (which is currently General enums, but if/when we add user-specified alignment, this will be a bigger problem and won't be limited to alignments for which types exist).

The problem, if I understand correctly, is that it's not fixing the padding after the last element, so the sizes don't match, which is why we get the assertion seen in #13186.

(Also, I don't know offhand if we're passing the alignment to LLVM when creating the global, but that seems to not be the problem here.)

stepancheg · 2014-04-11T00:46:44Z

@jld thanks, I'll do it (patch build_const_struct to append padding after last element).

stepancheg · 2014-04-11T17:03:50Z

Updated the patch with padding after last field instead of aligner magic.

flaper87 · 2014-04-11T18:35:52Z

@stepancheg Travis' failure seems legit. Mind taking a look?

stepancheg · 2014-04-11T19:08:58Z

@flaper87 yes, thanks.

stepancheg · 2014-04-11T19:18:16Z

@flaper87 funny thing is that this commit revealed another bug: #[packed] static constants don't work properly. Here's simple test case:

enum Foo {
    Bar = 1,
    Baz = 2
}

#[packed]
struct S3_Foo {
    a: u8,
    b: u16,
    c: Foo
}

static TEST_S3_Foo: S3_Foo = S3_Foo { a: 1, b: 2, c: Baz };

pub fn main() {
    println!("{:?}", TEST_S3_Foo);
}

(when compiler with rust/master, it crashes at runtime).

I think I have to fix it too.

Sorry, I didn't run make check myself.

flaper87 · 2014-04-11T20:36:57Z

@stepancheg no worries. You know you can run tests just for stage1 rpass, right? You can do that with: make check-stage1-rpass

I'm not a real expert in this part of the code but it would be amazing if you can fix that as part of this PR too.

huonw · 2014-04-12T00:08:29Z

I thought that was fixed by #9832 ? (Or are you saying that you caused it?)

stepancheg · 2014-04-12T00:12:13Z

@huonw I'm was wrong: packed static structs seems to work fine. Problem is that std::repr is broken for packed structs (not necessary static).

Edit: reported bug here: #13486.

This patch fixes issue rust-lang#13186. When generating constant expression for enum, it is possible that alignment of expression may be not equal to alignment of type. In that case space after last struct field must be padded to match size of value and size of struct. This commit adds that padding. See detailed explanation in src/test/run-pass/trans-tag-static-padding.rs

stepancheg · 2014-04-12T21:55:58Z

Fixed a patch.

Previously, build_const_struct function computed offsets incorrectly for packed structs. It did not affect generated code, but it messed up with my previous version of patch.

I checked it with make check-stage1-rpass.

flaper87 · 2014-04-13T10:49:46Z

src/librustc/middle/trans/adt.rs

+        let target_offset = *target_offsets.get(i);
+        if !st.packed {
+            let val_align = machine::llalign_of_min(ccx, val_ty(val))
+                /*bad*/as u64;


mind putting this in a single line and explain in a comment why this is bad for future readers? Also, it'd be nice to have a FIXME with an issue filed if there's no other way to do this right now.

@flaper87 I have no idea, why it is bad. It was here before me. I guess it is a joke.

I think it is because using u64 is not necessarily correct on all target platforms. (The thing to do would be check the git history.)

flaper87 · 2014-04-13T11:04:27Z

@stepancheg Thanks a lot for working on this. Great job. I left some minor comments there.

Also, it'd be nice to have a separate test for the latest fix.

Thanks again!

stepancheg · 2014-04-13T12:30:23Z

@flaper87 there's nothing to test: offsets were computed incorrectly, but computed offsets did not affect result of build_const_struct (before offset was used to compute size of last padding).

stepancheg · 2014-04-13T13:14:22Z

Updated the patch, minor changes requested by @flaper87.

huonw · 2014-05-02T06:35:40Z

Ping, is this waiting on anything other than a re-review?

This patch fixes issue #13186. When generating constant expression for enum, it is possible that alignment of expression may be not equal to alignment of type. In that case space after last struct field must be padded to match size of value and size of struct. This commit adds that padding. See detailed explanation in src/test/run-pass/trans-tag-static-padding.rs

flaper87 reviewed Apr 4, 2014
View reviewed changes

flaper87 reviewed Apr 9, 2014
View reviewed changes

stepancheg changed the title ~~Make constant of enum type properly aligned~~ Append missing padding after last field of struct Apr 11, 2014

flaper87 reviewed Apr 13, 2014
View reviewed changes

bors closed this May 5, 2014

stepancheg deleted the align branch May 5, 2014 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append missing padding after last field of struct #13271

Append missing padding after last field of struct #13271

stepancheg commented Apr 3, 2014

alexcrichton commented Apr 3, 2014

flaper87 Apr 4, 2014

flaper87 Apr 4, 2014

stepancheg commented Apr 4, 2014

stepancheg commented Apr 9, 2014

flaper87 Apr 9, 2014

stepancheg Apr 9, 2014

flaper87 Apr 9, 2014

stepancheg Apr 9, 2014

flaper87 commented Apr 9, 2014

alexcrichton commented Apr 10, 2014

jld commented Apr 11, 2014

stepancheg commented Apr 11, 2014

stepancheg commented Apr 11, 2014

flaper87 commented Apr 11, 2014

stepancheg commented Apr 11, 2014

stepancheg commented Apr 11, 2014

flaper87 commented Apr 11, 2014

huonw commented Apr 12, 2014

stepancheg commented Apr 12, 2014

stepancheg commented Apr 12, 2014

flaper87 Apr 13, 2014

stepancheg Apr 13, 2014

huonw Apr 13, 2014

flaper87 commented Apr 13, 2014

stepancheg commented Apr 13, 2014

stepancheg commented Apr 13, 2014

huonw commented May 2, 2014

Append missing padding after last field of struct #13271

Append missing padding after last field of struct #13271

Conversation

stepancheg commented Apr 3, 2014

alexcrichton commented Apr 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stepancheg commented Apr 4, 2014

stepancheg commented Apr 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaper87 commented Apr 9, 2014

alexcrichton commented Apr 10, 2014

jld commented Apr 11, 2014

stepancheg commented Apr 11, 2014

stepancheg commented Apr 11, 2014

flaper87 commented Apr 11, 2014

stepancheg commented Apr 11, 2014

stepancheg commented Apr 11, 2014

flaper87 commented Apr 11, 2014

huonw commented Apr 12, 2014

stepancheg commented Apr 12, 2014

stepancheg commented Apr 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaper87 commented Apr 13, 2014

stepancheg commented Apr 13, 2014

stepancheg commented Apr 13, 2014

huonw commented May 2, 2014