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

[Jorn Vernee]

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