[lworld] RFR: 8288135: [lworld] Implement HotSpot flag -XX:+EnablePrimitiveClasses [v10]

[Frederic Parain]

On Thu, 29 Sep 2022 06:49:13 GMT, David Simms <dsimms at openjdk.org> wrote:

>> Runtime component of -XX:+EnablePrimitiveClasses switch
>
> David Simms has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 22 commits:
> 
>  - Merge branch 'lworld' into 8288135
>  - Extra assertion info for tracking spurious issue
>  - supports_inline_types() refers to classfile and is separate from Enable flags
>  - Further testing explicity using flags
>  - Disable EnablePrimitiveClasses by default
>  - Explictly flag more tests
>  - Merge branch 'lworld' into 8288135
>  - jdk_valhalla tests
>  - Merge branch 'lworld' into 8288135
>  - Further test fixes
>  - ... and 12 more: https://git.openjdk.org/valhalla/compare/951544ff...1853e0c6

Thank you for tackling this task.
My review is more focused on the VM side than on the tests.
A few comments to be addressed.

Thank you,

Fred

src/hotspot/share/classfile/classFileParser.cpp line 3328:

> 3326:       recognized_modifiers |= JVM_ACC_MODULE;
> 3327:     }
> 3328:     // JVM_ACC_VALUE, JVM_ACC_PRIMITIVE, and JVM_ACC_IDENTITY are defined depending on version and feature flag

The implementation doesn't seem to match the comment nor the intent of the patch. JVM_ACC_VALUE and JVM_ACC_IDENTITY should be controlled by the class file version (tested with supports_inline_types()), but JVM_ACC_PRIMITIVE should be controlled by the class file version and the EnablePrimitiveClasses flag.

src/hotspot/share/classfile/classFileParser.cpp line 5235:

> 5233:       // Can't enable this check fully until JDK upgrades the bytecode generators.
> 5234:       // For now, compare to class file version 51 so old verifier doesn't see Q signatures.
> 5235:       if ( (_major_version < 51 /* CONSTANT_CLASS_DESCRIPTORS */ ) || (!EnablePrimitiveClasses)) {

Are there a CRs to track the changes requires in the JDK and the status of this check and its dependency to JDK updates?

src/hotspot/share/classfile/classFileParser.cpp line 6183:

> 6181:     recognized_modifiers |= JVM_ACC_MODULE;
> 6182:   }
> 6183:   // Are JVM_ACC_VALUE and JVM_ACC_PRIMITIVE support (version and feature check)

JVM_ACC_PRIMITIVE should be controlled by both the value of supports_inline_types() and EnablePrimitiveClasses.

src/hotspot/share/runtime/arguments.cpp line 3036:

> 3034:   }
> 3035: 
> 3036:   if (!EnableValhalla) {

Why silently change the option when -XX:+EnablePrimitiveClass is specified without -XX:+EnableValhalla? Wouldn't be more informative to throw an error message?

test/hotspot/jtreg/runtime/valhalla/inlinetypes/classfileparser/BadACCValue.java line 27:

> 25:  * @test
> 26:  * @summary test that if a class file has ACC_VALUE or ACC_PRIMITIVE set then it must be run
> 27:  *          with option -XX:+ÉnableValhalla or -XX:+EnablePrimitiveClasses respectively.

Looks like there's an accent on the E of -XX:+ÉnableValhalla.

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

Changes requested by fparain (Committer).

PR: https://git.openjdk.org/valhalla/pull/727