Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8244146: javac changes for JEP 306 #3831

Closed
wants to merge 22 commits into from
Closed

8244146: javac changes for JEP 306 #3831

wants to merge 22 commits into from

Conversation

@jddarcy
Copy link
Member

@jddarcy jddarcy commented May 1, 2021


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3831/head:pull/3831
$ git checkout pull/3831

Update a local copy of the PR:
$ git checkout pull/3831
$ git pull https://git.openjdk.java.net/jdk pull/3831/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3831

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3831.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 1, 2021

👋 Welcome back darcy! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 1, 2021

@jddarcy The following label will be automatically applied to this pull request:

  • compiler

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

Loading

@openjdk openjdk bot added the compiler label May 1, 2021
@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented May 1, 2021

Preliminary implementation of JEP 306 in javac; all langtools tests pass with the changes.

To allow the existing strictfp checks to be used (no abstract strictfp methods, etc.), the strictfp-ness is kept through the compiler stages until being filtered out in ClassWriter.

Loading

@briangoetz
Copy link
Contributor

@briangoetz briangoetz commented May 3, 2021

I'd like to better understand what the future plan for VM treatment of the ACC_STRICT flag. It would be nice to have a plan to eventually reclaim this flag. Right now, it seems we're not setting the flag any more in the static compiler (and eliminating it from the reflective view?), but the VM still accepts the flag. Is there a plan for eventually warning on, and then rejecting, and then repurposing this valuable real estate?

Loading

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented May 3, 2021

I'd like to better understand what the future plan for VM treatment of the ACC_STRICT flag. It would be nice to have a plan to eventually reclaim this flag. Right now, it seems we're not setting the flag any more in the static compiler (and eliminating it from the reflective view?), but the VM still accepts the flag. Is there a plan for eventually warning on, and then rejecting, and then repurposing this valuable real estate?

For the purposes of javac and JEP 306, in the changeset the ACC_STRICT bit is no longer emitted for -source 17/--target 17. This also implies any strictfp-ness is lost from a reflective representation constructed from a class file, which includes annotation processing from a class file (as directly tested for in TestStrictfpRetention.java) and would include core reflection as well.

Some details of the VM handling of ACC_STRICT are still under discussion. A necessary condition to reclaiming the bit for other purposes is having JVM validation checks dependent on class file version since ACC_STRICT to mean strict floating-point evaluation is valid for class file versions 46.0 through 60.0. Presumably accepting ACC_STRICT for class files 46.0 through 60.0 is desirably for compatibility.

Warning of ACC_STRICT on version 61.0 and higher class files is possible.

Loading

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented May 4, 2021

/issue add 8266398

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 4, 2021

@jddarcy
Adding additional issue to issue list: 8266398: Implement JEP 306.

Loading

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented May 4, 2021

/issue remove 8266398

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 4, 2021

@jddarcy
Removing additional issue from issue list: 8266398.

Loading

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented May 4, 2021

/issue add 8266399

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 4, 2021

@jddarcy
Adding additional issue to issue list: 8266399: Core libs update for JEP 306.

Loading

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented May 4, 2021

/csr needed

Loading

@openjdk openjdk bot added the csr label May 4, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 4, 2021

@jddarcy this pull request will not be integrated until the CSR request JDK-8266524 for issue JDK-8244146 has been approved.

Loading

@jddarcy jddarcy marked this pull request as ready for review May 4, 2021
@openjdk openjdk bot added the rfr label May 4, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 4, 2021

Loading

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented May 4, 2021

/cc core-libs

Loading

@openjdk openjdk bot removed the core-libs label May 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 5, 2021

@jddarcy
The core-libs label was successfully removed.

Loading

naotoj
naotoj approved these changes May 5, 2021
@jddarcy jddarcy mentioned this pull request May 5, 2021
3 tasks
Copy link

@sadayapalam sadayapalam left a comment

Javac changes look good to me.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Joe Darcy on core-libs-dev:

On 5/4/2021 10:30 PM, Srikanth Adayapalam wrote:

On Sat, 1 May 2021 23:00:05 GMT, Joe Darcy <darcy at openjdk.org> wrote:

8244146: javac changes for JEP 306
src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java line 1704:

1702: if (Feature.REDUNDANT_STRICTFP.allowedInSource(source))
1703: result = result & ~STRICTFP;
1704:
Nitpick: Doing in Rome as ... would mean this is better written as

result &= ~STRICTFP;

to be in harmony with the code in the vicinity

Sure; I had considered writing the update that way initially.

Also I am OK with the feature-allowed-in-source-check, but wonder if it is an overkill for smaller focussed changes like this one. I will say I don't know what is the standard operating procedure. See that elsewhere in Lint.java you are doing

if (source.compareTo(Source.JDK17) >= 0) {
values.add(LintCategory.STRICTFP);
}

Lint had some other checks directly against Source version enums, but
I'm happy to change it to a Source.Feature-based check.

Thanks,

-Joe

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Joe Darcy on core-libs-dev:

On 5/4/2021 10:30 PM, Srikanth Adayapalam wrote:

On Sat, 1 May 2021 23:00:05 GMT, Joe Darcy <darcy at openjdk.org> wrote:

8244146: javac changes for JEP 306
src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java line 1704:

1702: if (Feature.REDUNDANT_STRICTFP.allowedInSource(source))
1703: result = result & ~STRICTFP;
1704:
Nitpick: Doing in Rome as ... would mean this is better written as

result &= ~STRICTFP;

to be in harmony with the code in the vicinity

Sure; I had considered writing the update that way initially.

