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

Tue Nov 6 17:46:31 UTC 2018

I am afraid any attempt to support negative address without completely considered address unsigned is not gonna working well.

On the SPARC note, I did a simple test on sun4u and sun4v, both not showing negative address. I suspect the negative address is observed on 32-bit OS.

void main(int argc, char** argv) {
    char* buf = malloc(1024);
    printf("Allocated address: %x\n", buf);

    printf("printf address: %x\n", printf);
}

The patch failed TestJextractFFI failed because of EVERYTHING throw exception on length(), I think we could simply limit max length to -1L or Long.MAX_VALUE depending on if we want to allow 63-bit or 64-bit address and have Everything return accordingly. Before we have unsigned address story complete, I prefer EVERYTHING should simply be 0-Long.MAX_VALUE.

Any negative value exposed now may cause problem if user doesn’t treat those as unsigned values.

Cheers,
Henry

LibClang crash recovery disabled
null
java.lang.UnsupportedOperationException
	at java.base/jdk.internal.foreign.memory.BoundedMemoryRegion$Everything.length(BoundedMemoryRegion.java:351)
	at java.base/jdk.internal.foreign.memory.BoundedPointer.bytesSize(BoundedPointer.java:103)
	at java.base/jdk.internal.foreign.invokers.UniversalNativeInvoker.unboxValue(UniversalNativeInvoker.java:176)
	at java.base/jdk.internal.foreign.invokers.UniversalNativeInvoker.invoke(UniversalNativeInvoker.java:125)
	at clang/clang.Index$Impl/0x00000007c0170040.clang_getTokenSpelling(Unknown Source)
	at jdk.internal.clang/jdk.internal.clang.TranslationUnit$Token.spelling(TranslationUnit.java:139)
	at jdk.internal.clang/jdk.internal.clang.TranslationUnit.tokens(TranslationUnit.java:72)
	at jdk.jextract/com.sun.tools.jextract.parser.Parser.handleMacro(Parser.java:168)
	at jdk.jextract/com.sun.tools.jextract.parser.Parser.lambda$parse$8(Parser.java:127)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$11$1.accept(ReferencePipeline.java:442)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1654)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
	at jdk.jextract/com.sun.tools.jextract.parser.Parser.parse(Parser.java:104)
	at jdk.jextract/com.sun.tools.jextract.Context.parse(Context.java:411)
	at jdk.jextract/com.sun.tools.jextract.Context.parse(Context.java:387)
	at jdk.jextract/com.sun.tools.jextract.Main.run(Main.java:251)
	at jdk.jextract/com.sun.tools.jextract.Main.main(Main.java:316)

> On Nov 2, 2018, at 2:09 PM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
> 
> 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