8362885: A more formal way to mark javac's Flags that belong to a specific Symbol type only #26452

lahodaj · 2025-07-24T07:21:12Z

This PR proposes to improve handling of javac's Flags in two ways:

for each flag, there's now an informational annotation specifying what is the target Symbol type. Only targets right now are TypeSymbols, MethodSymbols and VarSymbols. If we ran out of flags for TypeSymbols, we could split those to module/package/class/type variable, but it does not seem to be quite necessary yet. There's an auxiliary special BLOCK, which is for JCBlock.
the manually handled Flags.Flag enum is replaced with autogenerated FlagsEnum

There may be some better to handle Flags eventually, but this hopefully improves the current situation at least somewhat, by providing more formal way to say the flags' target, and restricting the need to read comments and search for free flags.

As a side-effect of this annotation, the test/langtools/tools/javac/flags/FlagsTest.java now also prints which flags are free, for each Symbol type.

(I will remove the build label for now, until discussion on javac level is done, and will re-add it if we decide the goal to autogenerate the FlagsEnum makes sense.)

/label remove build

Progress

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

Issue

JDK-8362885: A more formal way to mark javac's Flags that belong to a specific Symbol type only (Task - P3)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) Review applies to f1d6def8
Erik Joelsson (@erikj79 - Reviewer) Review applies to f1d6def8
Magnus Ihse Bursie (@magicus - Reviewer)
Chen Liang (@liach - Reviewer)
Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26452

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…cific Symbol type only

bridgekeeper · 2025-07-24T07:21:56Z

👋 Welcome back jlahoda! 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 · 2025-07-24T07:23:05Z

@lahodaj 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:

8362885: A more formal way to mark javac's Flags that belong to a specific Symbol type only

Reviewed-by: ihse, liach, vromero, mcimadamore, erikj

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 210 new commits pushed to the master branch:

8cd7975: 8364761: (aio) AsynchronousChannelGroup.execute doesn't check null command
958383d: 8364501: Compiler shutdown crashes on access to deleted CompileTask
e9e331b: 8365238: 'jfr' feature requires 'services' with 'custom' build variant
... and 207 more: https://git.openjdk.org/jdk/compare/e6ac956a7ac613b916c0dbfda7e57856c1b8a83c...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 · 2025-07-24T07:23:43Z

@lahodaj The build label was not set.

openjdk · 2025-07-24T07:23:56Z

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

build
compiler

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.

lahodaj · 2025-07-24T07:24:00Z

/label remove build

openjdk · 2025-07-24T07:25:21Z

@lahodaj
The build label was successfully removed.

mlbridge · 2025-07-24T07:28:58Z

Webrevs

archiecobbs · 2025-07-24T15:25:33Z

This looks like a really nice improvement for maintainability.

? Would it make more sense (and/or be simpler?) to move the conflict-checking logic into FlagsGenerator?

It somehow seems odd to have a tool which can knowingly generate an invalid FlagsEnum.java file... and then defer the check for that until later, maybe, in a unit test... when we could just make the build fail instead.

? Is it worth adding some simple runtime check for correct FlagTarget usage?

For example (just brainstorming):

For each FlagTarget have FlagsGenerator output e.g. public static final long METHOD_FLAGS = 0x0301930420520017L;
At appropriate location(s) in the code, add Assert.check(this.flags_field & ~METHOD_FLAGS) == 0);

