[foreign] RFR 8224835: Pointer validity checks are applied inconsistently

[Jorn Vernee]

Little update: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8224835/webrev.03/

Where I've also added 2 test cases to OutOfBoundsTest for the bounds 
checking for arrays and structs. And some cases to PointerTest which 
check that addr() is performing the needed checks.

Cheers,
Jorn

Jorn Vernee schreef op 2019-05-29 15:51:
> I feel your sore point. tbh, I think if we go for checks for structs
> and arrays we should do a bounds check as well. This will make sure
> that half of the struct/array is not out of bounds. e.g. in the array
> case, I think a user could do an unsafe cast to make an array pointer
> larger, and not get an exception when calling get(lastElement) if the
> element type is a struct or array.
> 
> One of the niceties of not doing the check for structs/arrays is that
> this avoids redundant bounds checking when first calling get on a
> struct/array pointer, and then again when accessing a field/element.
> But, I think maybe this could also be solved later on with a JIT
> intrinsic that merges the two checks (though I've heard that for those
> kinds of larger -> smaller bounds checks this is pretty tricky to do
> in general).
> 
> I've added the checks back in:
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8224835/webrev.01/
> 
> I've implemented this by calling checkAccess(Pointer.AccessMode.NONE),
> which effectively does no read/write access check, since we don't know
> at that point if the pointer will be written to, or read from yet.
> 
> Changed files are References (adding the checks), Pointer (adding the
> NONE access mode) and PointerScopeTest (sharpening the tests again).
> 
> Cheers,
> Jorn
> 
> Maurizio Cimadamore schreef op 2019-05-29 14:54:
>> Looks very good - the changes to Pointer:addr alone (and associated
>> cleanups) are worth :-)
>> 
>> I understand the choice with respect to liveness checks for structs
>> and arrays - this reflects our big type vs. small type distinction.
>> The sore point to note there is that you'd only get notified of a
>> failure if the element you are accessing is, again, not a struct or an
>> array. What if we added a call to checkAlive in the struct and array
>> constructors?
>> 
>> Maurizio
>> 
>> On 29/05/2019 13:26, Jorn Vernee wrote:
>>> Hi,
>>> 
>>> Please review the following:
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8224835
>>> Webrev: 
>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8224835/webrev.00/
>>> 
>>> This replaces the various public checkXXX methods on BoundedPointer 
>>> with a single public checkAccess(AccessMode) method, which is then 
>>> used everywhere outside of BoundedPointer. This makes sure that every 
>>> place does all the needed checks, and not only one or the other.
>>> 
>>> I've also checked all the use sites, and noticed that the 
>>> IllegalAccessException that Pointer::addr declared as being thrown 
>>> was never actually thrown. Instead an AccessControlException was 
>>> being thrown. So I've updated the relevant signatures as well, and 
>>> this allowed for the removal of a bunch of try/catch blocks.
>>> 
>>> I've also removed the checks in References.OfStruct::get and 
>>> References.OfArray::get, since we're not actually dereferencing the 
>>> pointer at that point, only wrapping it. The checks will still happen 
>>> later on when accessing a field or element (i.e. when actually 
>>> dereferencing the pointer).
>>> 
>>> Thanks,
>>> Jorn