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

Fri Nov 2 04:02:18 UTC 2018

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.

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.

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. 

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