[foreign] RFR 8212987 : Binder should allows negative values for native pointers

[Maurizio Cimadamore]

Looks like a solid piece of work. I like the suggestion proposed in this 
thread, as well as their implementation.

One minor quibble: the PointerTest has a spurious import:

import java.beans.Transient

Maurizio

On 30/10/2018 16:49, Jorn Vernee wrote:
> Updated webrev: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.03/
> (beware that the BoundedMemoryRegion diff turned out a bit confusing)
>
> - Changed BoundeMemoryRegion to use factory methods.
> - Factories that don't take a length calculate a max length that can 
> not overflow (in the unsigned sense).
> - Moved BoundedMemoryRegion anonymous class from Pointer to 
> BoundedMemoryRegion, since constructor is now private.
> - Added one more test to check for the error in the case where the 
> automatic length calculated for a native pointer is shorter than the 
> length required by the carrier type.
> - Switched PointerTest to TestNG, since that makes it easier to test 
> for exceptions.
>
> I've added a check for the overflow to BoundedMemoryRegion, but when 
> trying to test for it it turned out that BoundedPointer was already 
> catching that case. I've left the check there as it will still be 
> useful to catch programming errors when an explicit length is being 
> passed.
>
> Trying to treat a signed long as unsigned is somewhat confusing, maybe 
> we should add an UnsignedLong utility class that abstracts this away 
> in a later RFR, especially if we start allowing 'unsigned' longs in 
> more places.
>
> Jorn
>
> Jorn Vernee schreef op 2018-10-30 11:26:
>> You're right about `toUnsignedString`. For some reason I thought
>> `toHexString` had the `-` as a prefix if the argument is negative.
>>
>> About the length constraint; My initial thought was just to trust the
>> pointer returned from native, but I guess we should do better. In the
>> specific case I'm testing, void *, the length that is being used is
>> Long.MAX_VALUE, basically the length is unknown, so I don't think I
>> can do a range check on min + length in the constructor in that case.
>>
>> As far as I can see, the `min` value is only used in a few min +
>> offset calculations before passing to Unsafe put/get routines, I think
>> we'd want to allow overflow to negative there, but not overflow to
>> positive, basically treating the long as if it was unsigned.
>>
>> I think I will switch BoundedMemoryRegion to having factory methods
>> that do the validation, and then have ones where the length is known,
>> which do a range check, and ones where the length is unknown, that cap
>> the it to where it can not overflow. Then I can use the latter for
>> void *. If a bad pointer is passed from native, the known-length case
>> should fail on construction of the memory region, and the
>> unknown-length case should fail when dereferencing (since there is
>> already a bounds check being done).
>>
>> What do you think?
>>
>> Thanks,
>> Jorn
>>
>> Henry Jen schreef op 2018-10-30 01:59:
>>> BTW, the change in toString is not needed, as toUnsignedString(l, 16)
>>> is same as toHexString(l)?
>>>
>>> Cheers,
>>> Henry
>>>
>>>
>>>> On Oct 29, 2018, at 5:56 PM, Henry Jen <henry.jen at oracle.com> wrote:
>>>>
>>>> I am afraid this is not that simple, we need to at least protect 
>>>> length from overflow the address to >= 0.
>>>>
>>>> I haven’t completely check the implementation, we need to make sure 
>>>> all other operation will not expand or shift the region, which I 
>>>> believe is the constraint of the design. If that’s indeed the case, 
>>>> guard at construction is probably good enough.
>>>>
>>>> Cheers,
>>>> Henry
>>>>
>>>>> On Oct 29, 2018, at 10:04 AM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> Please review this patch which tweaks the binder to allow negative 
>>>>> values for native pointers.
>>>>>
>>>>> The problem was previously discussed here: 
>>>>> http://mail.openjdk.java.net/pipermail/panama-dev/2018-October/002994.html
>>>>>
>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8212987
>>>>> Webrev : http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/
>>>>>
>>>>> As a reminder, I'm not a committer, so someone else will have to 
>>>>> push this.
>>>>>
>>>>> Thanks,
>>>>> Jorn
>>>>