Skip to content

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented Oct 8, 2015

For enum variants, the default alignment for a specific variant might be
lower than the alignment of the enum type itself. In such cases we, for
example, generate memcpy calls with an alignment that's higher than the
alignment of the constant we copy from.

To avoid that, we need to explicitly set the required alignment on
constants.

Fixes #28912.

@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignment mishap?

@eddyb
Copy link
Member

eddyb commented Oct 8, 2015

r=me with the formatting nit fixed.

@dotdash
Copy link
Contributor Author

dotdash commented Oct 8, 2015

@bors r=eddyb 00a79c0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this belongs inside addr_of_mut, rather than here.

@dotdash
Copy link
Contributor Author

dotdash commented Oct 8, 2015

@bors r-

@eefriedman Good point.

@dotdash
Copy link
Contributor Author

dotdash commented Oct 9, 2015

Update as per the suggestion of @eefriedman. I also had to fix the string that identifies the gdb script section, which is used as a C string but had no terminating 0 byte. The alignment changes caused that bug to trigger an error in a codegen test.

@alexcrichton
Copy link
Member

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned alexcrichton Oct 9, 2015
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity, you might want to use c_section_var_name.as_ptr() as *const _ below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For correctness even. Not sure where I lost that change (and even more confusing that it passed the test without it). Thanks!

For enum variants, the default alignment for a specific variant might be
lower than the alignment of the enum type itself. In such cases we, for
example, generate memcpy calls with an alignment that's higher than the
alignment of the constant we copy from.

To avoid that, we need to explicitly set the required alignment on
constants.

Fixes rust-lang#28912.
@dotdash
Copy link
Contributor Author

dotdash commented Oct 9, 2015

Updated to handle upgrading the alignment of constants that are shared between types with different alignment requirements.

Thanks a lot @eefriedman!

@eddyb
Copy link
Member

eddyb commented Oct 10, 2015

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 10, 2015

📌 Commit 6ad079e has been approved by eddyb

@bors
Copy link
Collaborator

bors commented Oct 10, 2015

⌛ Testing commit 6ad079e with merge 39376de...

bors added a commit that referenced this pull request Oct 10, 2015
For enum variants, the default alignment for a specific variant might be
lower than the alignment of the enum type itself. In such cases we, for
example, generate memcpy calls with an alignment that's higher than the
alignment of the constant we copy from.

To avoid that, we need to explicitly set the required alignment on
constants.

Fixes #28912.
@bors bors merged commit 6ad079e into rust-lang:master Oct 10, 2015
@bors bors mentioned this pull request Oct 10, 2015
@dotdash dotdash deleted the const_align branch January 31, 2016 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants