memorylayouts padding, jextract C_BOOL

[Maurizio Cimadamore]

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
>
>
>
>