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

[Jorn Vernee]

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