8316971: Add Lint warning for restricted method calls #15964

mcimadamore · 2023-09-28T13:13:31Z

This patch adds a new lint warning category, namely -Xlint:restricted to enable warnings on restricted method calls.

The patch is relatively straightforward: javac marks methods that are marked with the @Restricted annotation with a corresponding internal flag. This is done both in Annotate when compiling JDK from source, and in ClassReader when JDK classfiles are read. When calls to methods marked with the special flag are found, a new warning is issued.

While there are some similarities between this new warning and the preview API warnings, the compiler does not emit a mandatory note when a compilation unit is found to have one or more restricted method calls. In other words, this is just a plain lint warning.

The output from javac looks as follows:

Foo.java:6: warning: [restricted] MemorySegment.reinterpret(long) is a restricted method.
      Arena.ofAuto().allocate(10).reinterpret(100);
                                 ^
  (Restricted methods are unsafe, and, if used incorrectly, they might crash the JVM or result in memory corruption)

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8317242 to be approved
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8316971: Add Lint warning for restricted method calls (Enhancement - P4)
JDK-8317242: Add Lint warning for restricted method calls (CSR)

Reviewers

Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to 8370e79d
Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to 1951b742

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15964

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

Using diff file

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

Webrev

Link to Webrev Comment

mcimadamore · 2023-09-28T13:14:11Z

make/modules/java.base/Java.gmk

@@ -23,7 +23,7 @@
 # questions.
 #

-DISABLED_WARNINGS_java += this-escape
+DISABLED_WARNINGS_java += this-escape restricted


I've disabled restricted warnings in java.base as there's a lot of restricted calls in the Linker API implementation :-)

bridgekeeper · 2023-09-28T13:14:29Z

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

mcimadamore · 2023-09-28T13:14:30Z

make/test/BuildMicrobenchmark.gmk

