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

[Marc Chevalier]

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!

Nice description!

src/hotspot/share/c1/c1_Compilation.cpp line 490:

> 488:     install_code(frame_size);
> 489:   } else {
> 490:     // nothing else to do

Not sure how helpful is this comment: if one should not install the code, we don't install the code, but it doesn't really tells me why we need to do something then. I wouldn't mind no comment at all, and I can git blame and find the JBS issue/PR to see the motivation, but most of the time, I wouldn't question this line in particular, so I'm fine not having distractions in the code giving very specific details. There are very specific details I'm not challenging absolutely everywhere...

Otherwise, maybe writing something like "making sure the lack of installed code is not confused with a compilation bailout".

But once again, I'm rather on the side of "I'll look at the blame and PR if I ever question it, but I'll probably never". The PR explaining well the situation, I'm fine with it!

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.

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

PR Review: https://git.openjdk.org/jdk/pull/26310#pullrequestreview-3021010979
PR Review Comment: https://git.openjdk.org/jdk/pull/26310#discussion_r2207883258
PR Review Comment: https://git.openjdk.org/jdk/pull/26310#discussion_r2207867332