memorylayouts padding, jextract C_BOOL

[Maurizio Cimadamore]

For the records - filed:

https://bugs.openjdk.java.net/browse/JDK-8238325

https://bugs.openjdk.java.net/browse/JDK-8238320

https://bugs.openjdk.java.net/browse/JDK-8238316

Thanks
Maurizio

On 31/01/2020 15:48, Maurizio Cimadamore wrote:
> This is odd -I'd expect a C_BYTE, not a C_BOOL. There must be a bug 
> somewhere - I hope not in libclang reporting the wrong type - we'll 
> look into it.
>
> The alignment issues you report are also odd, and I agree that having 
> padding alignment be 8 bits always would be a better solution, 
> although I'm not sure that's the only issue you are seeing there.
>
> Thanks for the reports - some comments inline
>
> Maurizio
>
> On 31/01/2020 08:08, Michael Zucchi wrote:
>>
>> Hi guys,
>>
>> I just noticed jextract turns a 'char' into a C_BOOL.  I suppose it 
>> doesn't make a difference but it just looks wrong.  _Bool exists in 
>> C99+ but an 8 bit int is more likely to be a int8_t or char than 
>> _Bool (imo).  I think the whole idea of 'c named types' just isn't 
>> adding any value and they should all be basically the "stdint.h" 
>> types with the platform byte ordering, but it's no big deal.  The 
>> scoped allocator sort of muddies it further by using java types, but 
>> at least they have defined sizes which map to fixed stdint types.
>>
>> Also, this:
>>
>> struct foo {
>>     char x;
>>     int v __attribute__ ((aligned(8)));
>> };
>>
>> generates:
>>
>>     public static final MemoryLayout foo$LAYOUT = MemoryLayout.ofStruct(
>>         MemoryLayouts.SysV.C_BOOL.withName("x"),
>>         MemoryLayout.ofPaddingBits(56),
>>         MemoryLayouts.SysV.C_INT.withName("v"),
>>         MemoryLayout.ofPaddingBits(32)
>>     ).withName("foo");
> This seems already wrong - that is, the alignment constraint is not 
> reflected in the layout (you can specify custom alignment on a layout 
> using the MemoryLayout::withBitAlignment method) but that doesn't seem 
> to happen here.
>>
>> which breaks at this:
>>
>>     MemorySegment seg = MemorySegment.allocateNative(foo);
>>
>> Exception in thread "main" java.lang.IllegalArgumentException: 
>> Invalid alignment constraint : 7
>>         at 
>> jdk.incubator.foreign/jdk.incubator.foreign.MemorySegment.allocateNative(MemorySegment.java:422)
>>         at 
>> jdk.incubator.foreign/jdk.incubator.foreign.MemorySegment.allocateNative(MemorySegment.java:360)
>>         at notzed...
>>
>> This is because MemoryLayou8ts.ofPaddingBits(X) creates a padding of 
>> "X" bits with "X" bit alignment, which is just not likely to work in 
>> the general case.
>
> The exception occurs because the byte size is 16, while the byte 
> alignment (the max alignment in the struct) is 7. So, alignment is not 
> a power of two. As you say, the issue here is the fact that the 
> inferred layout for padding is wrong and interferes with the alignment 
> computation for the overall layout. Two solutions:
>
> - force align padding to 8 always
> - skip padding layouts when computing compound layout alignments
>
>
>> I hit this trying to use padding to skip private/unimportant fields 
>> (by using large bit paddings) and then recreated the problem using 
>> jextract.
>>
>> Given it's specifically padding which almost by definition isn't 
>> aligned, maybe padding alignment bits should always just be 8? Or 
>> just provide the alignment argument in MemoryLayout.ofPaddingBits() 
>> so you don't have to call.withBitAligment(8) every time (this 
>> currently works).
> Yeah - this seems good; in fact I'm thinking that perhaps padding bits 
> should always come with 8-bit alignment, regardless of the size, so as 
> not to interfere with the computations we do.
>>
>> From .withBitAlignment() javadoc it seems the alignment has to be >=8 
>> and 2^n so that means the MemoryLayouts.ofPaddingBits() 
>> implementation is incorrect in passing X as the alignment to new 
>> PaddingLayout().
>>
>>     /**
>>      * Creates a new layout which features the desired alignment 
>> constraint.
>>      *
>>      * @param bitAlignment the layout alignment constraint, expressed 
>> in bits.
>>      * @return a new layout which is the same as this layout, except 
>> for the alignment constraint associated to it.
>>      * @throws IllegalArgumentException if {@code bitAlignment} is 
>> not a power of two, or if it's less than than 8.
>>      */
>>     MemoryLayout withBitAlignment(long bitAlignment);
>>
>> But other functions say it can be < 8 bits so it's a little messy 
>> here.  If it's intended for describing bitfields well it doesn't 
>> really fit the api constraints, and for bitfields you'd want a bit 
>> alignment of 1 by definition.
> I agree docs should be cleaned up here. In general, I think >= 8 
> should be enforced everywhere. Bitfields are implemented by doing a 
> get/set of a bigger word and then doing bitmask manipulation on it - 
> while originally we had some desire to capture this bitfield layout in 
> the layout API itself, I'm not 100% sure it's the right way to go - 
> this was an issue that was put on the sideline as we were working on 
> the memory access API, and I think we should re-assess and make sure 
> the API specifies constraints that are consistent in spirit.
>>
>> Also note that this however /does/ work:
>>
>> struct foo {
>>     char x;
>>     long v;
>> };
>>
>>     public static final MemoryLayout foo$LAYOUT = MemoryLayout.ofStruct(
>>         MemoryLayouts.SysV.C_BOOL.withName("x"),
>>         MemoryLayout.ofPaddingBits(56),
>>         MemoryLayouts.SysV.C_LONG.withName("v"),
>>     ).withName("foo");
>>
>> Which doesn't make any sense if the padding alignment really was 56??
>
> That's because the alignment of the struct is inferred (where not 
> provided) to be the MAX alignment of all the components. You have a 
> LONG here which means 64 beats 56. So the struct is 8-byte aligned, 
> which is fine.
>
> Maurizio
>
>
>>
>> Cheers,
>>  Z
>>
>>
>>
>>