RFR: 8358573: CompileBroker fails with "expect failure reason" assert with -XX:-InstallMethods

[Dean Long]

On Tue, 15 Jul 2025 09:21:53 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

> This PR prevents from hitting an assert when disabling method installation at the end of a successful compilation with the `-XX:-InstallMethods` flag. Previously `CompileBroker` failed to mark the `CompileTask` as complete when using this flag.
> 
> ## Analysis
> 
> We can see that the assert is triggered in `CompileBroker::invoke_compiler_on_method`:
> 
> ```c++
>     if (!ci_env.failing() && !task->is_success()) {
>       assert(ci_env.failure_reason() != nullptr, "expect failure reason");
>       assert(false, "compiler should always document failure: %s", ci_env.failure_reason());
>       // The compiler elected, without comment, not to register a result.
>       // Do not attempt further compilations of this method.
>       ci_env.record_method_not_compilable("compile failed");
>     }
> 
> 
> The `task->is_success()` call accesses the private `_is_success` field. 
> This field is modified in `CompileTask::mark_success`.
> 
> By setting a breakpoint there, and execute the program without `-XX:-InstallMethods`, we get the following stacktrace:
> 
> CompileTask::mark_success compileTask.hpp:185
> nmethod::post_compiled_method nmethod.cpp:2212
> ciEnv::register_method ciEnv.cpp:1127
> Compilation::install_code c1_Compilation.cpp:425
> Compilation::compile_method c1_Compilation.cpp:488
> Compilation::Compilation c1_Compilation.cpp:609
> Compiler::compile_method c1_Compiler.cpp:262
> CompileBroker::invoke_compiler_on_method compileBroker.cpp:2324
> CompileBroker::compiler_thread_loop compileBroker.cpp:1968
> CompilerThread::thread_entry compilerThread.cpp:67
> JavaThread::thread_main_inner javaThread.cpp:773
> JavaThread::run javaThread.cpp:758
> Thread::call_run thread.cpp:243
> thread_native_entry os_linux.cpp:868
> 
> 
> We go up the stacktrace and see that in `Compilation::compile_method` we have:
> 
> ```c++
>   if (should_install_code()) {
>     // install code
>     PhaseTraceTime timeit(_t_codeinstall);
>     install_code(frame_size);
>   }
> 
> 
> If we do not install methods after compilation, the code path that marks the success is never executed
> and therefore results in hitting the assert.
> 
> ### Fix
> We simply mark the task as complete when `should_install_code()` evaluates to `false` in the block code above.
> 
> ### Testing
> - [x] [GitHub Actions](https://github.com/benoitmaillard/jdk/actions?query=branch%3AJDK-8358573)
> - [ ] tier1-3, plus some internal testing
> - [x] Added a test that starts the VM with the `-XX:-InstallMethods` flag
> 
> Thank you for reviewing!

Is this flag really supported?  I can't find any tests for it.  I wonder if anyone would miss it if we removed it.

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

PR Comment: https://git.openjdk.org/jdk/pull/26310#issuecomment-3075924647