[foreign] RFR 8213343: Update the public access mode API

Wed Nov 21 13:23:12 UTC 2018

Maurizio Cimadamore schreef op 2018-11-21 12:46:
> Hi Jorn,
> the patch is sensible, of course, but I do have some comments, some
> general, some more specific.
> 
> From a general perspective, I think in general it's better to keep the
> patches focussed on the issue at hand. If the problem here is to fix
> access mode, let's focus on that; I note that there's quite a bit of
> other refactoring going on, and that makes the review process harder.

Ok. Sorry about that.

> In terms of design, I view all the read/write accesses as some kind of 
> a placeholder for some story yet to materialize (and that is probably 
> going to become clearer when we'll use this API in anger for other 
> things such as ByteBuffers). That said, it is true that the constants 
> are not exposed in the public API and that is, in itself an issue.
> 
> I can see two ways of solving this problem:
> 
> - adding an enum, as you did; or adding public static final constants
> 
> Then, I can also see few different ways to change access mode:
> 
> - with a single method which takes an EnumSet (in case we encode access 
> as enum) or int mask (in case we encode access as ints, as now)
> - with separate 'asRead', 'asWrite' methods, as you did. This assumes 
> that we always start from a 'RW' state and the only possible choice is 
> to restrict the access. Seems like a sensible assumption

I considered the first option as well, but with the assumption that you 
can only narrow access, inhibiting the ability to use MODE_RW as an 
argument seemed sensible to me.

> Last, but more importantly, where does the access belong to:
> 
> - the pointer
> 
> - the region (what we have now)
> 
> - the scope
> 
> Having access on the pointer/region seems more flexible, but I also
> wonder how much people will take advantage of such flexibility. I
> believe in practice a scope will probably be created as 'morally
> read-only' from the start - e.g. all pointers allocated through it
> should be read-only. But we don't have enough hard evidence one way or
> another.

I was thinking about people writing libraries on top of panama. They 
might want to create a bit of memory, write some values to it, and then 
give out a read-only pointer to the client code. If the scope only gives 
out read-only pointers, then writing to them can AFAICT only take place 
on the native side.

> More specific comments on the code:
> 
> * the enum should be nested in the public Pointer API, as that's where 
> it's used

I changed the entire stack that was using `int` flags to using the enum. 
Since the access check happen in BoundedMemoryRegion it made sense to 
have the enum separate, but from a public API perspective the access 
mode is an attribute of a pointer, so I agree that it makes sense to put 
it there.

> * MODE_XYZ should be renamed to just R/W/RW (or
> READ/WRITE/READ_WRITE); after all the client code will do
> AccessMode... so repeating MODE in the constant name seems odd

Agreed.

> * in BoundedArray::cast I wonder why we are not checking compatibility
> of the element type (not an issue introduced by your patch)

I was wondering about that as well. I tried changing it to cast the 
element pointer:

     return new BoundedArray<>(elementPointer().cast(type), size);

But ScopeTest is failing when I do that. It's casting an i8 array to i32 
[2].

The argument size is also not being checked, so someone could make the 
array larger if they wanted. Is that something that should be 
dis-allowed as well?

> * The tests work fine for me even with the DIrectSignatureShuffler
> changes; that said, I think that change is suspicious because it
> relies on the pointer have same offset/size as the argument binding
> being passed as argument. So I would revert it anyway.

This is surprising. But, it also adds noise to the patch, so I'll leave 
that stuff out for now.

> * we typically do not use packages in tests

OK.

Thanks for the review so far.

Updated webrev: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/accessmode/webrev.04/

Jorn

[2] : 
http://hg.openjdk.java.net/panama/dev/file/e73c69d8f5b6/test/jdk/java/foreign/ScopeTest.java#l110

> 
> Cheers
> Maurizio
> 
> 
> On 21/11/2018 02:35, Jorn Vernee wrote:
>> Ok, I've found one more mistake.
>> 
>> I made this patch a while ago already, and ran the tests to see if it 
>> still worked. However I seem to have made a mistake in code that is 
>> not being tested on windows (DirectSignatureShuffler). It seems 
>> obvious now, but I guess I missed it a while back.
>> 
>> Updated webrev: 
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/accessmode/webrev.03/
>> 
>> I've reverted the BoundedPointer fields back to being public. Making 
>> them private wasn't as simple of a change as I thought.
>> 
>> Sorry about that,
>> Jorn
>> 
>> 
>> Jorn Vernee schreef op 2018-11-21 00:22:
>>> And I suppose it's useful if I include the actual AccessMode class in
>>> the webrev as well (oops)
>>> 
>>> Webrev: 
>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/accessmode/webrev.01/
>>> 
>>> Cheers,
>>> Jorn
>>> 
>>> Jorn Vernee schreef op 2018-11-21 00:14:
>>>> Hi,
>>>> 
>>>> While reading through the Pointer class, I noticed there is a method
>>>> `isAccessibleFor(int)` which is used to check if the pointer is
>>>> accessible for a certain access mode.
>>>> 
>>>> The problem is that the `int` value that's expected as an argument 
>>>> is
>>>> defined as the constants MODE_R, MODE_W and MODE_RW in
>>>> BoundedMemoryRegion [1]. This is in an internal package, so there 
>>>> will
>>>> be no way for the user to use those constants.
>>>> 
>>>> The other problem is that having an `int` as the argument type gives
>>>> no hint to what values can be used there. I would like to advocate 
>>>> for
>>>> switching to an enum there, since then it's a simple case of typing 
>>>> in
>>>> the enum name, hitting the auto-complete key and getting a list of
>>>> available argument values.
>>>> 
>>>> The third problem is that there seems to be no public API point for
>>>> setting the access mode of a Pointer. I can imagine that somebody
>>>> might want to give out a read-only pointer to a client (and maybe 
>>>> even
>>>> a write-only pointer).
>>>> 
>>>> I have a patch that attempts to solve all 3 problems;
>>>> 
>>>> 1 & 2: I have created an AccessMode enum in j.f.memory that is used 
>>>> in
>>>> place of the `int`s.
>>>> 3: I have added 2 methods to Pointer, asReadOnly and asWriteOnly, to
>>>> create a new pointer from the given one with either of the 2 access
>>>> modes.
>>>> 
>>>> As an additional cleanup I've also removed the mode field from
>>>> BoundedPointer, since it was unused, and made the other fields 
>>>> private
>>>> instead of public. In order to do the latter, I added
>>>> putLongBits/getLongBits methods to BoundedPointer, which replaces
>>>> direct access to the pointer's region put/get methods. I also added 
>>>> a
>>>> package declaration to ArrayTest since my IDE (thanks Maurizio ;P) 
>>>> was
>>>> complaining about a duplicate class, since there was a test class 
>>>> with
>>>> the same name in the jextract test directory.
>>>> 
>>>> Related bug: https://bugs.openjdk.java.net/browse/JDK-8213343
>>>> Webrev: 
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/accessmode/webrev.00/ 
>>>> Please let me know what you think about this.
>>>> 
>>>> Thanks,
>>>> Jorn
>>>> 
>>>> [1] :
>>>> http://hg.openjdk.java.net/panama/dev/file/cfee9bc4ebe4/src/java.base/share/classes/jdk/internal/foreign/memory/BoundedMemoryRegion.java#l33