8380129: Remove AccessFlags::print_on in favor of context-specific printing by caspernorrbin · Pull Request #30746 · openjdk/jdk

caspernorrbin · 2026-04-15T14:48:15Z

Hi everyone,

AccessFlags::print_on is used for class, field and method printing, but those entities do not share the same modifier set. The current helper hard-codes a single mixed list of access flags for all 3 and is thus unaware of the type of the printed value. Instead of using a single shared printing function, we should move the printing to the relevant class, field, and method call sites. This makes the printer aware of the type it is printing and lets us check only the flags that are relevant for that type.

For this change, I remove AccessFlags::print_on and split the printing into 3 separate helpers:

InstanceKlass::print_class_flags
Method::print_access_flags
fieldDescriptor::print_access_flags

As a part of that, I added the missing AccessFlags predicates used by each of the new printers, and updated each printer to check all the flags relevant for its type, as defined by jvm_constants.h. This lets us cover class-specific, method-specific, and field-specific modifiers that were not handled before.

I also added new gtests covering each of the 3 printing helpers and the previously missing flags.

Testing:

Oracle tiers 1-3
New gtests covering each of the printing helpers and each of the new flags

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8380129: Remove AccessFlags::print_on in favor of context-specific printing (Bug - P3)

Reviewers

Chen Liang (@liach - Reviewer)
David Holmes (@dholmes-ora - Reviewer)
Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30746/head:pull/30746
$ git checkout pull/30746

Update a local copy of the PR:
$ git checkout pull/30746
$ git pull https://git.openjdk.org/jdk.git pull/30746/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30746

View PR using the GUI difftool:
$ git pr show -t 30746

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30746.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-04-15T14:49:50Z

👋 Welcome back cnorrbin! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-04-15T14:49:53Z

@caspernorrbin This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8380129: Remove AccessFlags::print_on in favor of context-specific printing

Reviewed-by: liach, dholmes, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 172 new commits pushed to the master branch:

eb9a8ab: 8376626: Shenandoah: Remove command line options for setting min and max region sizes
7b92998: 8379722: C2: assert that result is used for CallLeafPureNode in final graph reshaping
b41edeb: 8383183: Shenandoah: Mangle trashed regions up to top instead of end
... and 169 more: https://git.openjdk.org/jdk/compare/0df4bd489de00056736321f1e77389add7a41f83...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2026-04-15T14:51:28Z

@caspernorrbin The following labels will be automatically applied to this pull request:

hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2026-04-15T14:54:57Z

Webrevs

liach

This direction looks reasonable. Similar effort happens in core libraries in #30093.

liach · 2026-04-15T15:41:04Z

+  bool is_annotation  () const         { return (_flags & JVM_ACC_ANNOTATION  ) != 0; }
  bool is_interface   () const         { return (_flags & JVM_ACC_INTERFACE   ) != 0; }
  bool is_abstract    () const         { return (_flags & JVM_ACC_ABSTRACT    ) != 0; }
+  bool is_strict_method() const        { return (_flags & JVM_ACC_STRICT      ) != 0; }


If "strict method" sounds too weird, we can call this "strictfp" following the original Java language modifier's name.

This will be short-lived either way as valhalla gets rid of the legacy strictfp notion for methods anyway.

Renamed to is_strictfp instead.

liach · 2026-04-15T15:45:26Z

  void print() const;
  void print_on(outputStream* st) const;
  void print_on_for(outputStream* st, oop obj);
+#if !defined(PRODUCT) || INCLUDE_JVMTI


The new !defined(PRODUCT) seems to be only for the gtest. I wonder if gtest can run the test for print_access_flags based on INCLUDE_JVMTI flag instead; the macro condition tweak seems weird.

I am surprised that we only ever print access flags in relation to JVMTI - I would have expected logging or crash reporting to do so. In any case I also find the !PRODUCT a little jarring. Given we always have JVMTI it makes no different to our binaries.

After some thought, I have removed all the #if guards on the printers. To me, these are now ordinary printing utilities belonging to either Method, InstanceKlass, or fieldDescriptor. They are not directly tied to JVMTI, so should not be guarded by it. Let me know if you agree.

That is fine with me.

liach · 2026-04-15T15:46:30Z

  bool is_volatile    () const         { return (_flags & JVM_ACC_VOLATILE    ) != 0; }
+  bool is_bridge      () const         { return (_flags & JVM_ACC_BRIDGE      ) != 0; }
  bool is_transient   () const         { return (_flags & JVM_ACC_TRANSIENT   ) != 0; }
+  bool has_vararg     () const         { return (_flags & JVM_ACC_VARARGS     ) != 0; }


I recommend is_varargs, this flag is not enforced by the VM so technically users can create class files that declare varargs methods without a trailing array argument.

I renamed it to is_varargs. I originally picked has_varargs because that is the name on the valhalla branch, but I am fine with this too.

dholmes-ora

I think this is a reasonable approach though I have little doubt something somewhere will be tripped up by these changes. There's an existing issue with inner classes that we may, or may not want to address (not necessarily in this PR).

Thanks

dholmes-ora · 2026-04-16T05:23:41Z

 }

