8341778: Some javac tests ignore the result of JavacTask::call #25645

vicente-romero-oracle · 2025-06-04T21:02:04Z

Several tests are ignoring the result of invoking com.sun.source.util.JavacTask::call which returns a Boolean. This implies that tests could seem to pass when in reality they are silently failing. This PR is fixing this issue by checking, in all applicable cases the result of the invocation.

TIA

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-8341778: Some javac tests ignore the result of JavacTask::call (Bug - P3)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25645

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-04T21:03:03Z

👋 Welcome back vromero! 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-06-04T21:03:05Z

@vicente-romero-oracle 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:

8341778: Some javac tests ignore the result of JavacTask::call

Reviewed-by: shade

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

af87035: 8355746: Start of release updates for JDK 26
c59e44a: 8357914: TestEmptyBootstrapMethodsAttr.java fails when run with TEST_THREAD_FACTORY=Virtual
23f1d4f: 8337666: AArch64: SHA3 GPR intrinsic
... and 182 more: https://git.openjdk.org/jdk/compare/b2a61a9972493d67d0f1a9f3f529c11e45838d5b...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-06-04T21:03:33Z

@vicente-romero-oracle The following labels will be automatically applied to this pull request:

compiler
hotspot-gc

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 · 2025-06-04T21:06:02Z

Webrevs

00: Full (4c775a19)

shipilev

Looks good, but I have question about hunks that are not about wrapping call()-s. Those look like fixing the actual test bugs?

shipilev · 2025-06-05T06:09:34Z

test/langtools/tools/javac/T6361619.java

@@ -50,7 +50,8 @@ public static void main(String... args) throws Throwable {

        final PrintWriter out = new PrintWriter(System.err, true);

-        Iterable<String> flags = Arrays.asList("-processorpath", testClassDir,
+        Iterable<String> flags = Arrays.asList("--add-exports", "jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED",


Why --add-exports? Did this test actually failed without us noticing?

correct, as we were ignoring the result of the ::call invocation it was returning false but the test went on and seemed to be OK

shipilev · 2025-06-05T06:16:48Z

test/langtools/tools/javac/api/8007344/Test.java

@@ -75,6 +76,12 @@ public static void main(String... args) throws Exception {
        }
    }

+    static final List<String> OPTIONS = List.of(


Same question here...

same answer here

shipilev · 2025-06-05T06:17:11Z

test/langtools/tools/javac/patterns/SOEDeeplyNestedBlocksTest.java

@@ -34,7 +34,7 @@

 public class SOEDeeplyNestedBlocksTest {

-    static final int NESTING_DEPTH = 1000;
+    static final int NESTING_DEPTH = 500;


Same question here.

With a NESTING_DEPTH of 1000 javac will run into a StackOverflowError, but it still completes and task.call() returns false. Using 500 javac can compile without error. Not sure what this test wants to prove. If it wants to show that javac can complete without throwing an SOE then it should keep depth 1000 and test that task.call() returns false.

yes this one was failing quietly, this test is stressing a code that was modified due to pattern matching. It is testing that javac can deal with deeply nested code without throwing SOE. 1000 depth was too much and the test was quietly failing

It is testing that javac can deal with deeply nested code without throwing SOE.

The current summary Javac fails with StackOverflowError when compiling deeply nested synchronized blocks is confusing and I don't know how it relates to your statement. Does it describe a bug or does it state the purpose of the test? Anyway, why not test with nesting levels of 500 and 1000, assert that task.call() returns true (500) and false (1000), but in the later case does not panic and terminate with a SOE.

the original version of the test was included as part of the fix for [1] as you can see in the related PR [2] method com.sun.tools.javac.jvm.Gen::visitBlock was visiting every block assuming that every one of them had pattern matching expressions in it. This implied a visitBlock with a higher number of local variables even for 90-98% of blocks that didn't have any pattern matching in them. So the test is checking that javac is not failing with SOE while generating code for a method with deeply nested blocks. Testing that javac will produce SOE after a given threshold is passed adds no information as there always be resource limits for any process. This is why I don't see the value of having a test that passes that threshold, which by the way could move over time and then we will need to go back and fix the test. While on the other hand having a test that will keep javac in check, as in being able to still compile without issues a nested enough code, seems more valuable to me.

[1] https://bugs.openjdk.org/browse/JDK-8322992
[2] #18832

shipilev · 2025-06-05T14:55:24Z

I believe we are still not in RDP1? Do you want to get it in JDK 25? Put Fix-Versions: 25 into the bug and integrate, if so.

vicente-romero-oracle · 2025-06-05T15:44:01Z

I believe we are still not in RDP1? Do you want to get it in JDK 25? Put Fix-Versions: 25 into the bug and integrate, if so.

I prefer to wait until 26, this is not a high priority issue

vnkozlov · 2025-06-05T15:47:54Z

Tests P1-P5 can be fixed during RDP1 and RDP2: https://openjdk.org/jeps/3#rdp-2

vicente-romero-oracle · 2025-06-06T14:10:01Z

thanks for the reviews

vicente-romero-oracle · 2025-06-06T14:10:07Z

/integrate

openjdk · 2025-06-06T14:11:30Z

Going to push as commit 8adb052.
Since your change was applied there have been 198 commits pushed to the master branch:

9658cec: 8358685: [TEST] AOTLoggingTag.java failed with missing log message
b2e7cda: 8358171: Additional code coverage for PEM API
65fda5c: 8358593: Add ucontext accessors for *BSD on Aarch64
... and 195 more: https://git.openjdk.org/jdk/compare/b2a61a9972493d67d0f1a9f3f529c11e45838d5b...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-06-06T14:11:38Z

@vicente-romero-oracle Pushed as commit 8adb052.

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

8341778: Some javac tests ignore the result of task.call()

4c775a1

openjdk bot added the rfr label Jun 4, 2025

openjdk bot added compiler hotspot-gc labels Jun 4, 2025

shipilev reviewed Jun 5, 2025

View reviewed changes

shipilev approved these changes Jun 5, 2025

View reviewed changes

openjdk bot added the ready label Jun 5, 2025

openjdk bot added the integrated label Jun 6, 2025

openjdk bot closed this Jun 6, 2025

openjdk bot removed ready rfr labels Jun 6, 2025

vicente-romero-oracle deleted the JDK-8341778 branch June 6, 2025 14:11

8341778: Some javac tests ignore the result of JavacTask::call #25645

8341778: Some javac tests ignore the result of JavacTask::call #25645

Uh oh!

Conversation

vicente-romero-oracle commented Jun 4, 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 Jun 4, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Jun 4, 2025

Uh oh!

mlbridge bot commented Jun 4, 2025

Webrevs

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

shipilev Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shipilev Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

shipilev Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

jdlib Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

jdlib Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shipilev commented Jun 5, 2025

Uh oh!

vicente-romero-oracle commented Jun 5, 2025

Uh oh!

vnkozlov commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vicente-romero-oracle commented Jun 6, 2025

Uh oh!

vicente-romero-oracle commented Jun 6, 2025

Uh oh!

openjdk bot commented Jun 6, 2025

Uh oh!

openjdk bot commented Jun 6, 2025

Uh oh!

Uh oh!

vicente-romero-oracle commented Jun 4, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 4, 2025 •

edited

Loading

vicente-romero-oracle Jun 5, 2025 •

edited

Loading

jdlib Jun 5, 2025 •

edited

Loading

vicente-romero-oracle Jun 5, 2025 •

edited

Loading

vnkozlov commented Jun 5, 2025 •

edited

Loading