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

[Tobias Hartmann]

On Tue, 15 Jul 2025 15:36:30 GMT, Marc Chevalier <mchevalier 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` ...
>
> test/hotspot/jtreg/compiler/c1/TestDisableInstallMethods.java line 45:
> 
>> 43:         ProcessBuilder pb = ProcessTools.createLimitedTestJavaProcessBuilder("-XX:-InstallMethods", "-version");
>> 44:         OutputAnalyzer output = new OutputAnalyzer(pb.start());
>> 45:         output.shouldHaveExitValue(0);
> 
> Is it needed to go through a subprocess (at least, this explicitly)? Would `@run driver/othervm -XX:-InstallMethods compiler.c1.TestDisableInstallMethods` and an empty main (or just enough to do something) do the job?
> 
> It seems simpler to me (doesn't need any import for instance), but also would allow testing it in interaction with other flags, as tests works. Then, with IgnoreUnrecognizedVMOptions, can we get rid of the `@requires`? Even tho... it means we are then testing an empty program with no actual flags... Might not be very interesting.

I agree, you could also just add a test case to `test/hotspot/jtreg/compiler/arguments/TestC1Globals.java`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26310#discussion_r2208026912