+#if !defined(PRODUCT) || INCLUDE_JVMTI
+void InstanceKlass::print_class_flags(outputStream* st) const {


This is incomplete. If the class is an inner class then additional access flags are possible (private, protected, static).
EDIT: Hmm jvm_constants.h does not recognise this either via JVM_RECOGNIZED_CLASS_MODIFIERS. Not sure how this should be handled.

Also do we need to handle ACC_MODULE, or do we not actually create an instanceKlass for those?

On jdk side we never have Class for modules. These classfiles are exclusively handled by Java code in ModuleDescriptor I think?

I have now changed it to use compute_modifier_flags() instead of access_flags() directly. With this, we get member-class modifiers printed. I expanded the test which shows that private/protected static gets printed. This matches the class' modifiers, which feels like the correct behavior.

ACC_MODULE is not needed here because it never becomes an InstanceKlass. It is rejected as a normal class during parsing.

dholmes-ora · 2026-04-16T05:40:02Z

  st->print   (" - constants:         " PTR_FORMAT " ", p2i(constants()));
  constants()->print_value_on(st); st->cr();
-  st->print   (" - access:            0x%x  ", access_flags().as_method_flags()); access_flags().print_on(st); st->cr();
+  st->print   (" - access:            0x%x  ", access_flags().as_method_flags()); print_access_flags(st); st->cr();


There is an existing inconsistency here in that we print the raw flags as "method flags" only but then we print them all. Your new code implicitly filters the flags by only printing the expected method flags.

as_method_flags() asserts that only recognized method modifiers are set, so these are not arbitrary raw flags. Previously, the generic printer could misinterpret overlapping bits, whereas the new method-specific printer prints them correctly. This looks like the right behavior to me.

dholmes-ora · 2026-04-16T05:43:46Z

  void print() const;
  void print_on(outputStream* st) const;
  void print_on_for(outputStream* st, oop obj);
+#if !defined(PRODUCT) || INCLUDE_JVMTI


I am surprised that we only ever print access flags in relation to JVMTI - I would have expected logging or crash reporting to do so. In any case I also find the !PRODUCT a little jarring. Given we always have JVMTI it makes no different to our binaries.

dholmes-ora · 2026-04-16T05:44:21Z

  bool is_volatile    () const         { return (_flags & JVM_ACC_VOLATILE    ) != 0; }
+  bool is_bridge      () const         { return (_flags & JVM_ACC_BRIDGE      ) != 0; }
  bool is_transient   () const         { return (_flags & JVM_ACC_TRANSIENT   ) != 0; }
+  bool has_vararg     () const         { return (_flags & JVM_ACC_VARARGS     ) != 0; }


dholmes-ora · 2026-04-16T05:50:41Z

+  bool is_annotation  () const         { return (_flags & JVM_ACC_ANNOTATION  ) != 0; }
  bool is_interface   () const         { return (_flags & JVM_ACC_INTERFACE   ) != 0; }
  bool is_abstract    () const         { return (_flags & JVM_ACC_ABSTRACT    ) != 0; }
+  bool is_strict_method() const        { return (_flags & JVM_ACC_STRICT      ) != 0; }


This will be short-lived either way as valhalla gets rid of the legacy strictfp notion for methods anyway.

caspernorrbin · 2026-04-16T12:59:48Z

Thank you for the reviews! I pushed some changes from your feedback. I am also running some more testing into more tiers to ensure nothing new comes up.

openjdk · 2026-04-16T18:54:17Z

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot. This can be overridden with the /reviewers command.

dholmes-ora · 2026-04-17T06:57:13Z

+  if (flags.is_private   ()) st->print("private ");
+  if (flags.is_protected ()) st->print("protected ");
  if (flags.is_public    ()) st->print("public ");


Nit: use the same order as for fields and methods.

dholmes-ora

Nothing further from me. Thanks

coleenp

This looks really good!

liach

Looks good besides an unrelated issue not for this patch.

liach · 2026-04-26T21:55:16Z

+  if (flags.is_abstract  ()) st->print("abstract ");
+  if (flags.is_annotation()) st->print("annotation ");
+  if (flags.is_enum      ()) st->print("enum ");
+  if (flags.is_synthetic ()) st->print("synthetic ");


The bit for SYNTHETIC, 0x1000, comes before ANNOTATION, 0x2000, and ENUM, 0x4000. That should be tracked in a separate issue though.

liach · 2026-04-26T21:56:48Z

+  if (flags.is_volatile ()) st->print("volatile ");
+  if (flags.is_transient()) st->print("transient ");
+  if (flags.is_enum     ()) st->print("enum ");
+  if (flags.is_synthetic()) st->print("synthetic ");


Same SYNTHETIC remark here.

liach · 2026-04-26T22:07:17Z

Created https://bugs.openjdk.org/browse/JDK-8380129 to track the synthetic flag problem instead.

removed accessflags::print_on and lifted to separate printers

9ac3afc

openjdk Bot changed the title ~~8380129~~ 8380129: Remove AccessFlags::print_on in favor of context-specific printing Apr 15, 2026

openjdk Bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Apr 15, 2026

openjdk Bot added the rfr Pull request is ready for review label Apr 15, 2026

liach reviewed Apr 15, 2026

View reviewed changes

dholmes-ora reviewed Apr 16, 2026

View reviewed changes

feedback fixes

f471984

dholmes-ora reviewed Apr 17, 2026

View reviewed changes

reorder private/public/protected in instanceklass

71b7c76

dholmes-ora approved these changes Apr 19, 2026

View reviewed changes

coleenp approved these changes Apr 23, 2026

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Apr 23, 2026

liach approved these changes Apr 26, 2026

View reviewed changes

Conversation

caspernorrbin commented Apr 15, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Apr 15, 2026

Uh oh!

openjdk Bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk Bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caspernorrbin commented Apr 16, 2026

Uh oh!

openjdk Bot commented Apr 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

caspernorrbin commented Apr 15, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Apr 15, 2026 •

edited

Loading

openjdk Bot commented Apr 15, 2026 •

edited

Loading

mlbridge Bot commented Apr 15, 2026 •

edited

Loading