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

Jorn Vernee jbvernee at xs4all.nl
Fri Nov 2 21:09:48 UTC 2018 

Comments inline...

Henry Jen schreef op 2018-11-02 19:19:
> 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?

Yeah, that seems fine.

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

It seems this doesn't hold for e.g. Solaris [1], but it depends on the 
OS. So it seems that dereferencing negative pointers will have to be 
supported at some point.

In the meantime I've worked on things a little more and revisited the 
idea of a subclass of BoundedMemoryRegion that doesn't do the bounds 
check. After thinking about it, I don't think this is as hacky as it 
seemed. The idea is that you can either have a memory region with an 
explicit length between 0 and Long.MAX_VALUE, and any offset, OR have a 
memory region starting at 0 that covers everything, without an explicit 
length. Since the only restriction is that the explicit length can not 
be larger than Long.MAX_VALUE, but still any offset is accepted I think 
this might get us a little more mileage.

Here's the webrev: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8212987/webrev.09/

Some implementation notes:

- Right now the memory region without an explicit offset or length 
(called Everything) is a nested subclass of BoundedMemoryRegion, but 
maybe these should be refactored into an abstract class and 
implementation pair.

- I noticed some missing access checks in put-/get-Address, and putBits 
was checking for read access instead of write access. So I've refactored 
the checks into checkRead and checkWrite methods to simplify things. The 
put-/get-Bits methods were also checking the size twice, both explicitly 
and as part of the switch, so I removed the explicit check.

- I've changed the BoundedPointer factories to use the new Everything 
class, instead of creating a memory region at the argument offset. That 
should allow 'looking back' with the returned pointer as well.

The other thing is that since the EVERYTHING region doesn't have a 
length, you can't request a pointer's `byteSize()` [2], or iterate over 
it's `elements()` [3]. I'm not sure I find those public API points all 
that sensible to be honest; with `byteSize()` I'd expect to get the size 
of the Pointer's layout, but instead it returns the size of the memory 
region - offset. You get the size of the layout by using 
`(type.layout().bitsSize() / 8)`. And something like `elements()` seems 
more at home in `Array`, since that has an explicit element count 
associated with it, while a pointer does not. Imho these API points 
should be moved to Array. The doc for `elements()` already talks about 
the pointer as if it's an array, so it seems like these API points were 
from before you had the Array interface?

Cheers,
Jorn

[1] : 
http://mail.openjdk.java.net/pipermail/panama-dev/2018-October/002995.html
[2] : 
http://hg.openjdk.java.net/panama/dev/file/cfee9bc4ebe4/src/java.base/share/classes/java/foreign/memory/Pointer.java#l94
[3] : 
http://hg.openjdk.java.net/panama/dev/file/cfee9bc4ebe4/src/java.base/share/classes/java/foreign/memory/Pointer.java#l58

> 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

More information about the panama-dev mailing list