[lworld] RFR: 8330582: [lworld] The JVM must enforce new field modifiers rules [v2]

[Dan Heidinga]

On Mon, 22 Apr 2024 17:48:00 GMT, Frederic Parain <fparain at openjdk.org> wrote:

>> Small changes to implement new field modifiers rules (JVMS 4.5 Fields) related to ACC_STRICT.
>> Those changes trigger some test failures because of a bug in javac related to final synthetic field. This bug is tracked by JDK-8330583.
>
> Frederic Parain has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix fields access flags filtering

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

> 4798:         is_illegal = true;
> 4799:       } else if (supports_inline_types()) {
> 4800:         if (!is_identity_class && !is_static && !is_strict) {

Suggestion:

        if (!is_identity_class && !is_static && !is_strict) {
            /* non-static value class fields must be be strict */

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

> 4800:         if (!is_identity_class && !is_static && !is_strict) {
> 4801:           is_illegal = true;
> 4802:         } else if (is_abstract && !is_identity_class && !is_static) {

Suggestion:

        } else if (is_abstract && !is_identity_class && !is_static) {
            /* abstract value classes can only have static fields */

If I'm reading that correctly, we're not yet allowing value classes to inherit instance fields.  Should this have a TODO so we come back and support as per JEP 401 later?

src/hotspot/share/include/jvm_constants.h line 49:

> 47:                                         JVM_ACC_ENUM | \
> 48:                                         JVM_ACC_SYNTHETIC | \
> 49:                                         JVM_ACC_STRICT)

Should we flip the order of these so they appear in bit order and match the METHOD_MODIFIERS order as well?

Suggestion:

                                        JVM_ACC_STRICT | \
                                        JVM_ACC_SYNTHETIC)

test/hotspot/jtreg/runtime/valhalla/inlinetypes/classfileparser/fieldModifiersTest.jcod line 158:

> 156: 
> 157: 
> 158: // A field has both ACC_STRCIT and ACC_STATIC set

Suggestion:

// A field has both ACC_STRICT and ACC_STATIC set

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1076#discussion_r1575190756
PR Review Comment: https://git.openjdk.org/valhalla/pull/1076#discussion_r1575192363
PR Review Comment: https://git.openjdk.org/valhalla/pull/1076#discussion_r1575197265
PR Review Comment: https://git.openjdk.org/valhalla/pull/1076#discussion_r1575199504