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

Fri Nov 2 11:46:27 UTC 2018

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].

> 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.

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