Also I am OK with the feature-allowed-in-source-check, but wonder if it is an overkill for smaller focussed changes like this one. I will say I don't know what is the standard operating procedure. See that elsewhere in Lint.java you are doing

if (source.compareTo(Source.JDK17) >= 0) {
values.add(LintCategory.STRICTFP);
}

Lint had some other checks directly against Source version enums, but
I'm happy to change it to a Source.Feature-based check.

Thanks,

-Joe

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Joe Darcy on compiler-dev:

On 5/5/2021 4:07 AM, Jan Lahoda wrote:

On Wed, 5 May 2021 05:26:47 GMT, Srikanth Adayapalam <sadayapalam at openjdk.org> wrote:

8244146: javac changes for JEP 306
src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java line 1704:

1702: if (Feature.REDUNDANT_STRICTFP.allowedInSource(source))
1703: result = result & ~STRICTFP;
1704:
Nitpick: Doing in Rome as ... would mean this is better written as

result &= ~STRICTFP;

to be in harmony with the code in the vicinity

Also I am OK with the feature-allowed-in-source-check, but wonder if it is an overkill for smaller focussed changes like this one. I will say I don't know what is the standard operating procedure. See that elsewhere in Lint.java you are doing

if (source.compareTo(Source.JDK17) >= 0) {
values.add(LintCategory.STRICTFP);
}
IMO it is better to have an enum constant in Feature for source level changes.

But here, I wonder if a Target method on this place wouldn't be more appropriate. One can write:

javac -source 16 -targete 17 Test.java

If `Test.java` contains `strictfp`, should the classfile have `STRICTFP` set or not?

Good point on the mixed 16 source / 17 target combination Jan. I've
pushed a revision that adds a predicate to Target and updated the tests
accordingly.

Thanks,

-Joe

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Joe Darcy on compiler-dev:

On 5/5/2021 4:07 AM, Jan Lahoda wrote:

On Wed, 5 May 2021 05:26:47 GMT, Srikanth Adayapalam <sadayapalam at openjdk.org> wrote:

8244146: javac changes for JEP 306
src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassWriter.java line 1704:

1702: if (Feature.REDUNDANT_STRICTFP.allowedInSource(source))
1703: result = result & ~STRICTFP;
1704:
Nitpick: Doing in Rome as ... would mean this is better written as

result &= ~STRICTFP;

to be in harmony with the code in the vicinity

Also I am OK with the feature-allowed-in-source-check, but wonder if it is an overkill for smaller focussed changes like this one. I will say I don't know what is the standard operating procedure. See that elsewhere in Lint.java you are doing

if (source.compareTo(Source.JDK17) >= 0) {
values.add(LintCategory.STRICTFP);
}
IMO it is better to have an enum constant in Feature for source level changes.

But here, I wonder if a Target method on this place wouldn't be more appropriate. One can write:

javac -source 16 -targete 17 Test.java

If `Test.java` contains `strictfp`, should the classfile have `STRICTFP` set or not?

Good point on the mixed 16 source / 17 target combination Jan. I've
pushed a revision that adds a predicate to Target and updated the tests
accordingly.

Thanks,

-Joe

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Joe Darcy on compiler-dev:

On 5/5/2021 1:15 AM, Srikanth Adayapalam wrote:

On Sat, 1 May 2021 23:00:05 GMT, Joe Darcy <darcy at openjdk.org> wrote:

8244146: javac changes for JEP 306
Overall, looks good other than the various minor issues called out.

I wonder if the tests would have turned out to be a good bit simpler if we simply checked diagnostics against a golden file and skipped the toolbox approach. Is there a strong benefit to using the toolbox approach for the present need ??

I've certainly written golden file tests before, but my understanding
was that approach was? considered out of favor for new work. While there
are different kinds of overheads for both approaches, I do like the
all-in-one-file aspect of the toolbox.

Also, another model for tests could have been test/langtools/tools/javac//7166455/CheckACC_STRICTFlagOnclinitTest.java
rather than using an annotation processor.

(Not asking for a change, just an academic question/observation)

Having worked on annotation processors, I reach for that tool more often
than others might :-)

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Joe Darcy on compiler-dev:

On 5/5/2021 1:15 AM, Srikanth Adayapalam wrote:

On Sat, 1 May 2021 23:00:05 GMT, Joe Darcy <darcy at openjdk.org> wrote:

8244146: javac changes for JEP 306
Overall, looks good other than the various minor issues called out.

I wonder if the tests would have turned out to be a good bit simpler if we simply checked diagnostics against a golden file and skipped the toolbox approach. Is there a strong benefit to using the toolbox approach for the present need ??

I've certainly written golden file tests before, but my understanding
was that approach was? considered out of favor for new work. While there
are different kinds of overheads for both approaches, I do like the
all-in-one-file aspect of the toolbox.

Also, another model for tests could have been test/langtools/tools/javac//7166455/CheckACC_STRICTFlagOnclinitTest.java
rather than using an annotation processor.

(Not asking for a change, just an academic question/observation)

Having worked on annotation processors, I reach for that tool more often
than others might :-)

Loading

@openjdk openjdk bot removed the csr label May 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 28, 2021

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

8244146: javac changes for JEP 306
8266399: Core libs update for JEP 306

Reviewed-by: sadayapalam, bpb, naoto

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

Loading

@openjdk openjdk bot added the ready label May 28, 2021
@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Jun 1, 2021

/integrate

Loading

@openjdk openjdk bot closed this Jun 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 1, 2021

@jddarcy Pushed as commit 0ae4ceb.

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

Loading

@jddarcy jddarcy deleted the 8244146 branch Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants