RFR: 8291654: AArch64: assert from JDK-8287393 causes crashes

[Evgeny Astigeevich]

On Mon, 1 Aug 2022 23:53:05 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> https://github.com/openjdk/jdk/commit/6cbc234ad17c5a0c4b3d6ea76f807c27c1dc8330 adds an assert checking `in_scratch_emit_size` is set during the output phase of C2 compilation. The assert can fail if C1 uses `trampoline_call`.
>> The assert is not needed as the calculation of `in_scratch_emit_size` is short-circuited to false if it is C1.
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 883:
> 
>> 881:       assert(StubRoutines::aarch64::complete() &&
>> 882:              Thread::current()->is_Compiler_thread() &&
>> 883:              Compile::current()->output() != NULL, "not in output phase of C2 compilation");
> 
> The code is under `#ifdef COMPILER2` which assumes that it is executed only for C2. Which make it confusing because it CAN be executed by C1.
> There is check at line 885 `is_c2_compile()` which is true only for C2. May be we can rearrange code to make it less confusing.
> On other hand if `check_emit_size` could be `true` for C1 why we should emit trampoline for it and not for C2?

@vnkozlov 
> On other hand if `check_emit_size` could be `true` for C1 why we should emit trampoline for it and not for C2?

IMHO `check_emit_size` should always be `false` for C1 because C1 does not have such functionality. This was not caught by testing.

The problem is what the default value `check_emit_size` should have. The current default value `true` leads to confusion for the cases where 'check emit size' has no meaning like C1 or stub generation. 

A possible solution simplifying everything can be:


virtual bool MacroAssembler::emit_trampoline_stub(...) {
  The default implementation which always generates a trampoline code.
}


address MacroAssembler::trampoline_call(Address entry, CodeBuffer* cbuf) {
  bool need_trampoline = far_branches();
  if (!need_trampoline && entry.rspec().type() == relocInfo::runtime_call_type && !CodeCache::contains(entry.target())) {
...
  }

  if (cbuf) cbuf->set_insts_mark();
  relocate(entry.rspec());
  if (!need_trampoline) {
    bl(entry.target());
  } else {
    if (!emit_trampoline_stub(offset(), entry.target())) {
      postcond(pc() == badAddress);
      return NULL; // CodeCache is full
    }
    bl(pc());
  }
  // just need to return a non-null address
  postcond(pc() != badAddress);
  return pc();
}

bool C2_MacroAssembler::emit_trampoline_stub(...) {
   auto phase_output = Compile::current()->output();
   if (phase_output !=NULL && phase_output->in_scratch_emit_size()) {
      // We don't want to emit a trampoline if C2 is generating dummy
      // code during its branch shortening phase.
     return true;
   }
   return MacroAssembler::emit_trampoline_stub(...);
}


I can submit PR with it if there is interest in the solution.
What do you think?

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

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