(Of course even better would be to check this at compile time using stronger typing (e.g., by replacing long flags_field with some EnumSet, etc.) but that's probably way beyond this first step.)

liach · 2025-07-24T18:12:05Z

test/langtools/tools/javac/flags/FlagsTest.java

+
+    private static void printFreeFlags(String comment, long freeFlags) {
+            System.err.print("free flags for " + comment + ": ");
+            for (int bit = 16; bit < 64; bit++) { //lowest 16 bits are used in classfiles, never suggest adding anything there


Replace 16 with Character.SIZE and 64 with Long.SIZE?

I don't think Character.SIZE is really appropriate, but I've declared U2_SIZE for 16, and used Long.SIZE for 64.

liach · 2025-07-24T18:15:04Z

make/langtools/tools/flagsgenerator/FlagsGenerator.java

+
+            try (PrintWriter out = new PrintWriter(Files.newBufferedWriter(Paths.get(args[1])))) {
+                out.println("""
+                            package com.sun.tools.javac.code;


Do we need a license header for generated sources?

CompilerProperties and other files that are generated using similar means don't have license headers, so I assume we don't need that.

jdlib · 2025-07-24T18:45:55Z

test/langtools/tools/javac/flags/FlagsTest.java

+    }
+
+    private static void verifyFlagsNonOverlapping() throws Throwable {
+        Map<FlagTarget, Map<Long, List<Field>>> target2Flag2Fields = computeTarget2Flag2Fields();


You could calculate this once in main and pass to verifyFlagNonOverlapping and findFreeFlags

Given this is a test, I would prefer to keep the test cases separate. This is a bit moot now, as the test is no doing the verification anymore.

- when conflict is detected, the generator fails - adding runtime checks - using constants for number of bits

mcimadamore · 2025-07-25T09:20:20Z

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Flags.java

    public static final int StandardFlags = 0x0fff;

    // Because the following access flags are overloaded with other
    // bit positions, we translate them when reading and writing class
    // files into unique bits positions: ACC_SYNTHETIC <-> SYNTHETIC,
    // for example.
+    @NotFlag
    public static final int ACC_SUPER    = 0x0020;


Why didn't we add annotation on these ACC ones? After all, they also overlap with previous flags -- e.g. ACC_SUPER overlaps with SYNCHRONIZED, and it works because the former goes on types and the latter on methods. So, isn't this the same trick we're pulling?

…plit of the TYPE target into CLASS/MODULE/PACKAGE/TYPE_VARIABLE.

mcimadamore · 2025-07-25T13:42:53Z

make/langtools/tools/flagsgenerator/FlagsGenerator.java

@@ -167,8 +168,11 @@ private static Object valueOfValueAttribute(AnnotationMirror am) {

    public enum FlagTarget {


Just flyby comment: symbol kind is an enum, so in principle we could use constants such as MTH, VAR, TYP as targets, if we wanted to connect the decls more with the actual code. But I like your approach as well (as symbol kind has also a lot of other more dubious stuff/noise)

lahodaj · 2025-07-25T13:46:00Z

This looks like a really nice improvement for maintainability.

? Would it make more sense (and/or be simpler?) to move the conflict-checking logic into FlagsGenerator?

It somehow seems odd to have a tool which can knowingly generate an invalid FlagsEnum.java file... and then defer the check for that until later, maybe, in a unit test... when we could just make the build fail instead.

Done.

? Is it worth adding some simple runtime check for correct FlagTarget usage?

For example (just brainstorming):
* For each `FlagTarget` have `FlagsGenerator` output e.g. `public static final long METHOD_FLAGS = 0x0301930420520017L;`

* At appropriate location(s) in the code, add `Assert.check(this.flags_field & ~METHOD_FLAGS) == 0);`

I've added the checks. It may be useful, but it is somewhat limited, as it cannot detect the most dangerous case where the flag with overloaded meaning is set incorrectly.

(Of course even better would be to check this at compile time using stronger typing (e.g., by replacing long flags_field with some EnumSet, etc.) but that's probably way beyond this first step.)

There were several experiments, in the past and currently ongoing, but it is difficult to do something that is safer and does not cost too much on performance/memory consumption. Value classes may or may not help.

mcimadamore · 2025-07-25T13:48:33Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

@@ -2127,6 +2127,9 @@ private boolean needPackageInfoClass(JCPackageDecl pd) {
    }

    public void visitModuleDef(JCModuleDecl tree) {
+        FlagsEnum.assertNoUnexpectedFlags(tree.sym.flags_field,


Not super sure about rolling this into Lower... but a separate visitor will also look a bit "too much"...

For instance, for type variables there's no way to check them at this point?

mcimadamore · 2025-07-25T14:38:18Z

There's a lot to like in this patch. Now flags cannot erroneously overlap, which will give us all javac developers more confidence that we're not stepping into each other toes. Also, by making the mechanism more formally correct, it's easier to see which "holes" we have available (and there's probably quite a lot left).

The only thing I'm unsure of, as pointed out in the review, is the dynamic checks. I believe that, as far as this PR is concerned, this feels like "scope creep". I think it would be cleaner if this PR only concerned about introducing the machinery. Then maybe we can brainstorm separately on what would be the best way to add some of these checks (but I'm skeptical that we can find anything that doesn't feel like a compromise).

For instance, another way to do this could be to do another combo test like the tree test, where we compile all the existing tests, and check the validity of all the flags on all the symbols. That's an offline way to make sure that what we do in javac is "sort of" correct, but one that doesn't leak into the code.

The real fix for this, as we know (and as @archiecobbs commented) is to move away from longs, but that's not what this PR is about.

lahodaj · 2025-07-28T15:05:43Z

/label add build

openjdk · 2025-07-28T15:07:38Z

@lahodaj
The build label was successfully added.

lahodaj · 2025-07-28T15:40:15Z

Thanks @mcimadamore for the review.

I think the build changes are now ready to be reviewed. Thanks!

liach · 2025-07-28T21:10:24Z

make/langtools/tools/flagsgenerator/FlagsGenerator.java

+                                           .map(value -> FlagTarget.valueOf(value.toString()))
+                                           .forEach(target -> target2FlagBit2Fields.computeIfAbsent(target, _ -> new HashMap<>())
+                                                                                   .computeIfAbsent(flagBit, _ -> new ArrayList<>())
+                                                                                   .add(flagName));


Instead of a dedicated loop to verify no overlaps, we can make the nested map Map<Integer, String> and fail fast whenever we detect a conflict, like:

var oldFlagName = target2FlagBit2Fields.computeIfAbsent(target, _ -> new HashMap<>()).put(flagBit, flagName); if (oldFlagName != null) { // Fail fast code }

I personally don't see a reason to collect all conflicting fields for a flag if any of these conflicts already causes a failure.

Its unlikely to be common in the long run, but there may be conflicts among more than two fields. Reporting all at once (rather than reporting one conflict, and after resolving reporting another) seems nicer.

liach · 2025-07-28T21:11:59Z

make/langtools/tools/flagsgenerator/FlagsGenerator.java

+
+            TypeElement clazz = (TypeElement) trees.getElement(new TreePath(new TreePath(cut), cut.getTypeDecls().get(0)));
+            Map<Integer, List<String>> flag2Names = new TreeMap<>();
+            Map<FlagTarget, Map<Integer, List<String>>> target2FlagBit2Fields = new HashMap<>();


Suggested change

Map<FlagTarget, Map<Integer, List<String>>> target2FlagBit2Fields = new HashMap<>();

Map<FlagTarget, Map<Integer, List<String>>> target2FlagBit2Fields = new EnumMap<>(FlagTarget.class);

erikj79 · 2025-08-01T18:13:40Z

make/modules/jdk.compiler/Gensrc.gmk

 ################################################################################
+


Can remove one of these comment delimiter lines.

make/modules/jdk.compiler/Gensrc.gmk

Co-authored-by: Magnus Ihse Bursie <[email protected]>

openjdk · 2025-08-11T09:53:49Z

⚠️ @lahodaj This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

magicus

Build changes look fine now

liach

Looks good. This should have minimal impact to javac performance.

vicente-romero-oracle

very nice!

lahodaj · 2025-08-13T08:05:09Z

/integrate

openjdk · 2025-08-13T08:07:58Z

Going to push as commit 72e22b4.
Since your change was applied there have been 230 commits pushed to the master branch:

25480f0: 8365184: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java Re-enable SerialGC flag on debuggee process
636c61a: 8365302: RISC-V: compiler/loopopts/superword/TestAlignVector.java fails when vlen=128
87d7340: 8364756: JFR: Improve slow tests
... and 227 more: https://git.openjdk.org/jdk/compare/e6ac956a7ac613b916c0dbfda7e57856c1b8a83c...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-08-13T08:08:16Z

@lahodaj Pushed as commit 72e22b4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

lahodaj added 2 commits July 23, 2025 12:07

8362885: A more formal way to mark javac's Flags that belong to a spe…

9c7007f

…cific Symbol type only

Fixing tests.

0ab2ba4

openjdk bot added the rfr Pull request is ready for review label Jul 24, 2025

openjdk bot added build [email protected] compiler [email protected] labels Jul 24, 2025

openjdk bot removed the build [email protected] label Jul 24, 2025

liach reviewed Jul 24, 2025

View reviewed changes

jdlib reviewed Jul 24, 2025

View reviewed changes

Reflecting review feedback:

5bc1d6d

- when conflict is detected, the generator fails - adding runtime checks - using constants for number of bits

mcimadamore reviewed Jul 25, 2025

View reviewed changes

lahodaj added 2 commits July 25, 2025 12:14

Review feedback: marking ACC flags with the @use annotation, forces s…

b0fbd91

…plit of the TYPE target into CLASS/MODULE/PACKAGE/TYPE_VARIABLE.

Fixing toString value for flags that have ACC_ overloaded flags.

e00209f

mcimadamore reviewed Jul 25, 2025

View reviewed changes

Reverting runtime checks, as suggested.

f1d6def

mcimadamore approved these changes Jul 28, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 28, 2025

openjdk bot added the build [email protected] label Jul 28, 2025

liach reviewed Jul 28, 2025

View reviewed changes

erikj79 approved these changes Aug 1, 2025

View reviewed changes

magicus reviewed Aug 11, 2025

View reviewed changes

make/modules/jdk.compiler/Gensrc.gmk Outdated Show resolved Hide resolved

Update make/modules/jdk.compiler/Gensrc.gmk as suggested.

019d006

Co-authored-by: Magnus Ihse Bursie <[email protected]>

openjdk bot removed the ready Pull request is ready to be integrated label Aug 11, 2025

lahodaj added 3 commits August 11, 2025 11:47

Removing unnecessary delimited, as suggested.

ab6486e

Using EnumMap, as suggested.

47ee2db

Merge remote-tracking branch 'origin/JDK-8362885' into JDK-8362885

2095f57

magicus approved these changes Aug 11, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 11, 2025

liach approved these changes Aug 11, 2025

View reviewed changes

vicente-romero-oracle approved these changes Aug 11, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 13, 2025

openjdk bot closed this Aug 13, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 13, 2025

		@@ -167,8 +168,11 @@ private static Object valueOfValueAttribute(AnnotationMirror am) {

		public enum FlagTarget {

	Map<FlagTarget, Map<Integer, List<String>>> target2FlagBit2Fields = new HashMap<>();
	Map<FlagTarget, Map<Integer, List<String>>> target2FlagBit2Fields = new EnumMap<>(FlagTarget.class);

		################################################################################

8362885: A more formal way to mark javac's Flags that belong to a specific Symbol type only #26452

8362885: A more formal way to mark javac's Flags that belong to a specific Symbol type only #26452

Conversation

lahodaj commented Jul 24, 2025 • 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 Jul 24, 2025

Uh oh!

openjdk bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 24, 2025

Uh oh!

openjdk bot commented Jul 24, 2025

Uh oh!

lahodaj commented Jul 24, 2025

Uh oh!

openjdk bot commented Jul 24, 2025

Uh oh!

mlbridge bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

archiecobbs commented Jul 24, 2025

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!

lahodaj commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore commented Jul 25, 2025

Uh oh!

lahodaj commented Jul 28, 2025

Uh oh!

openjdk bot commented Jul 28, 2025

Uh oh!

lahodaj commented Jul 28, 2025

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!

Uh oh!

openjdk bot commented Aug 11, 2025

Uh oh!

magicus 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!

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

lahodaj commented Jul 24, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 24, 2025 •

edited

Loading

mlbridge bot commented Jul 24, 2025 •

edited

Loading