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

[Jorn Vernee]

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