8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly #15532

biboudis · 2023-09-01T10:02:20Z

The switch structure is translated in handleSwitch where we rewrite pattern matching switches. In some occasions a switch has multiple cases with multiple patterns where the n-th case does not complete normally and falls through to the n+1-st case:

switch (obj) {
   case Integer _ when ((Integer) obj) > 0:
   case String _ when !((String) obj).isEmpty():
      return 1;
   default:
      return -1;
}

This PR addresses that by translating the second case correctly and also replicates the body of the latter to the former (which we can do because no bindings are introduced in either statement blocks by the definition of unnamed variables.).

Previously the code was translated into:

switch (java.lang.runtime.SwitchBootstraps.typeSwitch(selector0$temp, index$1)) {
    case 0:
        Integer _;
        if (!(true && ((Integer)obj) > 0)) {
            index$1 = 1;
            continue;
        }

    case 1 when !((String)obj).isEmpty():
        return 1;

    default:
        return -1;
}

This PR adjusts the translation into:

switch (java.lang.runtime.SwitchBootstraps.typeSwitch(selector0$temp, index$1)) {
case 0:
    Integer _;
    if (!(true && ((Integer)obj) > 0)) {
        index$1 = 1;
        continue;
    }
    return 1;

case 1:
    String _;
    if (!((selector0$temp instanceof String || false) && (true && !((String)obj).isEmpty()))) {
        index$1 = 2;
        continue;
    }
    return 1;
}

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-8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly (Bug - P4)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to c46952b3
Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15532

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-01T10:04:05Z

👋 Welcome back abimpoudis! 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 · 2023-09-01T10:07:09Z

@biboudis 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.

…ompiled incorrectly

mlbridge · 2023-09-01T10:31:58Z

Webrevs

mcimadamore · 2023-09-01T10:51:15Z

So, as we discussed offline, type tests are normally enforced in the switch bootstrap. The first time we hit the test, we know we have some object and that the first label expects an Integer, so we test for that. But if the when clause fails, javac tells us to try again from the second label. Now, while the proposed code to the generated code looks ok - I wonder if this condition shouldn't instead be detected by the switch bootstrap: e.g. we're asking it to start again from a case which expects a String, but we really have some other Object on our hands, so that label index should never be returned by the switch bootstrap, which should either pick another applicable label index (if it exists) or just pick the default.

This would allow us to keep the generated code as is, and avoid introducing coupling between cases in the generated code.

mcimadamore · 2023-09-01T10:55:55Z

So, as we discussed offline, type tests are normally enforced in the switch bootstrap. The first time we hit the test, we know we have some object and that the first label expects an Integer, so we test for that. But if the when clause fails, javac tells us to try again from the second label. Now, while the proposed code to the generated code looks ok - I wonder if this condition shouldn't instead be detected by the switch bootstrap: e.g. we're asking it to start again from a case which expects a String, but we really have some other Object on our hands, so that label index should never be returned by the switch bootstrap, which should either pick another applicable label index (if it exists) or just pick the default.

This would allow us to keep the generated code as is, and avoid introducing coupling between cases in the generated code.

Btw, looking at the comment in SwitchBootstraps::createRepeatIndexSwitch, it seems that's already the case:

/*
     * Construct test chains for labels inside switch, to handle switch repeats:
     * switch (idx) {
     *     case 0 -> if (selector matches label[0]) return 0; else if (selector matches label[1]) return 1; else ...
     *     case 1 -> if (selector matches label[1]) return 1; else ...
     *     ...
     * }
     */

mcimadamore · 2023-09-01T11:04:34Z

So, as we discussed offline, type tests are normally enforced in the switch bootstrap. The first time we hit the test, we know we have some object and that the first label expects an Integer, so we test for that. But if the when clause fails, javac tells us to try again from the second label. Now, while the proposed code to the generated code looks ok - I wonder if this condition shouldn't instead be detected by the switch bootstrap: e.g. we're asking it to start again from a case which expects a String, but we really have some other Object on our hands, so that label index should never be returned by the switch bootstrap, which should either pick another applicable label index (if it exists) or just pick the default.
This would allow us to keep the generated code as is, and avoid introducing coupling between cases in the generated code.

Btw, looking at the comment in SwitchBootstraps::createRepeatIndexSwitch, it seems that's already the case:
/*
     * Construct test chains for labels inside switch, to handle switch repeats:
     * switch (idx) {
     *     case 0 -> if (selector matches label[0]) return 0; else if (selector matches label[1]) return 1; else ...
     *     case 1 -> if (selector matches label[1]) return 1; else ...
     *     ...
     * }
     */

Also, in the original test here:
https://bugs.openjdk.org/browse/JDK-8314226

It seems to me that the problem is slightly different - e.g. the client is passing a String, but the when clause doesn't seem to be applied by javac. Which seems to conflict with the original generated code you report above: if javac currently generates this:

 case 1 when !((String)obj).isEmpty():
        return 1;

