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

Wed Oct 31 11:02:59 UTC 2018

Oh. I ran the tests, but I'm getting a pass there since Windows does not 
use the direct invoker, which means a different factory method on 
BoundedPointer is getting called, which doesn't offset into EVERYTHING. 
I'm afraid that's not something I can test right now by myself.

I tried to fix it by removing EVERYTHING in favor of calling 
BoundedMemoryRegion.of(offset). But when testing that I'm seeing a bunch 
of crashes in `V ~BufferBlob::invoke_native_blob` [1]. I think that is 
the generated downcall stub that's crashing right? Not sure what that's 
about, but I guess maybe some objects are now getting GC'd too early 
since the memory region is no longer saved in a static field, or maybe I 
still have a bug in that stub generation code :/ (I took another look, 
but everything looks OK to me).

So instead I've made EVERYTHING an anonymous subclass which overrides 
checkBounds to be empty, since technically any offset is in bounds.

I also realized the calculation still needed a special case for 
Long.MIN_VALUE since that can't be negated.

Can you test the new webrev? 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.05/

Thanks,
Jorn

[1] : 
http://hg.openjdk.java.net/panama/dev/file/f1afd0ae2932/src/hotspot/share/prims/universalNativeInvoker.cpp#l44

Henry Jen schreef op 2018-10-31 01:05:
> That looks good, but we still have a little issue to iron out.
> 
> New test failed because of BoundedPointer is offsetting EVERYTHING
> region, where length set to Long.MAX_VALUE.
> 
> Cheers,
> Henry
> 
>> test PointerTest.test1ByteRegionAtMaxHeapOffset(): failure
>> java.lang.IndexOutOfBoundsException: offset=0xffffffffffffffff 
>> length=0x7fffffffffffffff
>> 	at 
>> java.base/jdk.internal.foreign.memory.BoundedMemoryRegion.checkBounds(BoundedMemoryRegion.java:153)
>> 	at 
>> java.base/jdk.internal.foreign.memory.BoundedMemoryRegion.checkRange(BoundedMemoryRegion.java:158)
>> 	at 
>> java.base/jdk.internal.foreign.memory.BoundedPointer.<init>(BoundedPointer.java:77)
>> 	at 
>> java.base/jdk.internal.foreign.memory.BoundedPointer.<init>(BoundedPointer.java:68)
>> 	at 
>> java.base/jdk.internal.foreign.memory.BoundedPointer.createNativeVoidPointer(BoundedPointer.java:175)
>> 	at 
>> java.base/jdk.internal.foreign.invokers.DirectSignatureShuffler.longToPointer(DirectSignatureShuffler.java:335)
>> 	at 
>> PointerTest$pointers$Impl/0x00000007c035b440.get_1_byte_pointer(Unknown 
>> Source)
>> 	at PointerTest.test1ByteRegionAtMaxHeapOffset(PointerTest.java:297)
>> 	at 
>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
>> Metho
> 
>> On Oct 30, 2018, at 11:52 AM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
>> 
>> Henry Jen schreef op 2018-10-30 18:46:
>>> I wonder if it can be simplified a little bit, let’s say address is
>>> indeed 0xFFFFFFFF, we should be able to have a memory region of 
>>> length
>>> 1.
>> 
>> Yes, that's a good point. That will allow for a 1 byte memory region 
>> at 0xFFFFFFFF.
>> 
>> I've also added a test for that exact case.
>> 
>>> When the min(real address) is negative, the cap of length for unknown
>>> is pretty much just to negate the value of min.
>> 
>> Yes! And I think when min is positive the length maximum is always 
>> Long.MAX_VALUE.
>> 
>>> min + length should be <= 0. We can only access min + offset - 1, so
>>> that we won’t access 0x0 in negative case.
>> 
>> That won't work for the case where min and length are both positive, 
>> but min + length is also positive. In that case the check should also 
>> pass.
>> 
>> The way I have it right now, since length is always positive, min + 
>> length can only overflow to positive if min is negative, so that's 
>> also the check :) But I've changed it to check for min + length > 0 to 
>> allow for a 1 byte region if min == -1L.
>> 
>>> What do you think?
>> 
>> Good suggestions, here is the updated webrev: 
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.04/index.html
>> 
>> I've also removed the spurious import in PointerTest, which 
>> auto-complete put in there when auto-completing to @Transient instead 
>> of @Test :)
>> 
>>> Cheers,
>>> Henry
>> 
>> Thanks for the reviews,
>> Jorn
>> 
>>>> On Oct 30, 2018, at 9:49 AM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
>>>> Updated webrev: 
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.03/
>>>> (beware that the BoundedMemoryRegion diff turned out a bit 
>>>> confusing)
>>>> - Changed BoundeMemoryRegion to use factory methods.
>>>> - Factories that don't take a length calculate a max length that can 
>>>> not overflow (in the unsigned sense).
>>>> - Moved BoundedMemoryRegion anonymous class from Pointer to 
>>>> BoundedMemoryRegion, since constructor is now private.
>>>> - Added one more test to check for the error in the case where the 
>>>> automatic length calculated for a native pointer is shorter than the 
>>>> length required by the carrier type.
>>>> - Switched PointerTest to TestNG, since that makes it easier to test 
>>>> for exceptions.
>>>> I've added a check for the overflow to BoundedMemoryRegion, but when 
>>>> trying to test for it it turned out that BoundedPointer was already 
>>>> catching that case. I've left the check there as it will still be 
>>>> useful to catch programming errors when an explicit length is being 
>>>> passed.
>>>> Trying to treat a signed long as unsigned is somewhat confusing, 
>>>> maybe we should add an UnsignedLong utility class that abstracts 
>>>> this away in a later RFR, especially if we start allowing 'unsigned' 
>>>> longs in more places.
>>>> Jorn
>>>> Jorn Vernee schreef op 2018-10-30 11:26:
>>>>> You're right about `toUnsignedString`. For some reason I thought
>>>>> `toHexString` had the `-` as a prefix if the argument is negative.
>>>>> About the length constraint; My initial thought was just to trust 
>>>>> the
>>>>> pointer returned from native, but I guess we should do better. In 
>>>>> the
>>>>> specific case I'm testing, void *, the length that is being used is
>>>>> Long.MAX_VALUE, basically the length is unknown, so I don't think I
>>>>> can do a range check on min + length in the constructor in that 
>>>>> case.
>>>>> As far as I can see, the `min` value is only used in a few min +
>>>>> offset calculations before passing to Unsafe put/get routines, I 
>>>>> think
>>>>> we'd want to allow overflow to negative there, but not overflow to
>>>>> positive, basically treating the long as if it was unsigned.
>>>>> I think I will switch BoundedMemoryRegion to having factory methods
>>>>> that do the validation, and then have ones where the length is 
>>>>> known,
>>>>> which do a range check, and ones where the length is unknown, that 
>>>>> cap
>>>>> the it to where it can not overflow. Then I can use the latter for
>>>>> void *. If a bad pointer is passed from native, the known-length 
>>>>> case
>>>>> should fail on construction of the memory region, and the
>>>>> unknown-length case should fail when dereferencing (since there is
>>>>> already a bounds check being done).
>>>>> What do you think?
>>>>> Thanks,
>>>>> Jorn
>>>>> Henry Jen schreef op 2018-10-30 01:59:
>>>>>> BTW, the change in toString is not needed, as toUnsignedString(l, 
>>>>>> 16)
>>>>>> is same as toHexString(l)?
>>>>>> Cheers,
>>>>>> Henry
>>>>>>> On Oct 29, 2018, at 5:56 PM, Henry Jen <henry.jen at oracle.com> 
>>>>>>> wrote:
>>>>>>> I am afraid this is not that simple, we need to at least protect 
>>>>>>> length from overflow the address to >= 0.
>>>>>>> I haven’t completely check the implementation, we need to make 
>>>>>>> sure all other operation will not expand or shift the region, 
>>>>>>> which I believe is the constraint of the design. If that’s indeed 
>>>>>>> the case, guard at construction is probably good enough.
>>>>>>> Cheers,
>>>>>>> Henry
>>>>>>>> On Oct 29, 2018, at 10:04 AM, Jorn Vernee <jbvernee at xs4all.nl> 
>>>>>>>> wrote:
>>>>>>>> Hello,
>>>>>>>> Please review this patch which tweaks the binder to allow 
>>>>>>>> negative values for native pointers.
>>>>>>>> The problem was previously discussed here: 
>>>>>>>> http://mail.openjdk.java.net/pipermail/panama-dev/2018-October/002994.html
>>>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8212987
>>>>>>>> Webrev : 
>>>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/
>>>>>>>> As a reminder, I'm not a committer, so someone else will have to 
>>>>>>>> push this.
>>>>>>>> Thanks,
>>>>>>>> Jorn