openjdk
8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
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-8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage (Bug - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21616/head:pull/21616
$ git checkout pull/21616
Update a local copy of the PR:
$ git checkout pull/21616
$ git pull https://git.openjdk.org/jdk.git pull/21616/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21616
View PR using the GUI difftool:
$ git pr show -t 21616
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21616.diff
Webrev
: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](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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:
8342682: Errors related to unused code on Windows after 8339120 in dt_shmem jdwp security and jpackage
Reviewed-by: cjplummer, asemenyuk, almatvee
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 3443 new commits pushed to the master branch:
- f153e415d740f4ede272929171e9bb3e73ddbe1c: 8361253: CommandLineOptionTest library should report observed values on failure
- fba74f796eeeb42accc60ecab444c3d933b73e70: 8361306: jdk.compiler-gendata needs to depend on java.base-launchers
- 56ebb8c1b936e5a4c14486153c9f60df705095ad: 8359110: Log accumulated GC and process CPU time upon VM exit
- ... and 3440 more: https://git.openjdk.org/jdk/compare/5ca6698ba418e82ff93471fbb495759850f26f63...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.
@TheShermanTanker The following labels will be automatically applied to this pull request:
-
build -
client -
core-libs -
i18n -
net -
security -
serviceability
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.
Webrevs
- 06: Full - Incremental (53036a65)
- 05: Full - Incremental (5e9039fb)
- 04: Full - Incremental (68767cbe)
- 03: Full - Incremental (4d15fe78)
- 02: Full - Incremental (cd57fc6c)
- 01: Full - Incremental (2294b27f)
- 00: Full (e149e654)
and whatever team is responsible for HotSpot debugging.
I don't see anything hotspot related here.
I think you would be better off splitting this up into distinct issues and PRs for different areas. I expect the client team in particular would prefer that.
and whatever team is responsible for HotSpot debugging.
I don't see anything hotspot related here.
I think you would be better off splitting this up into distinct issues and PRs for different areas. I expect the client team in particular would prefer that.
Aren't the dt_shmem and jdwp changes related to HotSpot? But I guess I'll split it up in that case, I thought it might have been quicker to group it all together in one shot. This particular Pull Request I'll probably reserve for jdwp and the like, core-libs and client (And everything else) I'll move to their own PRs
Aren't the dt_shmem and jdwp changes related to HotSpot?
Nope. That's core-svc - the non-hotspot side of serviceability. :)
Aren't the dt_shmem and jdwp changes related to HotSpot?
Nope. That's core-svc - the non-hotspot side of serviceability. :)
Oh, well I guess you learn something new everyday :)
Thanks Weijun for the ok on security (Though it still makes me uneasy because it smells like a bug, but I guess it's alright). I haven't pushed yet to remove the changes to the other files in this Pull Request because the way I did it I'd have to force push and that would destroy all the review comments, I'll do that once the comments have been fully addressed
the way I did it I'd have to force push
That should not be the case. You can just anti-delta changes.
@TheShermanTanker Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.
the way I did it I'd have to force push
That should not be the case. You can just anti-delta changes.
I did it in a really inefficient way, by checking out all files except for the files that I wanted to stay. I could not figure out how to undo it. In any case, I'm sorry for the sin I just committed...
All the original labels are still there. A better approach had probably been to close the PR and open a new. You either need to do that now, or delete all the irrelevant labels. (I can't be bothered to figure out; at least build is wrong.)
/label -build
I don't know what label jpackage falls under /label -core-libs /label -i18n
I do wonder if mutex support can be implemented for Windows with Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it would be nice to have. Shame threads.h is not available with some Visual Studio versions we support, or at all with gcc. mtx_lock/unlock would be a nice solution to this problem
I do wonder if mutex support can be implemented for Windows with Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it would be nice to have. Shame threads.h is not available with some Visual Studio versions we support, or at all with gcc. mtx_lock/unlock would be a nice solution to this problem
I'll also start looking into this in the meantime, unless you explicitly want me not to do so
I do wonder if mutex support can be implemented for Windows with Acquire/ReleaseSRWLockExclusive. I know it's not strictly needed, but it would be nice to have. Shame threads.h is not available with some Visual Studio versions we support, or at all with gcc. mtx_lock/unlock would be a nice solution to this problem
I'll also start looking into this in the meantime, unless you explicitly want me not to do so
Feel free to if you'd like to. Personally I don't consider it to be that important.
I do wonder if mutex support can be implemented for Windows with Acquire/ReleaseSRWLockExclusive.
A CriticalSection is a mutex. A RWLock is not a "mutex".
I guess Weijun's comment means security is good to go, even though it's not an approval. jpackage, any comments?