8297284: ResolutionErrorTable's key is wrong #11244
Conversation
|
👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into |
coleenp
changed the title
Webrevs
|
| } | ||
|
|
||
| static bool equals(const ResolutionErrorKey& l, const ResolutionErrorKey& r) { | ||
| return l._cpool == r._cpool && l._index == r._index; |
There was a problem hiding this comment.
This should work correctly, but maybe use parentheses here for readability?
|
Thanks Matias! |
coleenp
changed the title
| // This prevents issues where the cpCache index is the same as the constant pool | ||
| // index of another entry in the table. | ||
| static const int CPCACHE_INDEX_MANGLE_VALUE = 1000000; | ||
|
|
There was a problem hiding this comment.
Do we still need CPCACHE_INDEX_MANGLE_VALUE and encode_cpcache_index(int index)? The comment around that function says that index is negative, so it shouldn't be confused with regular cp indices which are non-negative.
There was a problem hiding this comment.
I wanted to leave that in so that there's an assert that it's negative first. The whole cpCache index thing is something that @matias9927 is trying to rationalize with his ResolvedIndyInfo array - should we continue to use negative indexes in the bytecode stream or not? We think not so he'll clean this part up, and if we do we'll need some encoding like what's in CPCACHE_INDEX_MANGLE_VALUE. Maybe the mangle value should be negative and he can change his positive indexes to negative to distinguish them from constant pool indexes.
There was a problem hiding this comment.
I wanted to leave that in so that there's an assert that it's negative first. The whole cpCache index thing is something that @matias9927 is trying to rationalize with his ResolvedIndyInfo array - should we continue to use negative indexes in the bytecode stream or not? We think not so he'll clean this part up, and if we do we'll need some encoding like what's in CPCACHE_INDEX_MANGLE_VALUE. Maybe the mangle value should be negative and he can change his positive indexes to negative to distinguish them from constant pool indexes.
OK, we can revisit this later.
|
@coleenp 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 161 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. ➡️ To integrate this PR with the above commit message to the |
|
Thank you Matias and Ioi! |
|
Going to push as commit 301cf52.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
The resolution error table had a trick for the lookup key of adding constant pool index into constant pool address because that key was assumed to be in the range of the constant pool, so unique. For failed indy resolution, we also use the resolution error table and the index is not relative to the constant pool, so the key is potentially not unique. Created a Key class for the table and moved the cpool and index into that. Now the ResolutionErrorEntry just has the messages.
Tested tier1-4.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11244/head:pull/11244$ git checkout pull/11244Update a local copy of the PR:
$ git checkout pull/11244$ git pull https://git.openjdk.org/jdk pull/11244/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11244View PR using the GUI difftool:
$ git pr show -t 11244Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11244.diff