8371418: Methods in AdapterHandlerLibrary use HashtableBase iterate method incorrectly

[ashu-mehra]

The closure passed to HashTable::iterate in AdapterHandlerLibrary::contains and AdapterHandlerLibrary::print_handler_on is returning incorrect value. If the search is successful, it should return false to terminate the iteration, but it is returning true. This patch fixes the return value of these closures.
In addition, I noticed CompactHashTable::iterate goes through all entries unconditionally, which is not optimal for cases where we may want to terminate the iteration when some condition is met. This is the case in AdapterHandlerLibrary::contains and AdapterHandlerLibrary::print_handler_on when it iterates over _aot_adapter_handler_table. This patch updates CompactHashTable::iterate to be the same as HashTAble::iterate by using return value of the closure to determine if the iteration should continue or abort. It also adds CompactHashTable::iterate_all to iterate all the values unconditionally and the users of CompactHashTable::iterate are updated to use CompactHashTable::iterate_all.

---

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-8371418: Methods in AdapterHandlerLibrary use HashtableBase iterate method incorrectly (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28197

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back asmehra! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@ashu-mehra The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (f46d3dae)
• 00: Full (b827e012)

[vnkozlov]

Add comment explaining when it is exiting, when iteration is interrupted to show difference from iterate_all()

[ashu-mehra]

Done

[vnkozlov]

Why do you need to pass AdapterFingerPrint* key argument which is not used here?

[ashu-mehra]

This is because the function passed to HashTable::iterate is called with both key and value:

jdk/src/hotspot/share/utilities/hashTable.hpp

Line 272 in c865644

bool cont = function(node->_key, node->_value);

[vnkozlov]

Thank you for pointing this.

[vnkozlov]

Good. I submitted testing.

[vnkozlov]

@ashu-mehra, do you know what issue current code (before these changes) could cause?

[ashu-mehra]

do you know what issue current code (before these changes) could cause?

AdapterHandlerLibrary::print_handler_on is called from os::print_location() -> codeBlob::dump_for_addr().
os::print_location() is only used in error reporting to print the location of the address.
With the current code (before this patch) AdapterHandlerLibrary::print_handler_on could have returned without printing anything, even if the CodeBlob passed as parameter is of type AdapterBlob. In debug builds it could also trigger the assert:

assert(found, "Should have found handler");

btw the change that introduced this bug was made more than 3 years ago in https://bugs.openjdk.org/browse/JDK-8292384 and it went in JDK 20.

[ashu-mehra]

@vnkozlov fyi - I also opened https://bugs.openjdk.org/browse/JDK-8371493 which is going to touch the same code as this patch. I didn't includes the changes in this patch to make it easier to backport this patch if needed.

[vnkozlov]

@vnkozlov fyi - I also opened https://bugs.openjdk.org/browse/JDK-8371493 which is going to touch the same code as this patch. I didn't includes the changes in this patch to make it easier to backport this patch if needed.

Good. We usually don't port enhancement but we can consider it since its simplification of printing code which should not affect code execution.