jdk icon indicating copy to clipboard operation
jdk copied to clipboard

8342868: Errors related to unused code on Windows after 8339120 in core libs

Open TheShermanTanker opened this issue 1 year ago • 4 comments

After 8339120, gcc began catching many different instances of unused code in the Windows specific codebase. Some of these seem to be bugs. I've taken the effort to mark out all the relevant globals and locals that trigger the unused warnings and addressed all of them by commenting out the code as appropriate. I am confident that in many cases this simplistic approach of commenting out code does not fix the underlying issue, and the warning actually found a bug that should be fixed. In these instances, I will be aiming to fix these bugs with help from reviewers, so I recommend anyone reviewing who knows more about the code than I do to see whether there is indeed a bug that needs fixing in a different way than what I did


Progress

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

Issue

  • JDK-8342868: Errors related to unused code on Windows after 8339120 in core libs (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21654

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

Using diff file

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

Webrev

Link to Webrev Comment

TheShermanTanker avatar Oct 23 '24 04:10 TheShermanTanker

:wave: Welcome back jwaters! 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.

bridgekeeper[bot] avatar Oct 23 '24 04:10 bridgekeeper[bot]

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

8342868: Errors related to unused code on Windows after 8339120 in core libs

Reviewed-by: naoto, jlu

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

  • 08b1fa4cb39e56497052e3ee13e679c8734cf7c5: 8359972: Problem list TestStaticCallStub until JDK-8359963 is fixed
  • 23e1e2ff4a4a75ec268c7925fb98d6b96a01bbcf: 8359180: Apply java.io.Serial annotations in java.instrument
  • 5a62e99523904e89caf561d4c1068c1565a97450: 8357686: Remove unnecessary Map.get from AWTAutoShutdown.unregisterPeer
  • ... and 3563 more: https://git.openjdk.org/jdk/compare/07f550b85a3910edd28d8761e2adfb8d6a1352f6...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[bot] avatar Oct 23 '24 04:10 openjdk[bot]

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

  • core-libs
  • i18n
  • net

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.

openjdk[bot] avatar Oct 23 '24 04:10 openjdk[bot]

Webrevs

mlbridge[bot] avatar Oct 23 '24 04:10 mlbridge[bot]

Bumping, please advise on whether the fixes are correct or not

TheShermanTanker avatar Oct 30 '24 05:10 TheShermanTanker

Thanks for the reviews! I would delete them entirely, but I don't want to invalidate the existing reviews and force you guys to re-approve again :(

TheShermanTanker avatar Nov 04 '24 06:11 TheShermanTanker

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Dec 02 '24 06:12 bridgekeeper[bot]

Keep it open Skara, I'm waiting for the other Pull Requests to be approved so I can do them all at once

TheShermanTanker avatar Dec 02 '24 12:12 TheShermanTanker

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Dec 30 '24 13:12 bridgekeeper[bot]

Stop asking, I still need awt and accessibility to be approved :(

TheShermanTanker avatar Dec 30 '24 13:12 TheShermanTanker

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Jan 27 '25 19:01 bridgekeeper[bot]

Keep open please

TheShermanTanker avatar Jan 28 '25 06:01 TheShermanTanker

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Feb 25 '25 08:02 bridgekeeper[bot]

Keep open please

TheShermanTanker avatar Feb 25 '25 08:02 TheShermanTanker

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Mar 25 '25 10:03 bridgekeeper[bot]

Keep it open please. Life is not being kind to me at the moment, I apologize for the delays

TheShermanTanker avatar Mar 25 '25 11:03 TheShermanTanker

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Apr 22 '25 18:04 bridgekeeper[bot]

Stay open

TheShermanTanker avatar Apr 23 '25 06:04 TheShermanTanker

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar May 21 '25 09:05 bridgekeeper[bot]

/touch

TheShermanTanker avatar May 21 '25 11:05 TheShermanTanker

@TheShermanTanker The pull request is being re-evaluated and the inactivity timeout has been reset.

openjdk[bot] avatar May 21 '25 11:05 openjdk[bot]

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Jun 18 '25 17:06 bridgekeeper[bot]

/touch

TheShermanTanker avatar Jun 19 '25 07:06 TheShermanTanker

@TheShermanTanker The pull request is being re-evaluated and the inactivity timeout has been reset.

openjdk[bot] avatar Jun 19 '25 07:06 openjdk[bot]

Hello @TheShermanTanker, I see that this PR is marked as ready for integration since several months. But there's also your comment which says:

I still need awt and accessibility to be approved

I don't see any client area changes in this PR. Is there anything that's needed to be done to move forward with integrating this? I don't mean that you should integrate it "now" - I see that this branch hasn't been merged against master branch for several months, so that would be the first thing to do.

jaikiran avatar Jun 27 '25 13:06 jaikiran

Sorry for waiting so long. It's become clear that I won't be able to get awt and accessibility up to speed for a long time, so I will go ahead with this one first

TheShermanTanker avatar Jul 08 '25 01:07 TheShermanTanker

/integrate

TheShermanTanker avatar Jul 08 '25 01:07 TheShermanTanker

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

  • 563a3358f6f1ecff816318cbb32376487365c1fa: 8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
  • ec7c6be6a9e84c8cd2077fea07930592ddd13669: 8359388: Stricter checking for cipher transformations
  • 197fde5363e314de7cd6090ecd77521f3a90c56d: 8361303: L10n comment for javac.opt.Xlint.desc.synchronization in javac.properties
  • ... and 3759 more: https://git.openjdk.org/jdk/compare/07f550b85a3910edd28d8761e2adfb8d6a1352f6...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Jul 08 '25 01:07 openjdk[bot]

@TheShermanTanker Pushed as commit bbc5c98b144014a0423d666f74c4a5a15b08a7c2.

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

openjdk[bot] avatar Jul 08 '25 01:07 openjdk[bot]

@TheShermanTanker the commented out code really should have been deleted, not just left commented out. Please file anpther JBS issue to have this cleaned up so it is not forgotten. Thanks.

dholmes-ora avatar Jul 08 '25 01:07 dholmes-ora