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

Fri Nov 2 18:19:34 UTC 2018

On Nov 2, 2018, at 4:46 AM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
> 
> Henry Jen schreef op 2018-11-02 05:02:
>> This pretty much passed all tests, except that we are expecting wrong
>> exception, depends on SPEC, we either need to fix test or fix
>> implementation. I’ll let Maurizio comments on what exception should we
>> expect.
> 
> This depends on the used invoker again. Since the universal native invoker wraps the IOOBE inside an IllegalStateException that's what I'm checking for. TestNG doesn't seem to have an easy way to check the cause of an exception. I could do that manually, but I agree that it depends on the spec, so it needs to be decided what kind of exception we want to see there.
> 
>> However, change the BoundedPointer to create new MemoryRegion has a
>> subtle difference, which I think worth attention. EVERYTHING has a
>> special meaning that it’s the whole available memory. With the new
>> implementation, the region won’t allow access before the address, this
>> is not correct in turns of Pointer as it can be moved freely in the
>> bounded memory region, even backwards.
> 
> I hadn't thought about that yet, that's a good point.
> 
> Though I have to point out that the current implementation of some of the BoundedPointer factories were already disallowing this by creating a memory region at the requested offset [1].
> 

Thanks for all the work, you can tell this is prototype and not production quality. :P

To me, Pointer to throw IndexOutOfBoundException seems reasonable. Like you said, the UniversalNativeInvoker actually rely on Pointer.get() to get the return Pointer, and that wrap up the IOOBE into ISE.

For now, I think we can probably just relax the expectation to be RuntimeException?

>> Although we can argue that it’s better to have Pointer construct out
>> of a strictly bounded memory region, it’s a change of current behavior
>> and that probably need some discussion regarding to use cases.
>> Anyway, like I said, I think enable full 64bit address space is
>> probably not a bad thing. I suspect once we change the length instead
>> of using min and max address value, the bound check won’t be that
>> confusing, but you may feel differently.
> 
> I had a version which used min and max offset, but to make that safe I had to implement a version of Math.addExact for unsigned numbers, which was being called in checkBounds and checkRange to make sure that the offset being passed in didn't overflow when adding to the region's `min` address and length. The downside of this was that it made the implementation noticeably slower, I guess since my version of addExact isn't intrinsified. I got rid of one of the overflow checks by using lengthMinusOne (i.e. length - 1). I could probably look into that some more though.
> 
> Really, since BoundedMemoryRegion only does a bounds check and then an access, maybe we should choose a design where we create an unbounded version of `Pointer` and use that instead of using EVERYTHING. If the intent is really to make every 64bit address accessible than that should behave the same I think, but it seems much easier to make. It just leaves the problem of not being able to create a memory region of size >= Long.MAX_VALUE for another time.
> 

I like your idea. For special cases, that is with address to be negative, we can have a special unbounded pointer that cannot be dereferenced. After all, this is the use case here, a special error code. I doubt there would be a valid address with negative value, but I don’t have extensive knowledge about CPU architecture.

IMHO, when we want to enable full 64-bit address space, EVERYTHING should still mean the whole available address space, and NOTHING would be a special implementation that won’t be able to do much. By that, I mean BoundedMemoryRegion will always have size of at least one byte.

Cheers,
Henry

