ir: Fix a bunch of bitfield correctness issues. #736
Conversation
|
|
|
r? @fitzgen |
|
(Probably needs some test updates for libclang <4.0) |
Or not, yay! |
| @@ -490,49 +490,67 @@ fn bitfields_to_allocation_units<E, I>(ctx: &BindgenContext, | |||
| let mut unit_align = 0; | |||
| let mut bitfields_in_unit = vec![]; | |||
|
|
|||
| // TODO(emilio): Determine this from attributes or pragma ms_struct | |||
| // directives. Also, perhaps we should check if the target is MSVC? | |||
There was a problem hiding this comment.
I think checking the target makes most sense.
There was a problem hiding this comment.
In fact, we could do this right now, right? Since the mangling PR, we have access to the target triple, so we should probably jsut check if "msvc" is in it or something like that here.
There was a problem hiding this comment.
We could, but I'd like to check with @upsuper in order to confirm that integration tests work fine for him with that bit.
tests/headers/bitfield_align.h
Outdated
| FOUR | ||
| }; | ||
|
|
||
| struct TaggedPtr { |
There was a problem hiding this comment.
Would really prefer to put the new definitions in a new test, because it is much easier to debug failing tests when they are small and isolated.
bitfield_align_2.h? issue_726.h?
There was a problem hiding this comment.
I'd argue that it's somewhat easy to isolate once these fail, but sure :)
|
|
||
| // Now we're working on a fresh bitfield allocation unit, so reset | ||
| // the current unit size and alignment. | ||
| #[allow(unused_assignments)] |
There was a problem hiding this comment.
Why are these assignments considered unused? Because we are always is_ms_struct == false?
There was a problem hiding this comment.
only the unit_size_in_bits is unused, because we re-assign it in terms of offset. Still, I think it's saner to have all the assignments here.
In particular, the "flush the allocation unit" logic is only valid for ms_structs (that is, MSVC). It's slightly annoying to have this different behavior, but it'd work just fine if we'd turn that on for MSVC. This patch doesn't do that, yet at least, and adds tests for all the weird bitfield alignments around. Fixes rust-lang#726 (and another set of hidden issues by the old code).
|
@bors-servo r=fitzgen |
|
📌 Commit 10106aa has been approved by |
ir: Fix a bunch of bitfield correctness issues. In particular, the "flush the allocation unit" logic is only valid for ms_structs (that is, MSVC). It's slightly annoying to have this different behavior, but it'd work just fine if we'd turn that on for MSVC. This patch doesn't do that, yet at least, and adds tests for all the weird bitfield alignments around. Fixes #726 (and another set of hidden issues by the old code).
|
☀️ Test successful - status-travis |
In particular, the "flush the allocation unit" logic is only valid for
ms_structs (that is, MSVC).
It's slightly annoying to have this different behavior, but it'd work just fine
if we'd turn that on for MSVC.
This patch doesn't do that, yet at least, and adds tests for all the weird
bitfield alignments around.
Fixes #726 (and another set of hidden issues by the old code).