How can javac go ahead with this case, considering that the provided string (in the original test) is empty?

I wonder if the real issue is that there is no real when guard emitted for the second case (because of javac getting confused with cascading).

mcimadamore · 2023-09-01T11:06:37Z

I wonder if the real issue is that there is no real when guard emitted for the second case (because of javac getting confused with cascading).

To be clear, IMHO the generated code should be:

switch (java.lang.runtime.SwitchBootstraps.typeSwitch(selector0$temp, index$1)) {
case 0:
    Integer _;
    if (!(true && ((Integer)obj) > 0)) {
        index$1 = 1;
        continue;
    }
    return 1;

case 1:
    String _;
    if (!(true && !((String)obj).isEmpty())) {
        index$1 = 2;
        continue;
    }
    return 1;
}

biboudis · 2023-09-01T12:18:23Z

I think you are right @mcimadamore. We do not need to generate the extraneous condition in the if. Removed and updated the PR.

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

…andleSwitch`

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

mcimadamore

Looks good!

openjdk · 2023-09-01T16:04:01Z

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

8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly

Reviewed-by: mcimadamore, 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 160 new commits pushed to the master branch:

a731a24: 8315934: RISC-V: Disable conservative fences per vendor
b3dad24: 8316021: Serial: Remove unused Generation::post_compact
f9ab115: 8316050: Use hexadecimal encoding in MemorySegment::toString
f804f86: 8314612: TestUnorderedReduction.java fails with -XX:MaxVectorSize=32 and -XX:+AlignVector
f8df754: 8311207: Cleanup for Optimization for UUID.toString
fecd2fd: 8315898: Open source swing JMenu tests
bb6b3f2: 8315761: Open source few swing JList and JMenuBar tests
2d168c5: 8313202: MutexLocker should disallow null Mutexes
36552e7: 8316123: ProblemList serviceability/dcmd/gc/RunFinalizationTest.java on AIX
fe5ef5f: 8315677: Open source few swing JFileChooser and other tests
... and 150 more: https://git.openjdk.org/jdk/compare/145d8bc1a3ef4f4fe0c10385be111838e7ed9250...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mcimadamore, @vicente-romero-oracle) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

biboudis · 2023-09-04T08:10:26Z

/integrate

openjdk · 2023-09-04T08:11:25Z

@biboudis
Your change (at version c46952b) is now ready to be sponsored by a Committer.

lahodaj · 2023-09-07T14:26:25Z

I think I'd like to ask for some more tests. Especially:

multiple cases with statements where the fall through goes from one to the other, e.g. something like:

   case Integer _ when ((Integer) obj) > 0:
   case String _ when !((String) obj).isEmpty():
       System.err.println(1);
   case null:
       System.err.println(2);
   default:    
       System.err.println(3);

cases with loop statements with break (and/or continue) to the loop. E.g.:

while (true) {break;}

instead of System.err.println(2);

break/continue, return, yield(?) going outside of the switch (probably one of them would be enough) in the duplicated section.

biboudis · 2023-09-08T07:46:24Z

Thank you Jan. Does multipleCasesWithReturn cover your third bullet? Did I understood correctly?

biboudis · 2023-09-08T20:19:04Z

Thank you @lahodaj for the improvement. This is extremely elegant and reduces the code size!!!

vicente-romero-oracle

looks sensible

vicente-romero-oracle · 2023-09-13T14:54:57Z

/sponsor

openjdk · 2023-09-13T14:57:29Z

@vicente-romero-oracle The PR has been updated since the change author (@biboudis) issued the integrate command - the author must perform this command again.

biboudis · 2023-09-13T14:59:30Z

/integrate

openjdk · 2023-09-13T15:02:15Z

@biboudis
Your change (at version d44f2de) is now ready to be sponsored by a Committer.

vicente-romero-oracle · 2023-09-13T15:06:36Z

/sponsor

openjdk · 2023-09-13T15:09:07Z

Going to push as commit 3b0a6d2.
Since your change was applied there have been 161 commits pushed to the master branch:

ff240a9: 8316087: Test SignedLoggerFinderTest.java is still failing
a731a24: 8315934: RISC-V: Disable conservative fences per vendor
b3dad24: 8316021: Serial: Remove unused Generation::post_compact
f9ab115: 8316050: Use hexadecimal encoding in MemorySegment::toString
f804f86: 8314612: TestUnorderedReduction.java fails with -XX:MaxVectorSize=32 and -XX:+AlignVector
f8df754: 8311207: Cleanup for Optimization for UUID.toString
fecd2fd: 8315898: Open source swing JMenu tests
bb6b3f2: 8315761: Open source few swing JList and JMenuBar tests
2d168c5: 8313202: MutexLocker should disallow null Mutexes
36552e7: 8316123: ProblemList serviceability/dcmd/gc/RunFinalizationTest.java on AIX
... and 151 more: https://git.openjdk.org/jdk/compare/145d8bc1a3ef4f4fe0c10385be111838e7ed9250...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-13T15:09:31Z

@vicente-romero-oracle @biboudis Pushed as commit 3b0a6d2.

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

biboudis · 2023-09-15T08:07:57Z

/backport jdk21u

openjdk · 2023-09-15T08:08:45Z

@biboudis the backport was successfully created on the branch biboudis-backport-3b0a6d2a in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 3b0a6d2a from the openjdk/jdk repository.

The commit being backported was authored by Aggelos Biboudis on 13 Sep 2023 and was reviewed by Maurizio Cimadamore and Vicente Romero.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u:

$ git fetch https://github.com/openjdk-bots/jdk21u.git biboudis-backport-3b0a6d2a:biboudis-backport-3b0a6d2a
$ git checkout biboudis-backport-3b0a6d2a
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u.git biboudis-backport-3b0a6d2a

biboudis force-pushed the fix-#8314226 branch from e7a8574 to c236a21 Compare September 1, 2023 10:04

openjdk bot added the compiler compiler-dev@openjdk.org label Sep 1, 2023

8314226: Series of colon-style fallthrough switch cases with guards c…

09db2c7

…ompiled incorrectly

biboudis force-pushed the fix-#8314226 branch from c236a21 to 09db2c7 Compare September 1, 2023 10:23

biboudis marked this pull request as ready for review September 1, 2023 10:23

openjdk bot added the rfr Pull request is ready for review label Sep 1, 2023

Address review

18f52d1

mcimadamore reviewed Sep 1, 2023

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java Outdated Show resolved Hide resolved

mcimadamore reviewed Sep 1, 2023

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java Outdated Show resolved Hide resolved

biboudis added 2 commits September 1, 2023 17:05

Perform patching in reverse order separately from the main loop in `h…