@@ -91,7 +91,7 @@ $(eval $(call SetupJavaCompilation, BUILD_JDK_MICROBENCHMARK, \
    TARGET_RELEASE := $(TARGET_RELEASE_NEWJDK_UPGRADED), \
    SMALL_JAVA := false, \
    CLASSPATH := $(MICROBENCHMARK_CLASSPATH), \
-    DISABLED_WARNINGS := this-escape processing rawtypes cast serial preview, \
+    DISABLED_WARNINGS := restricted this-escape processing rawtypes cast serial preview, \


This is needed so that we can compile FFM API benchmarks.

openjdk · 2023-09-28T13:16:32Z

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

build
compiler
core-libs

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.

mcimadamore · 2023-09-28T13:17:10Z

test/langtools/tools/javac/diags/examples.not-yet.txt

@@ -215,3 +215,6 @@ compiler.misc.illegal.signature                               # the compiler can

 # this one needs a forged class file to be reproduced
 compiler.err.annotation.unrecognized.attribute.name
+
+# this one is transitional (waiting for FFM API to exit preview)


Note: in principle I could have added an example for this diagnostic. In practice, the fact that FFM is still a preview API makes this a bit difficult - because we need the sample to also have enable preview flags, and also to catch a bunch of preview diagnostics (some of which I can't add directly as they are also excluded on this file). Since this PR already adds a test, I opted to exclude the diagnostic sample for now, and I will come back later (after FFM is no longer preview) to add one (so as to minimize disruption).

mlbridge · 2023-09-28T13:21:46Z

Webrevs

mcimadamore · 2023-09-28T13:33:21Z

test/langtools/tools/javac/RestrictedMethods.java

+ * @test /nodynamiccopyright/
+ * @bug 8316971
+ * @summary Smoke test for restricted method call warnings
+ * @compile/fail/ref=RestrictedMethods.out -Xlint:restricted -Werror -XDrawDiagnostics --enable-preview --source ${jdk.version} RestrictedMethods.java


The --enable-preview related options will need to be removed when FFM is finalized

magicus

Build changes look trivially fine.
/reviewers 2

openjdk · 2023-09-28T15:22:40Z

@magicus
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

vicente-romero-oracle · 2023-09-28T15:44:15Z

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

@@ -389,6 +389,11 @@ public static EnumSet<Flag> asFlagSet(long flags) {
     */
    public static final long SEALED = 1L<<62; // ClassSymbols

+    /**
+     * Flag to indicate sealed class/interface declaration.


this javadoc needs to be adjusted to restricted methods

vicente-romero-oracle · 2023-09-28T15:56:36Z

question shouldn't the new Restricted annotation be annotated with the @PreviewFeature annotation? it depends on a preview feature

vicente-romero-oracle · 2023-09-28T17:28:47Z

test/langtools/tools/javac/RestrictedMethods.java

+            MemorySegment.NULL.reinterpret(10); // no warning here
+        }
+    }
+}


shouldn't this test include a case using method references? For example:

void m(MemorySegment m) { foo(m::reinterpret); } <R> void foo(LongFunction<R> f) {}

we could also clarify in the CSR that the warning will also be issued for method references where the identifier after :: is a restricted method

Good point. Will do.

mcimadamore · 2023-09-29T08:14:29Z

question shouldn't the new Restricted annotation be annotated with the @PreviewFeature annotation? it depends on a preview feature

The Restricted annotation is an internal annotation only. So there is no value in annotating it with @PreviewFeature. Also, note that there is a PR already filed which, when integrated, will move the FFM API out of preview [1].

[1] - https://git.openjdk.org/jdk/pull/15103

mcimadamore · 2023-09-29T08:22:47Z

src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties

@@ -1917,6 +1917,11 @@ compiler.err.is.preview=\
 compiler.warn.is.preview.reflective=\
    {0} is a reflective preview API and may be removed in a future release.

+# 0: symbol, 1: symbol
+compiler.warn.restricted.method=\


I've updated the name of this key (from compiler.warn.restricted.method.call to make it more general, and also applicable in the case of method reference. Note that the warning text itself has not changed, as that was already correct.

vicente-romero-oracle · 2023-09-29T15:18:38Z

question shouldn't the new Restricted annotation be annotated with the @PreviewFeature annotation? it depends on a preview feature

The Restricted annotation is an internal annotation only. So there is no value in annotating it with @PreviewFeature. Also, note that there is a PR already filed which, when integrated, will move the FFM API out of preview [1].

[1] - https://git.openjdk.org/jdk/pull/15103

I see, thanks

vicente-romero-oracle

looks good, thanks!

openjdk · 2023-10-03T16:47:10Z

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

8316971: Add Lint warning for restricted method calls

Reviewed-by: ihse, vromero

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

b859da9: 8316696: Remove the testing base classes: IntlTest and CollatorTest
b438cff: 8314085: Fixing scope from benchmark to thread for JMH tests having shared state
ae796a4: 8316923: Add DEF_STATIC_JNI_OnLoad for librmi
89987db: 8303773: Replace "main.wrapper" with "test.thread.factory" property in test code
0e501f6: 8308429: jvmti/StopThread/stopthrd007 failed with "NoClassDefFoundError: Could not initialize class jdk.internal.misc.VirtualThreads"
c47a0ce: 8317235: Remove Access API use in nmethod class
353d139: 8317340: Windows builds are not reproducible if MS VS compiler install path differs
3bcfac1: 8317246: Cleanup java.net.URLEncoder and URLDecoder use of file.encoding property
b6a97c0: 8316880: AArch64: "stop: Header is not fast-locked" with -XX:-UseLSE since JDK-8315880
287b243: 8316893: Compile without -fno-delete-null-pointer-checks
... and 52 more: https://git.openjdk.org/jdk/compare/798125152ba40ff2d093711629f275b5d74f0bcb...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.

mcimadamore · 2023-10-04T09:36:02Z

/integrate

openjdk · 2023-10-04T09:36:55Z

Going to push as commit 0d4de8a.
Since your change was applied there have been 70 commits pushed to the master branch:

d4c904d: 8317294: Classloading throws exceptions over already pending exceptions
48f1a92: 8316679: C2 SuperWord: wrong result, load should not be moved before store if not comparable
0b0f8b5: 8219652: [aix] Tests failing with JNI attach problems.
8c0d026: 8315042: NPE in PKCS7.parseOldSignedData
f7deaf4: 8316778: test hprof lib: invalid array element type from JavaValueArray.elementSize
8ff10a0: 8317446: ProblemList gc/arguments/TestNewSizeFlags.java on macosx-aarch64 in Xcomp
1809b8c: 8317265: ListFormat::format specification could be made clearer regarding handling IllegalArgumentException.
cfabcbf: 8317121: vector_masked_load instruction is moved too early after JDK-8286941
b859da9: 8316696: Remove the testing base classes: IntlTest and CollatorTest
b438cff: 8314085: Fixing scope from benchmark to thread for JMH tests having shared state
... and 60 more: https://git.openjdk.org/jdk/compare/798125152ba40ff2d093711629f275b5d74f0bcb...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-04T09:37:03Z

@mcimadamore Pushed as commit 0d4de8a.

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

mcimadamore added 2 commits September 28, 2023 13:13

Initial push

675cbb3

Add tests

8370e79

mcimadamore commented Sep 28, 2023

View reviewed changes

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Sep 28, 2023

openjdk bot added build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Sep 28, 2023

mcimadamore commented Sep 28, 2023

View reviewed changes

magicus approved these changes Sep 28, 2023

View reviewed changes

Update warning message

21b7d86

vicente-romero-oracle reviewed Sep 28, 2023

View reviewed changes

Address review comments

1951b74

mcimadamore commented Sep 29, 2023

View reviewed changes

vicente-romero-oracle approved these changes Sep 29, 2023

View reviewed changes

Add comment in jdk.compiler's module-info

1751bef

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Oct 3, 2023

openjdk bot added the integrated Pull request has been integrated label Oct 4, 2023

openjdk bot closed this Oct 4, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 4, 2023

lukellmann mentioned this pull request Feb 5, 2024

JDK-8316972: Add javadoc support for restricted methods #16188

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8316971: Add Lint warning for restricted method calls #15964

8316971: Add Lint warning for restricted method calls #15964

mcimadamore commented Sep 28, 2023 •

edited by openjdk bot

Loading

mcimadamore Sep 28, 2023

bridgekeeper bot commented Sep 28, 2023

mcimadamore Sep 28, 2023

openjdk bot commented Sep 28, 2023

mcimadamore Sep 28, 2023

mlbridge bot commented Sep 28, 2023 •

edited

Loading

mcimadamore Sep 28, 2023 •

edited

Loading

magicus left a comment

openjdk bot commented Sep 28, 2023

vicente-romero-oracle Sep 28, 2023

vicente-romero-oracle commented Sep 28, 2023

vicente-romero-oracle Sep 28, 2023 •

edited

Loading

mcimadamore Sep 29, 2023

mcimadamore commented Sep 29, 2023

mcimadamore Sep 29, 2023

vicente-romero-oracle commented Sep 29, 2023

vicente-romero-oracle left a comment

openjdk bot commented Oct 3, 2023

mcimadamore commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

8316971: Add Lint warning for restricted method calls #15964

8316971: Add Lint warning for restricted method calls #15964

Conversation

mcimadamore commented Sep 28, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

mcimadamore Sep 28, 2023

Choose a reason for hiding this comment

bridgekeeper bot commented Sep 28, 2023

mcimadamore Sep 28, 2023

Choose a reason for hiding this comment

openjdk bot commented Sep 28, 2023

mcimadamore Sep 28, 2023

Choose a reason for hiding this comment

mlbridge bot commented Sep 28, 2023 • edited Loading

Webrevs

mcimadamore Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

magicus left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 28, 2023

vicente-romero-oracle Sep 28, 2023

Choose a reason for hiding this comment

vicente-romero-oracle commented Sep 28, 2023

vicente-romero-oracle Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

mcimadamore Sep 29, 2023

Choose a reason for hiding this comment

mcimadamore commented Sep 29, 2023

mcimadamore Sep 29, 2023

Choose a reason for hiding this comment

vicente-romero-oracle commented Sep 29, 2023

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 3, 2023

mcimadamore commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

openjdk bot commented Oct 4, 2023

mcimadamore commented Sep 28, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 28, 2023 •

edited

Loading

mcimadamore Sep 28, 2023 •

edited

Loading

vicente-romero-oracle Sep 28, 2023 •

edited

Loading