[foreign] RFR 8212987 : Binder should allows negative values for native pointers

[Henry Jen]

On Nov 6, 2018, at 11:10 AM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
> 
> Hey Henry, thanks for taking a look at this again. Comments inline...
> 
>> The patch failed TestJextractFFI failed because of EVERYTHING throw
>> exception on length(), I think we could simply limit max length to -1L
>> or Long.MAX_VALUE depending on if we want to allow 63-bit or 64-bit
>> address and have Everything return accordingly. Before we have
>> unsigned address story complete, I prefer EVERYTHING should simply be
>> 0-Long.MAX_VALUE.
> 
> Ah, sorry, looks like that check is not in my local copy due to a bad merge (I have the unboxValue method in SystemABI).
> 
> The check that it's crashing on can be changed to `if(struct.ptr().type().bytesSize() != 0) {` instead, and that should fix the crash. Instead of checking the size of the underlying memory region, it would check the size of the struct's layout, and it seems that that is the actual intent of that check as well? i.e. checking that it's not an empty struct.
> 

I agree.

> About the length() method; I have another patch that moves the bytesSize() and elements() functions from `Pointer` to `Array` and that removes the need to have a length() method on BoundedMemoryRegion at all, since they can instead rely on the size field of the array. [1] (that patch is designed to apply on top of the one from this RFR)
> 
> This RFR should enable unsigned addresses, at least as far as BoundedMemoryRegion goes. It doesn't complete the story of having a BoundedMemoryRegion larger than Long.MAX_VALUE, but that is a different problem. If I make EVERYTHING be a region from 0-Long.MAX_VALUE again, but the offset is unsigned then AFAICT there are no use-cases left for EVERYTHING, since the offset could be outside of it, so it would always be out of bounds.
> 

Since you override checkBound and checkRange, negative address is possible. What I am suggesting is basically don’t override the length method, and that any pointer base on EVERYTHING is simply have a Long.MAX_VALUE of address space to access and be consistent all around until we figure out how to deal with length on MemoryRegion.

Cheers,
Henry


>> Any negative value exposed now may cause problem if user doesn’t treat
>> those as unsigned values.
> 
> Would just documenting that behavior be sufficient for the user you think? AFAICT the only way the offset is currently exposed is through the `Pointer::addr` method and through the string returned by `BoundedPointer::toString` (although it doesn't show up as negative for the latter).
> 
> Thanks,
> Jorn
> 
> [1] : http://cr.openjdk.java.net/~jvernee/panama/webrevs/arrays/webrev.01/