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

Fix ICE on statics with fancy nullable enums. #12703

Closed
wants to merge 1 commit into from

Conversation

jld
Copy link
Contributor

@jld jld commented Mar 5, 2014

Closes #8506.

@alexcrichton
Copy link
Member

Could you add Closes #XXXX to the commit message as well? I'm not very familiar with this area, so it'd be nice with some more explanation of the bug in the commit message as well, but it doesn't really need to block this.

@flaper87
Copy link
Contributor

flaper87 commented Mar 5, 2014

I'm not familiar with this part of the code but based on your comment here and the commit, it seems to make sense.

I agree with @alexcrichton, it would be nice to have a better description in the commit message too.

Closes rust-lang#8506.

The `trans::adt` code for statics uses fields with `C_undef` values to
insert alignment padding (because LLVM's own alignment padding isn't
always sufficient for aggregate constants), and assumes that all fields
in the actual Rust value are represented by non-undef LLVM values, to
distinguish them from that padding.

But for nullable pointer enums, if non-null variant has fields other
than the pointer used as the discriminant, they would be set to undef in
the null case, to reflect that they're never accessed.

To avoid the obvious conflict between these two items, the latter undefs
were wrapped in unary LLVM structs to distinguish them from the former
undefs.  Except this doesn't actually work -- LLVM, not unreasonably,
treats the "wrapped undef" as a regular undef.

So this commit just sets all fields to null in the null pointer case of
a nullable pointer enum static, because the other fields don't really
need to be undef in the first place.
@jld
Copy link
Contributor Author

jld commented Mar 6, 2014

Re-pushed, with more words.

bors added a commit that referenced this pull request Mar 6, 2014
@bors bors closed this Mar 6, 2014
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.

assertion failed: !is_undef(wrapped)
4 participants