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

TheShermanTanker · 2024-10-23T04:40:50Z

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)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

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

Reviewers

Naoto Sato (@naotoj - Reviewer)
Justin Lu (@justin-curtis-lu - Committer)

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-23T04:41:43Z

👋 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.

openjdk · 2024-10-23T04:42:09Z

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

08b1fa4: 8359972: Problem list TestStaticCallStub until JDK-8359963 is fixed
23e1e2f: 8359180: Apply java.io.Serial annotations in java.instrument
5a62e99: 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 · 2024-10-23T04:42:42Z

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

mlbridge · 2024-10-23T04:45:41Z

Webrevs

01: Full - Incremental (6c74ac76)
00: Full (d2557517)

TheShermanTanker · 2024-10-30T05:27:35Z

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

dholmes-ora · 2024-10-30T05:51:30Z

src/java.base/windows/native/libjava/HostLocaleProviderAdapter_md.c

+/*

Don't understand this one at all - what did gcc actually complain about here? This code all seems used.

The warning received is that got is set but not used - I had a look at getLocaleInfoWrapper and it seems like it has no side effects, its only purpose is to return a value, which is the variable got in this case. I'm pretty certain got not being used is a bug, and that it is needed somewhere, but someone forgot to hook it up to whatever needs it

It does seem a bug, where the return value is not handled correctly. Can you please file a separate bug?

I had a look at getLocaleInfoWrapper and it seems like it has no side effects, its only purpose is to return a value

While got is not needed here, I am not convinced that getLocaleInfoWrapper is not used, so we shouldn't comment out all of the code, but rather just remove got. getLocaleInfoWrapper updates the outbound variable pattern (see https://learn.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getlocaleinfoex), which is used to get the right string pattern from fixes.

Usually, based on got, appropriate action is taken, but that's not needed in this case, because pattern is either updated or remains its fallback 0 value. So I presume that is why got is ignored in this case.

We could just add that got removal to this patch, what do you think @naotoj?

Thanks, Justin. That is correct. Only pattern is relevant here. No need to check got so it can safely be removed.

dholmes-ora · 2024-10-30T05:53:06Z

src/java.base/share/native/libzip/zip_util.c

Not my area but this fix looks reasonable.

dholmes-ora · 2024-10-30T05:58:25Z

src/java.base/windows/native/libjava/TimeZone_md.c

@@ -232,7 +232,7 @@ static int getWinTimeZone(char *winZoneName, size_t winZoneNameBufSize)
        WCHAR stdNameInReg[MAX_ZONE_CHAR];
        TziValue tempTzi;
        WCHAR *stdNamePtr = tzi.StandardName;
-        int onlyMapID;
+     // int onlyMapID;


Looks like this became unused with JDK-8209167, so deleting it seems fine to me.

dholmes-ora · 2024-10-30T06:01:05Z

src/java.base/windows/native/libnet/NTLMAuthSequence.c

@@ -47,7 +47,7 @@ static jfieldID ntlm_ctxHandleID;
 static jfieldID ntlm_crdHandleID;
 static jfieldID status_seqCompleteID;

-static HINSTANCE lib = NULL;
+// static HINSTANCE lib = NULL;


Looks like this became unused with JDK-7030256, so removal seems fine.

justin-curtis-lu

Locale and TimeZone files LGTM. I also think its safe to just delete the offending onlyMapID and got (as opposed to commenting), since there is no bug and they are simply unusued.

TheShermanTanker · 2024-11-04T06:02:41Z

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 :(

bridgekeeper · 2024-12-02T06:16:52Z

@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!

TheShermanTanker · 2024-12-02T12:03:05Z

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

bridgekeeper · 2024-12-30T13:08:52Z

@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!

TheShermanTanker · 2024-12-30T13:43:30Z

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

bridgekeeper · 2025-01-27T19:40:13Z

@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!

TheShermanTanker · 2025-01-28T06:19:13Z

Keep open please

bridgekeeper · 2025-02-25T08:28:46Z

@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!

TheShermanTanker · 2025-02-25T08:40:23Z

Keep open please

bridgekeeper · 2025-03-25T10:47:02Z

@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!

TheShermanTanker · 2025-03-25T11:31:24Z

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

bridgekeeper · 2025-04-22T18:17:24Z

@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!

TheShermanTanker · 2025-04-23T06:16:14Z

Stay open

bridgekeeper · 2025-05-21T09:35:26Z

@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!

TheShermanTanker · 2025-05-21T11:01:51Z

/touch

openjdk · 2025-05-21T11:02:45Z

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

bridgekeeper · 2025-06-18T17:45:06Z

@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!

TheShermanTanker · 2025-06-19T07:18:09Z

/touch

openjdk · 2025-06-19T07:19:28Z

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

8342868

d255751

openjdk bot changed the title ~~8342868~~ 8342868: Errors related to unused code on Windows after 8339120 in core libs Oct 23, 2024

openjdk bot added the rfr Pull request is ready for review label Oct 23, 2024

openjdk bot added core-libs [email protected] net [email protected] i18n [email protected] labels Oct 23, 2024

dholmes-ora reviewed Oct 30, 2024

View reviewed changes

Remove the got local

6c74ac7

naotoj approved these changes Oct 31, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 31, 2024

justin-curtis-lu approved these changes Oct 31, 2024

View reviewed changes

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

Are you sure you want to change the base?

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

Conversation

TheShermanTanker commented Oct 23, 2024 • 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 Oct 23, 2024

Uh oh!

openjdk bot commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 23, 2024

Uh oh!

mlbridge bot commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TheShermanTanker commented Oct 30, 2024

Uh oh!

dholmes-ora Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

TheShermanTanker Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

naotoj Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

justin-curtis-lu Oct 30, 2024 • 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!

naotoj Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

justin-curtis-lu left a comment

Choose a reason for hiding this comment

Uh oh!

TheShermanTanker commented Nov 4, 2024

Uh oh!

bridgekeeper bot commented Dec 2, 2024

Uh oh!

TheShermanTanker commented Dec 2, 2024

Uh oh!

bridgekeeper bot commented Dec 30, 2024

Uh oh!

TheShermanTanker commented Dec 30, 2024

Uh oh!

bridgekeeper bot commented Jan 27, 2025

Uh oh!

TheShermanTanker commented Jan 28, 2025

Uh oh!

bridgekeeper bot commented Feb 25, 2025

Uh oh!

TheShermanTanker commented Feb 25, 2025

Uh oh!

bridgekeeper bot commented Mar 25, 2025

Uh oh!

TheShermanTanker commented Mar 25, 2025

Uh oh!

bridgekeeper bot commented Apr 22, 2025

Uh oh!

TheShermanTanker commented Apr 23, 2025

Uh oh!

bridgekeeper bot commented May 21, 2025

Uh oh!

TheShermanTanker commented Oct 23, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 23, 2024 •

edited

Loading

mlbridge bot commented Oct 23, 2024 •

edited

Loading

justin-curtis-lu Oct 30, 2024 •

edited

Loading