[foreign-memaccess] RFR 8223978: Add alignment support to layouts

[Maurizio Cimadamore]

Here's new new webrev:

http://cr.openjdk.java.net/~mcimadamore/panama/8223978_v2/

I've changed implementation approach, and now added an OptionalLong to 
the AbstractLayout class. This way we can do almost everything in the 
superclass, and we can just rely on the already existing 'dup' feature 
(which we have to expand a bit) in order to produce a new layout with 
desired alignment.

This is more regular, and automatcally handles thing such as covariant 
overrides.

Maurizio

On 16/05/2019 12:00, Maurizio Cimadamore wrote:
>
> On 16/05/2019 11:57, Jorn Vernee wrote:
>>> I think you can still model that use case - essentially what you want
>>> is to just align everything to 8. (in fact that's what pragma pack(1)
>>> does).
>>
>> Ah, yes of course! My mistake.
>>
>> ---
>>
>> Btw, while testing this I also noticed that aligning e.g. a Group 
>> with alignTo() will return a Layout. I think we should use covariant 
>> return types here (and in the other Layout sub-types). Also, Address 
>> is missing on override for alignTo, so this will just return a Value.
>
> Whoops - good point.
>
> I also added a test with your 'pragma pack(1)' example. I'll spin a 
> new webrev shortly.
>
> Thanks
> Maurizio
>
>>
>> Thanks,
>> Jorn
>>
>> [1] : https://docs.microsoft.com/en-us/cpp/cpp/align-cpp?view=vs-2019
>>
>> Maurizio Cimadamore schreef op 2019-05-16 12:25:
>>> On 15/05/2019 21:07, Jorn Vernee wrote:
>>>>> * you can align a layout using Layout::alignTo(long) - this sets the
>>>>> additional constraint (provided is power of two, non-negative, and
>>>>> >=8)
>>>>
>>>> Why the power of two constraint? Shouldn't it just be a multiple of 8?
>>>>
>>>> This seems to preclude some packed structs, e.g. with elements with 
>>>> 3 byte alignment:
>>>>
>>>>     #pragma pack(1)
>>>>
>>>>     struct Foo {
>>>>         char x;
>>>>         short y; // 1 byte alignment
>>>>         int z; // 3 byte alignment
>>>>     }; // size = 7 bytes
>>>>
>>>> In the layout API this is:
>>>>
>>>>     Value vChar = Value.ofSignedInt(8);
>>>>     Value vShort = Value.ofSignedInt(16);
>>>>     Value vInt = Value.ofSignedInt(32);
>>>>     Group g = Group.struct(vChar, vShort.alignTo(8), 
>>>> vInt.alignTo(24));
>>>>
>>>> But this throws an IAE because alignment of 24 is not allowed.
>>>
>>> I think you can still model that use case - essentially what you want
>>> is to just align everything to 8. (in fact that's what pragma pack(1)
>>> does).
>>>
>>> I don't think there's such a thing as 3-byte aligned memory access: if
>>> you want to read 4 bytes on a 3-byte aligned address, that's just
>>> unaligned read.
>>>
>>> In other words, nothing new here - we're kind of following what the
>>> pragma pack allows you to do: the argument to pragma pack must also be
>>> a power of 2 (1, 2, 4, 8 ...), see here [1, 2] - the only difference
>>> in the Panama API is that everything has to be multiplied by 8,
>>> because alignment (as sizes) are expressed in bits, to have more room
>>> to add sub-byte alignment (bit-fields, etc.) later on.
>>>
>>> So, back to your example:
>>>
>>> Value vChar = Value.ofSignedInt(8);
>>> Value vShort = Value.ofSignedInt(16);
>>> Value vInt = Value.ofSignedInt(32);
>>> Group g = Group.struct(vChar.alignTo(8), vShort.alignTo(8), 
>>> vInt.alignTo(8));
>>>
>>> Maurizio
>>>
>>> [1] - 
>>> https://docs.microsoft.com/en-us/cpp/preprocessor/pack?view=vs-2019
>>> [2] -
>>> https://gcc.gnu.org/onlinedocs/gcc-4.4.4/gcc/Structure_002dPacking-Pragmas.html 
>>>
>>>
>>>
>>>>
>>>> Jorn
>>>>
>>>> Maurizio Cimadamore schreef op 2019-05-15 19:57:
>>>>> Hi,
>>>>> this patch adds support to the API to express alignment 
>>>>> constraints on
>>>>> layouts. Following the model John put forward in [1], I did the
>>>>> following:
>>>>>
>>>>> * there's a way to compute the 'natural alignment' of a layout
>>>>>
>>>>> * if no alignment is specified, alignment is the natural alignment
>>>>>
>>>>> * you can align a layout using Layout::alignTo(long) - this sets the
>>>>> additional constraint (provided is power of two, non-negative, and
>>>>> >=8)
>>>>>
>>>>> * When we obtain a VarHandle out of a Layout, at that point we are in
>>>>> the process of computing offsets; here we can catch e.g. if the 
>>>>> offset
>>>>> of a given path doesn't match the specified alignment - in this case
>>>>> we fail fast, even before memory is accessed; this occurs e.g. if the
>>>>> layout path offset is not a multiple of its alignment, or if the
>>>>> alignment of the nested element is stricter than the one of the
>>>>> enclosing element.
>>>>>
>>>>> (in principle we could enforce these checks on layout creation, but
>>>>> given we have unresolved layouts in our radar, I think it's best 
>>>>> to do
>>>>> minimal checks on layout creation and let the check fully kick in on
>>>>> path creation).
>>>>>
>>>>> * If VarHandle can be constructed, a dynamic check verifies that
>>>>> alignment of address passed to VH matches the layout requirements
>>>>>
>>>>> * MemoryScope::allocate also honors the alignment requirements - this
>>>>> works by up-allocating memory (to make sure a pointer with desired
>>>>> alignment exists in the allocated area) and then adjusting it after
>>>>> the fact. For alignments < 16bytes, nothing is done given that 
>>>>> malloc,
>>>>> at least on x64 is guaranteed to respect that.
>>>>>
>>>>>
>>>>> I think this is powerful and yet relatively simple to understand,
>>>>> overall I quite like it.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8223978/
>>>>>
>>>>> Maurizio