[foreign-abi] [Rev 01] RFR: 8237585: Dismantle ForeignUnsafe (foreign-abi part)

[Maurizio Cimadamore]

On Tue, 25 Feb 2020 17:18:23 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Removes ForeignUnsafe, moves `getUnsafeBase` and `getUnsafeAccess` to `MemoryAddress`, and moves `ofNativeUnchecked` to `MemorySegment`.
>> 
>> I've added a warning about the safety of these methods to the javadoc.
>> 
>> Use of the methods is guarded by a check against a system property, `jdk.incubator.foreign.permitUnsafeInterop` for the first 2, and `jdk.incubator.foreign.permitUncheckedSegments` for the latter. This needs to be set to `true` when running an application, or otherwise the use of these methods results in an `IllegalAccessError` being thrown.
>> 
>> I've updated the test accordingly, and also remove the FASTPATH option from the SysV ABI implementation, since it was not actually being used, but still causing the tests to be run multiple times.
> 
> The pull request has been updated with 1 additional commit.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 163:

> 162:      */
> 163:     static Object getUnsafeBase(MemoryAddress address) throws IllegalAccessError {
> 164:         Utils.checkUnsafeAccess("jdk.incubator.foreign.MemoryAddress#getUnsafeBase");

Maybe we should drop this?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java line 103:

> 102:                 System.err.println("WARNING: Accessing unsafe method: " + sourceMethod);
> 103:             case "debug" ->
> 104:                 System.out.println("DEBUG: Accessing unsafe method: " + sourceMethod);

Look at `IllegalAccessLogger::log` - that one prints the stack trace when the `debug` flavor is used:

// print warning if this is the first (or not a recent) usage
        if (added) {
            String msg = msgSupplier.get();
            if (mode == Mode.DEBUG) {
                StringBuilder sb = new StringBuilder(msg);
                stack.forEach(f ->
                    sb.append(System.lineSeparator()).append("\tat " + f)
                );
                msg = sb.toString();
            }
            warningStream.println(msg);
        }

Would be nice if we could reuse some of the code here.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java line 96:

> 95: 
> 96:     public static void checkUnsafeAccess(String sourceMethod) {
> 97:         switch (restrictedMethods) {

rename to checkRestrictedAcccess

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 433:

> 432:      * <p>
> 433:      * This method is <em>unsafe</em>. Its use can result in putting the VM in a corrupt state when used incorrectly,
> 434:      * and is provided solely to cover use-cases that can not otherwise be addressed safely. When used incorrectly, there

Let's use the term `restricted` in the italic. Also, let's make the the text more streamlined - e.g. 

This method is <em>restricted</em>. Restricted method are unsafe, and, if used incorrectly, their use might crash the JVM crash or, worse, silently result in memory corruption. Thus, clients should refrain from depending on restricted methods, and use safe and supported functionalities, where possible.

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/31