RFR: JDK-8297408: Consolidate code in runtime/ErrorHandling

[David Holmes]

On Tue, 22 Nov 2022 10:39:34 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> There is a lot of duplicate coding in runtime/ErrorHandling that could be cut down using the new HsErrFileUtils utilities added with [JDK-8296906](https://bugs.openjdk.org/browse/JDK-8296906).
> 
> This patch mainly cuts away duplicate coding. Very minor functional changes:
> - All tests will now use try-with-resources when opening hs-err files
> - [TestCrashOnOutOfMemoryError.java](https://github.com/openjdk/jdk/pull/11283/files#diff-7560f2e2700932e015563716b3b0880495922c4c5731ddadba3b33bd937358bd) now checks for at least one matching line in the hs-err file - before, it basically just scanned for the END marker, which seemed pointless
> - [test/hotspot/jtreg/runtime/ErrorHandling/TestDwarf.java](https://github.com/openjdk/jdk/pull/11283/files#diff-c04dd18d5a29a42e3a9a008c0b917663a7617c8c9a0d6e9efe8755773260ea95) : heaved Pattern construction out of loop
> - [test/hotspot/jtreg/runtime/ErrorHandling/TimeoutInErrorHandlingTest.java](https://github.com/openjdk/jdk/pull/11283/files#diff-086956c42403a16043a111b0582736ac21ce0522dba866f5f9bc3fdb16037a0c) : I tried to preserve the debug output that was painstakingly added by @dcubed-ojdk apart from one change: I now printe the hs-err file as part of the scanning process. This works fine since in case of success, it will only be parsed as far as all patterns are matched, which happens after five-ish lines. Only if none are found, we scan and therefore print the whole hs-err file. So the old behavior is preserved in spirit.

Overall this seems quite reasonable. A few minor nits below.

Thanks.

test/hotspot/jtreg/runtime/ErrorHandling/BadNativeStackInErrorHandlingTest.java line 69:

> 67:     };
> 68: 
> 69:     HsErrFileUtils.checkHsErrFileContent(hs_err_file, null, negativePatterns, true, false);

Can we annotate the true/false parameters so the reader doesn't have to go and look up what they mean i.e. `true /* check END */, false /*not verbose */` ?

test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java line 64:

> 62: 
> 63:     /**
> 64:      * Given an open hs-err file, read it line by line and check for existence of a pattern. Will fail

"a set of patterns".

test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java line 65:

> 63:     /**
> 64:      * Given an open hs-err file, read it line by line and check for existence of a pattern. Will fail
> 65:      * if pattern are missing, or if the END marker is missing.

"patterns are".

test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java line 79:

> 77:      * Given an open hs-err file, read it line by line and check for various conditions.
> 78:      * @param f input file
> 79:      * @param positivePatterns Optional array of patterns that need to appear, in given order, in the file. Missing

Consistency check: Sometimes you start the description with a capital letter and other times not. I don't know what preferred JDK Java style is.

test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java line 80:

> 78:      * @param f input file
> 79:      * @param positivePatterns Optional array of patterns that need to appear, in given order, in the file. Missing
> 80:      *                        pattern cause the test to fail.

patterns

test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java line 83:

> 81:      * @param negativePatterns Optional array of patterns that must not appear in the file; test fails if they do.
> 82:      *                        Order is irrelevant.
> 83:      * @param checkEndMarker If true, we check for the final "END" in an hs-err file; it missing indicates hs-err file

"if it is missing it indicates"

test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java line 133:

> 131:                 }
> 132:                 lastLine = line;
> 133:                 lineNo ++;

No space before unary operators.

test/hotspot/jtreg/runtime/ErrorHandling/TestDwarf.java line 129:

> 127: 
> 128:         File hs_err_file = HsErrFileUtils.openHsErrFileFromOutput(crashOut);
> 129:         try (BufferedReader reader = new BufferedReader(new FileReader(hs_err_file))) {

Not sure this is the correct twr idiom when constructing multiple resources ie should it be:

try (var fr = new FileReader(hs_err_file);
      var reader = new BufferredReader(fr);) {
...
}

-------------

PR: https://git.openjdk.org/jdk/pull/11283