RFR: 8332474: Tighten up ToolBox' JavacTask to not silently accept javac crash as a failure

[Jonathan Gibbons]

On Mon, 1 Jul 2024 14:09:39 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

> Tests for javac use several test frameworks, and one of them is the "toolbox", which provides `JavacTask` that allows to conveniently run javac on a given set of sources. The problem with `JavacTask` is that, by default, when a compilation failure is expected, javac crash (exit code 4) is tolerated by the `JavacTask`. And the test must manually select a specific exit code to overwrite this behavior.
> 
> This then leads to bugs like https://bugs.openjdk.org/browse/JDK-8335385, which are silently ignored by `JavacTask`.
> 
> The proposal herein is to tighten up the `JavacTask`, and effectively disallow exit code 4 for `JavacTask` (but permit any other exit code, as javac is using several exit codes). The base implementation in `AbstractTask` is changed to use a validator for the exit codes, which is then leveraged by `JavacTask`.
> 
> This patch depends on PR #19969, as that fixes JDK-8335385, where the javac crash is ignored. It also tweaks module attribution to not leave empty `JCModuleDecl.sym` for duplicate modules, and set it to an erroneous module. The empty (`null`) symbol here crashes javac in the `test/langtools/tools/javac/modules/MultiModuleModeTest.java#testDuplicateModules`, and the test wouldn't pass with this more strict `JavacTask`.

test/langtools/tools/lib/toolbox/AbstractTask.java line 94:

> 92:      * @param exitCodeValidator an exit code validator. The first parameter will
> 93:      *                          be the actual exit code, the second test name,
> 94:      *                          should throw TaskError is the exit code is not

typo: "is the exit code" -> "if the exit code"

test/langtools/tools/lib/toolbox/JavacTask.java line 331:

> 329:     @Override
> 330:     public Result run(Expect expect, int exitCode) {
> 331:         if (exitCode == 4) {

I guess we never expect `javac` to crash, and should not be able to test that it does, right?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19972#discussion_r1661398227
PR Review Comment: https://git.openjdk.org/jdk/pull/19972#discussion_r1661400077