8382485: ArrayIndexOutOfBoundsException was hiding in ClhsdbPrintAll.jtr#30808
8382485: ArrayIndexOutOfBoundsException was hiding in ClhsdbPrintAll.jtr#30808YaSuenag wants to merge 2 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back ysuenaga! A progress list of the required criteria for merging this PR into |
|
@YaSuenag 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 110 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 |
Webrevs
|
|
Changes in ClhsdbLauncher.java causes new error in mixed jstack tests on Alpine, but it would be fixed in #30815. |
| if (!allowStderrOutput) { | ||
| oa.stderrShouldBeEmptyIgnoreVMWarnings(); | ||
| } |
There was a problem hiding this comment.
We need to be careful here. I think the reason for by default allowing stderr output is because it can sometimes appear, but is not necessarily indicative of bug. For example, sometimes during the stack dump an active thread can produce an exception because it is not in a consistent state, but the thread we care about (like SteadyStateThread) produces what we are looking for, so the test should pass. I worry that this change is going to produce a lot of intermittent failures.
There was a problem hiding this comment.
Indeed the thread might have inconsistent state because they would be stopped by signal or ptrace().
So is it better to check *Exception: and *Error: in stderr?
There was a problem hiding this comment.
But that is what my concern is. We can see exceptions on stderr that we want to ignore.
There was a problem hiding this comment.
Understood, so should I remove changes for the test from PR? I think it is difficult to choose exceptions/errors what we can ignore.
There was a problem hiding this comment.
I reverted changes for testcode. I think it is reasonable so far.
@plummercj Can you review?
plummercj
left a comment
There was a problem hiding this comment.
Looks good. Thanks for the fix!
|
/integrate |
|
Going to push as commit df8a940.
Your commit was automatically rebased without conflicts. |
ArrayIndexOutOfBoundsException as following was hiding in ClhsdbPrintAll.java. See attached .jtr file in JBS.
AFAICS all of failures are caused by
getfieldbytecode. We can see it injavap, but in SA, another bytecode values (e.g. "234") were detected. Thusgetfieldwould not be handled as field operation at folloing code in BytecodeWithCPIndex.java, eventually the exception was thrown.In Bytecodes.java, following values should be handled as field operations.
Bytecodes.isFieldCode()should identify them.Maybe this is long-standing bug because the exception could not be caught jtreg tests, thus I updated testcase to check stderr. But ClhsdbDumpheap.java and ClhsdbJhisto.java would ignore this because they prints message into stderr or checks strings in stderr.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30808/head:pull/30808$ git checkout pull/30808Update a local copy of the PR:
$ git checkout pull/30808$ git pull https://git.openjdk.org/jdk.git pull/30808/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30808View PR using the GUI difftool:
$ git pr show -t 30808Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30808.diff
Using Webrev
Link to Webrev Comment