8286397: Address possibly lossy conversions in jdk.hotspot.agent #9280
Conversation
Applied required casts in jdk.hotspot.agent for the upcoming warning. Verified by cherry-picking @asotona's patch.
|
👋 Welcome back rjernst! A progress list of the required criteria for merging this PR into |
Webrevs
|
| @@ -537,7 +537,7 @@ private int calculateClassDumpRecordSize(Klass k) { | |||
| List<Field> instanceFields = new ArrayList<>(); | |||
| Iterator<Field> itr = null; | |||
| // loader + signer + protectionDomain + 2 reserved + fieldSize + cpool entris number | |||
| size += OBJ_ID_SIZE * 5 + INT_SIZE + SHORT_SIZE; | |||
| size += (int) (OBJ_ID_SIZE * 5 + INT_SIZE + SHORT_SIZE); | |||
There was a problem hiding this comment.
Why are INT_SIZE and SHORT_SIZE longs? It seems that making them int (along with all the other related _SIZE fields) is the proper fix for this and the changes below.
There was a problem hiding this comment.
These come from ObjectHeap. I do agree it would be better to change these upstream (all the type sizes there are currently long), but that would be a much more invasive change. I'm happy to try that, but it looked like a can of worms.
There was a problem hiding this comment.
Unfortunately that eventually leads to Type.getSize(), which returns a long. I don't think you can change it to return an int, because some types (nonscalar) could need a long to fit their size, so you do need an int cast some point later. Probably the cast should be in the following code since we know all of these fit an in int:
public ObjectHeap(TypeDataBase db) throws WrongTypeException {
// Get commonly used sizes of basic types
oopSize = VM.getVM().getOopSize();
byteSize = db.getJByteType().getSize();
charSize = db.getJCharType().getSize();
booleanSize = db.getJBooleanType().getSize();
intSize = db.getJIntType().getSize();
shortSize = db.getJShortType().getSize();
longSize = db.getJLongType().getSize();
floatSize = db.getJFloatType().getSize();
doubleSize = db.getJDoubleType().getSize();
}
There was a problem hiding this comment.
I pushed 8115af5 to do as you suggested. That cleaned up most of the casts in HeapHprofBinWriter.
|
@rjernst 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: 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 60 new commits pushed to the
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 (@plummercj, @ChrisHegarty) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
/sponsor |
|
Going to push as commit 7b5bd25.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@ChrisHegarty @rjernst Pushed as commit 7b5bd25. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Applied required casts in jdk.hotspot.agent for the upcoming warning.
Verified by cherry-picking @asotona's patch.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9280/head:pull/9280$ git checkout pull/9280Update a local copy of the PR:
$ git checkout pull/9280$ git pull https://git.openjdk.org/jdk pull/9280/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9280View PR using the GUI difftool:
$ git pr show -t 9280Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9280.diff