> What do you think?
> 
> Jorn
> 
> [1] : http://hg.openjdk.java.net/panama/dev/file/cfee9bc4ebe4/src/java.base/share/classes/jdk/internal/foreign/memory/BoundedPointer.java#l178
> 
>> Cheers,
>> Henry
>> test PointerTest.test1ByteRegionAtMaxHeapOffset(): success
>> test PointerTest.testAutomaticLengthRegionNotBigEnough(): failure
>> java.lang.IndexOutOfBoundsException: offset=0x3 length=0x1
>> 	at
>> java.base/jdk.internal.foreign.memory.BoundedMemoryRegion.checkBounds(BoundedMemoryRegion.java:159)
>> 	at
>> java.base/jdk.internal.foreign.memory.BoundedMemoryRegion.checkRange(BoundedMemoryRegion.java:166)
>> 	at
>> java.base/jdk.internal.foreign.memory.BoundedPointer.<init>(BoundedPointer.java:77)
>> 	at
>> java.base/jdk.internal.foreign.memory.BoundedPointer.cast(BoundedPointer.java:149)
>> 	at
>> java.base/jdk.internal.foreign.invokers.DirectSignatureShuffler.longToPointer(DirectSignatureShuffler.java:335)
>> 	at
>> PointerTest$pointers$Impl/0x00000007c032e040.get_overflow_pointer(Unknown
>> Source)
>> 	at PointerTest.testAutomaticLengthRegionNotBigEnough(PointerTest.java:292)
>> 	at
>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
>> Method)
>> 	at
>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>> 	at
>> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>> 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
>> 	at
>> org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:85)
>> 	at org.testng.internal.Invoker.invokeMethod(Invoker.java:639)
>> 	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:821)
>> 	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1131)
>> 	at
>> org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:124)
>> 	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
>> 	at org.testng.TestRunner.privateRun(TestRunner.java:773)
>> 	at org.testng.TestRunner.run(TestRunner.java:623)
>> 	at org.testng.SuiteRunner.runTest(SuiteRunner.java:357)
>> 	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:352)
>> 	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:310)
>> 	at org.testng.SuiteRunner.run(SuiteRunner.java:259)
>> 	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
>> 	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
>> 	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1185)
>> 	at org.testng.TestNG.runSuitesLocally(TestNG.java:1110)
>> 	at org.testng.TestNG.run(TestNG.java:1018)
>> 	at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94)
>> 	at
>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
>> Method)
>> 	at
>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>> 	at
>> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>> 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
>> 	at
>> com.sun.javatest.regtest.agent.MainActionHelper$SameVMRunnable.run(MainActionHelper.java:229)
>> 	at java.base/java.lang.Thread.run(Thread.java:835)
>>> On Nov 1, 2018, at 6:27 PM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
>>> Ok, I have one more addition to this. I figured out the bug I was seeing before; it turns out I was passing a double offset.
>>> I've removed the hackish anonymous class, and removed BoundedMemoryRegion.EVERYTHING for now since the length is just not big enough to cover all cases. Instead I'm now creating a new memory region at the requested offset for BoundedPointer. I looked into treating `length` as unsigned as well but this was pretty tricky, since to represent the length of EVERYTHING you need to have a 65-bit number. So I was working with lengthMinusOne everywhere but that was a lot more confusing. I think what I have right now is better than that.
>>> Updated webrev: http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.07/
>>> Thanks for all the reviews and discussion so far.
>>> Cheers,
>>> Jorn
>>> Jorn Vernee schreef op 2018-11-01 17:42:
>>>> Henry Jen schreef op 2018-11-01 16:21:
>>>>> I was hesitate to reply as the fix is kind of hackish, but not ready
>>>>> to suggest we go all the way to enable full range of 64-bit, after
>>>>> all, Long.MAX_VALUE is giving us 8EB and that’s seems way more than
>>>>> enough. But to completely address rather than simply make it work for
>>>>> special cases, we probably should just treat address as unsigned and
>>>>> use Long.compareUnsigned to do comparison.
>>>> It feels kind of hackish to me too tbh, especially since I can only do
>>>> partial testing. But if all the tests pass on your side as well then I
>>>> see no problem for now. If this is not the case then I will leave it
>>>> for now and focus on something else until I also have the full test
>>>> suite running as well.
>>>> As an interim alternative for dealing with special values maybe we
>>>> could add a 'dummy' pointer type that is used for negative values, but
>>>> does not support getting/setting and is really only a wrapper around a
>>>> `long`. The way to use it would be to pass it back to native which
>>>> _can_ handle the negative value. But that is not a long term solution,
>>>> since some systems use negative pointers as normal, as pointed out by
>>>> Florian [1].
>>>>> Therefore, max length for a memory always 0 - min. The length should
>>>>> be Long.compareUnsigned(length, maxLength) <= 0, with all other offset
>>>>> operation compare against length established at construction time with
>>>>> Long.compareUnsigned, that should be enough to deal with this.
>>>> OK, I will use Long.compareUnsigned to do the check.
>>>>> However, taking offset with large(negative) number seems really confusing.
>>>> Yes, so hopefully in almost all cases this will be abstracted away,
>>>> and not really visible. `long` is kind of a sub-optimal carrier type
>>>> for a pointer value for this reason I think. It's OK to store the
>>>> pointer, but things get confusing when you try to look at it, since it
>>>> will show up as a signed integer. But maybe once we have value types
>>>> we could consider having an UnsignedLong type which has the right
>>>> semantics and representation to it?
>>>> Here is a version that uses Long.compareUnsigned to do the overflow
>>>> check:
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.06/
>>>> There are 2 lines of factory methods; the first which calculate the
>>>> length and then pass args to a private internal factory method, the
>>>> others that check the input length for overflow and then pass args to
>>>> the internal factory. The internal factory does not do the overflow
>>>> check.
>>>> Jorn
>>>>> What do you think?
>>>>> Cheers,
>>>>> Henry
>>>> [1] : http://mail.openjdk.java.net/pipermail/panama-dev/2018-October/002995.html