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

[Jorn Vernee]

Looks good!

Jorn

Maurizio Cimadamore schreef op 2019-05-16 13:37:
> 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