[foreign-abi] RFR 8236004: Memory access var handles should support MemoryAddress carrier

[Maurizio Cimadamore]

And, another iteration

http://cr.openjdk.java.net/~mcimadamore/panama/8236004_v5

I've found two bugs:

* since we erased var handle carrier pretty early, that means that we 
have no way to recover the unerased carrier from the var handle later on 
(which is needed when combining a memory access handle)

* when the var handle converts from long to MemoryAddress, if the long 
value is zero, the singleton NULL instance should be returned

I've beefed up the test to check for both conditions.

Maurizio

On 18/12/2019 14:18, Maurizio Cimadamore wrote:
> New patch (rebased):
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8236004_v4
>
> I've removed some of the smart logic for detecting pointer size in 
> Utils::POINTER_SIZE. The reason being that, currently, the VarHandle 
> assumes that addresses are fetches as longs (e.g. Unsafe::getLong) 
> anyway, so there's no way the current generated var handle will work 
> on, say, 32-bit pointers. If we want to add support for smaller 
> addresses, we will need to make the code generation scheme more 
> nuanced. I think what we have is good for now though (as all the ABIs 
> we support ATM feature 64-bit pointers)
>
> Maurizio
>
>
>
> On 18/12/2019 13:42, Maurizio Cimadamore wrote:
>>
>> On 18/12/2019 12:42, Jorn Vernee wrote:
>>> Hi,
>>>
>>> Some comments:
>>>
>>> AddressVarHandleGenerator.java:
>>> - `erase` method seems to be indented too far.
>> That was kind of deliberate, to suggest that it was only used in the 
>> constructor (we do this in other parts of the JDK).
>>>
>>> X-VarHandleMemoryAddressView.java.template:
>>> - In the long2addr method I'd prefer using invokeExact instead of 
>>> invoke, to make sure the call always gets inlined.
>>>
>>> LayoutPath.java:
>>> - In dereferenceHandle, can the carrier check here be made to use 
>>> Utils as well?
>> Will fix these
>>>
>>> Rest looks good. (Casing over the ABI impls in Utils to get the 
>>> POINTER_SIZE seems pretty unfortunate, but that was already on the 
>>> todo list I believe).
>>>
>>> Also, the patch does not apply cleanly (probably due to the manual 
>>> merge with foreign-memaccess I just did, sorry), so you might want 
>>> to rebase.
>>
>> Yeah - I'm doing a rebase; after that I'll submit another iteration
>>
>> Cheers
>> Maurizio
>>
>>>
>>> cheers,
>>> Jorn
>>>
>>> On 18/12/2019 12:03, Maurizio Cimadamore wrote:
>>>> Here's a new revision which restores erased carrier and access types:
>>>>
>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8236004_v3/
>>>>
>>>> Thanks for taking the time of retracing the steps and explaining 
>>>> how linking is supposed to work; I feel I now have a much better 
>>>> understanding of how things are connected.
>>>>
>>>> Maurizio
>>>>
>>>> On 17/12/2019 19:01, Paul Sandoz wrote:
>>>>>
>>>>>
>>>>>> On Dec 17, 2019, at 10:47 AM, Maurizio Cimadamore 
>>>>>> <maurizio.cimadamore at oracle.com 
>>>>>> <mailto:maurizio.cimadamore at oracle.com>> wrote:
>>>>>>>
>>>>>>> Yes, it works fine, until you pass an instance of something that 
>>>>>>> is not of MemoryAddressProxy e.g. pass in an instance of String, 
>>>>>>> which will get spoofed as an instance of MemoryAddressProxy. 
>>>>>>>  The guards will only check if the object is a reference type.
>>>>>>
>>>>>> Ok, I see what you mean, the problem is not that linking occurs 
>>>>>> abnormally, the problem is that the underlying linkage is not 
>>>>>> sound (in the absence of Java casts)
>>>>>>
>>>>> Yes, the linkage + invocation runtime checks are not sound.
>>>>>
>>>>>> I guess this means that same treatment should also be applied to 
>>>>>> the MemoryAddressProxy parameter (e.g. of a VarHandle::set 
>>>>>> operation) right?
>>>>>>
>>>>>
>>>>> Yes, for all access modes where the arg of $type$ is 
>>>>> of MemoryAddressProxy a cast check is required (which should 
>>>>> optimize away).
>>>>>
>>>>> I think that can be performed by the addr2long method?
>>>>>>
>>>>> Paul.