[foreign] RFR 8217414: Remove bounds check in BoundedPointer constructor

[Jorn Vernee]

Sorry, I forgot to add a test for struct field set as well.

Updated webrev: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217414/webrev.02

I also went ahead and implemented the check for Callbacks as well by 
delegating to the pointer unboxing code in the Callback unboxing code. 
There is no test for the Callback case since there was no public API to 
create a OOB Callback.

Jorn

Jorn Vernee schreef op 2019-01-21 15:29:
> Updated webrev:
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217414/webrev.01
> 
> I ended up not adding the check for Callbacks since AFAICT there is
> currently no public API to create a Callback with an OOB pointer.
> Should I add the check any ways?
> 
> Jorn
> 
> Jorn Vernee schreef op 2019-01-21 14:54:
>> Ok, I see your point. Plus, if a user finds the API too restrictive
>> they could always fall back on native allocation APIs like malloc &
>> free, or possibly custom LayoutTypes when those are introduced.
>> 
>> I think it will be sufficient to move the check to
>> `References.OfPointer` and `References.OfCallback`. But I will add
>> some tests to make sure as well.
>> 
>> Jorn
>> 
>> Maurizio Cimadamore schreef op 2019-01-21 14:42:
>>> On 21/01/2019 13:12, Jorn Vernee wrote:
>>>> There is a `checkAlive()` that gets called when a pointer is unboxed 
>>>> [1], I thought you were talking about that check. But, I don't think 
>>>> we should do a bounds check before passing a pointer to native code. 
>>>> The "one past the end of a container" pointer idiom is common and 
>>>> some APIs might expect to be passed such a pointer. It seems too 
>>>> restrictive to require that pointers passed to native code have to 
>>>> be in bounds (tbh I have my doubts about the liveliness check as 
>>>> well).
>>> 
>>> The whole point of the Panama API is to provide enriched safety. Now,
>>> if users want to opt out of that, we might (in the future) provide
>>> knobs to do so, but I see no point for passing a pointer that is not
>>> alive around (given it is known to contain garbage). Similarly, I see
>>> very little point in passing a pointer that is out of bounds - 
>>> granted
>>> there might be few use cases for that (e.g. pass a 'limit' pointer to
>>> a library), but I'd prefer to stick with stricter semantics and 
>>> loosen
>>> that up on a use case by use case basis.
>>> 
>>> I think the main point of this patch should be to _delay_ the check
>>> that is currently done on the constructor, not to remove it
>>> completely. It is ok if some piece of code temporarily produces an 
>>> out
>>> of bound pointers (e.g. when iterating), but I think we should limit
>>> the exposure of that weird pointer to the rest of the machinery.
>>> 
>>> Maurizio
>>> 
>>>> 
>>>> Jorn
>>>> 
>>>> [1] : 
>>>> http://hg.openjdk.java.net/panama/dev/file/tip/src/java.base/share/classes/jdk/internal/foreign/memory/BoundedPointer.java#l75
>>>> 
>>>> Maurizio Cimadamore schreef op 2019-01-21 13:56:
>>>>> I agree that the check should removed from the constructor - but we
>>>>> should make sure that checks happen when the pointer is used.
>>>>> Dereference is one situation, function call is another. I guess
>>>>> dereference is already covered (after all, a memory read will fail 
>>>>> if
>>>>> outside the boundaries), but I don't think passing an out of bound
>>>>> pointer to a function is currently being checked, and it should if 
>>>>> we
>>>>> remove the constructor eager check
>>>>> 
>>>>> Maurizio
>>>>> 
>>>>> On 21/01/2019 12:00, Jorn Vernee wrote:
>>>>>> Hi,
>>>>>> 
>>>>>> I have a patch addressing 2 vacuously passing tests 
>>>>>> (api/PointerTest and api/ArrayTest). The patch fixes the tests, 
>>>>>> and to make that happen removes the bounds check in 
>>>>>> BoundedPointer's constructor. As discussed offline; this check is 
>>>>>> not needed since we already de a check when dereferencing a 
>>>>>> pointer in BoundedMemoryRegion, and the check in the constructor 
>>>>>> is overly restrictive since it should be fine to create 
>>>>>> out-of-bounds pointers as long as it is not dereferenced. This 
>>>>>> does mean the tests needed to be tweaked since now an access 
>>>>>> exception is throw at a later point.
>>>>>> 
>>>>>> Please review the following.
>>>>>> 
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217414
>>>>>> Webrev: 
>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217414/webrev.00
>>>>>> 
>>>>>> Thanks,
>>>>>> Jorn