358f3de

…andleSwitch`

Cleanup

5b0bf6c

mcimadamore reviewed Sep 1, 2023

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java Outdated Show resolved Hide resolved

mcimadamore reviewed Sep 1, 2023

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java Outdated Show resolved Hide resolved

Make patchCompletingNormallyCases void returning

c46952b

mcimadamore approved these changes Sep 1, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 1, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 4, 2023

Add new tests and adjustment

52bf0e3

openjdk bot removed the sponsor Pull request is ready to be sponsored label Sep 8, 2023

Improving the desugared code. (#4)

d44f2de

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

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 13, 2023

openjdk bot added the integrated Pull request has been integrated label Sep 13, 2023

openjdk bot closed this Sep 13, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly #15532

8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly #15532

biboudis commented Sep 1, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 1, 2023

openjdk bot commented Sep 1, 2023

mlbridge bot commented Sep 1, 2023 •

edited

Loading

mcimadamore commented Sep 1, 2023

mcimadamore commented Sep 1, 2023

mcimadamore commented Sep 1, 2023

mcimadamore commented Sep 1, 2023 •

edited

Loading

biboudis commented Sep 1, 2023

mcimadamore left a comment

openjdk bot commented Sep 1, 2023 •

edited

Loading

biboudis commented Sep 4, 2023

openjdk bot commented Sep 4, 2023

lahodaj commented Sep 7, 2023

biboudis commented Sep 8, 2023

biboudis commented Sep 8, 2023

vicente-romero-oracle left a comment

vicente-romero-oracle commented Sep 13, 2023

openjdk bot commented Sep 13, 2023

biboudis commented Sep 13, 2023

openjdk bot commented Sep 13, 2023

vicente-romero-oracle commented Sep 13, 2023

openjdk bot commented Sep 13, 2023

openjdk bot commented Sep 13, 2023

biboudis commented Sep 15, 2023

openjdk bot commented Sep 15, 2023

8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly #15532

8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly #15532

Conversation

biboudis commented Sep 1, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 1, 2023

openjdk bot commented Sep 1, 2023

mlbridge bot commented Sep 1, 2023 • edited Loading

Webrevs

mcimadamore commented Sep 1, 2023

mcimadamore commented Sep 1, 2023

mcimadamore commented Sep 1, 2023

mcimadamore commented Sep 1, 2023 • edited Loading

biboudis commented Sep 1, 2023

mcimadamore left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 1, 2023 • edited Loading

biboudis commented Sep 4, 2023

openjdk bot commented Sep 4, 2023

lahodaj commented Sep 7, 2023

biboudis commented Sep 8, 2023

biboudis commented Sep 8, 2023

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

vicente-romero-oracle commented Sep 13, 2023

openjdk bot commented Sep 13, 2023

biboudis commented Sep 13, 2023

openjdk bot commented Sep 13, 2023

vicente-romero-oracle commented Sep 13, 2023

openjdk bot commented Sep 13, 2023

openjdk bot commented Sep 13, 2023

biboudis commented Sep 15, 2023

openjdk bot commented Sep 15, 2023

biboudis commented Sep 1, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 1, 2023 •

edited

Loading

mcimadamore commented Sep 1, 2023 •

edited

Loading

openjdk bot commented Sep 1, 2023 •

edited

Loading