[foreign-memaccess+abi] RFR: 8293367: Enable native access for modules not in the boot layer [v2]

[Alan Bateman]

On Tue, 20 Sep 2022 14:18:19 GMT, Athijegannathan Sundararajan <sundar at openjdk.org> wrote:

>> Adding a new API ModuleLayer.Controller.enableNativeAccess(Module).
>> Reworked thread safety of enable native access flag reading and setting.
>
> Athijegannathan Sundararajan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review comments.

src/java.base/share/classes/java/lang/Module.java line 277:

> 275:         synchronized(target) {
> 276:             return target.enableNativeAccess;
> 277:         }

enableNativeAccess is `@Stable` and I'm trying to understand the access. ensureNativeAccess will only synchronize when false, isNativeAccessEnabled synchronizes always. Is that intentional?

src/java.base/share/classes/java/lang/Module.java line 280:

> 278:     }
> 279: 
> 280:     // Returns the Module object holds the enableNativeAccess

There's a typo, should be "that holds".  Also the method name is a bit misleading as it doesn't enable anything.  The isNativeAccessEnabled and enableNativeAccess are deeply coupled so the method isn't strictly needed. If you keep it then something like moduleForNativeAccess() in the usage by both methods might be clearer.

src/java.base/share/classes/java/lang/Module.java line 293:

> 291:         boolean isNativeAccessEnabled = target.enableNativeAccess;
> 292:         if (!isNativeAccessEnabled) {
> 293:             synchronized(target) {

Minor formatting nit, missing space in `synchronized(target)` and it looks like you are using 3 rather than 4-space indent.

src/java.base/share/classes/java/lang/Module.java line 325:

> 323:      * Update all unnamed modules to allow access to restricted methods.
> 324:      */
> 325:     static void implAddEnableNativeAccessAllUnnamed() {

Maybe this can be implAddEnableNativeAccessToAllUnnamed() as the existing methods that expand access to code in unnamed modules are named xxxToAllUnamed.

src/java.base/share/classes/java/lang/ModuleLayer.java line 305:

> 303:         /***
> 304:          * Enables native access for a module in the layer if the caller's module
> 305:          * already has native access.

The API looks okay, ModuleLayer.Controller is the right place for this. I think you can drop "already" from the javadoc.

src/java.base/share/classes/java/lang/ModuleLayer.java line 313:

> 311:          * where possible.
> 312:          *
> 313:          * @param target the module that's given native access

The other methods use the phrase "Updates module ..." so I think target should probably be described here as "The module to update".

src/java.base/share/classes/java/lang/ModuleLayer.java line 320:

> 318:          * and the command line option {@code --enable-native-access} is specified, but does not
> 319:          * mention the module name {@code M}, or {@code ALL-UNNAMED} in case {@code M} is an
> 320:          * unnamed module.

Formatting nit, would you mind using the same style for the @param and @throws as the other methods, only to keep the code using one style.

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

PR: https://git.openjdk.org/panama-foreign/pull/729