[foreign] RFR 8217784: Ignore anonymous bitfields so they are handled as padding

[Jorn Vernee]

I think it's okay. The anonymous bitfield is still handled in the sense 
that libClang will use it to compute the offset for the next named 
field. We just derive the padding from that offset instead of trying to 
compute it ourselves from the anonymous bitfield. Handling the anonymous 
bitfield manually would at first glance also seem to make the code a bit 
more complex, since we are breaking the assumption that we can get the 
offset of any field we are processing. Ignoring the anonymous bitfield 
just kind of works out nicely.

The change in UnionLayoutComputer looks kind of magic. There might be 
other reasons why the computed size of a union is smaller than the size 
reported by libClang. An anonymous bitfield is the only reason I could 
think of, so that's why I wrote that specific comment there. The change 
in implementation is because the previous implementation was incorrect 
AFACT; if the computed size is too small, we need a padding member with 
the expected size of the union, since all fields of a union overlap.

Jorn

Maurizio Cimadamore schreef op 2019-01-25 22:24:
> The patch is functionally good, I just wonder if it's too magic - e.g.
> wouldn't be better if the record computer processed the anonymous
> bitfield (so that the correct padding can be added) instead of relying
> on the right thing to happen (which will be fragile if e.g. the impl
> changes a bit) ?
> 
> Maurizio
> 
> On 25/01/2019 19:18, Jorn Vernee wrote:
>> Good catch. I kinda glossed over unions since I thought there would be 
>> no point in having anonymous bitfields in a union, since there is no 
>> need for padding - but on second thought, these could be used to 
>> manually widen the type. Any ways, the compiler seems to allow 
>> anonymous bitfields in unions, so it's good to support that as well.
>> 
>> Updated webrev: 
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217784/webrev.01/
>> 
>> Jorn
>> 
>> Maurizio Cimadamore schreef op 2019-01-25 19:25:
>>> Looks good - but what happens with unions? I'm worried we'd get a
>>> similar failure there...
>>> 
>>> Maurizio
>>> 
>>> On 25/01/2019 13:52, Jorn Vernee wrote:
>>>> Hi,
>>>> 
>>>> From the bug description:
>>>> 
>>>> Jextract fails on the following example:
>>>> 
>>>> ```
>>>> struct Foo {
>>>>     unsigned int x: 1;
>>>>     int :7;
>>>>     unsigned int y: 8;
>>>>     int :16;
>>>>     int z;
>>>> };
>>>> ```
>>>> 
>>>> The current code tries to look up the second field, but because it 
>>>> has no name this throws an illegal field name error.
>>>> 
>>>> ---
>>>> 
>>>> I've fixed this by ignoring anonymous bitfields, since the 
>>>> StructLayoutComputer inserts padding automatically before the next 
>>>> field.
>>>> 
>>>> Please review the following.
>>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217784
>>>> Webrev: 
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217784/webrev.00/
>>>> 
>>>> Thanks,
>>>> Jorn