RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee

[Lutz Schmidt]

On Thu, 31 Mar 2022 15:45:05 GMT, Jaroslav Bachorik <jbachorik at openjdk.org> wrote:

> A gist of the fix is to allow relaxed instantiation of a frame when done from a signal handler - eg. for profiling purposes. 
> 
> Currently, a frame instantiation will fail on guarantee when we happen to hit a zombie method which is still on stack. While this would indicate a serious error for the normal execution flow, in case of profiling where the executing thread can be expected at any possible method this is something which may happen and we really should not take the profiled JVM down due to it.
> 
> The behaviour defaults to checking the code blob status in the guarantee so nothing will change for the rest of the callers - just ASGCT will be affected.
> 
> <hr>
> Unfortunately, I am not able to create a simple reproducer for the crash other that testing in our production where the crash is happening sporadically.
> However, thanks to @parttimenerd and his [ASGCT stress test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be reproduced quite reliably.

This is not a complete review yet, just some general remarks.

While I absolutely appreciate the tedious work and time you invested, I dislike the widespread nature of your changes. This is not just your fault. Your changes only make deficiencies in the code layout really obvious. 

Did you spend a thought on consolidating e.g. the changes in `os_cpu` into a common file, potentially in `os/linux` or, even better, `os/posix`? The` cpu` tree is begging for consolidation as well. Maybe most of the init stuff can be done in `runtime/frame.*`?

While browsing through the changes, I found that in `pd_get_top_frame()`, the code is inconsistent. Some instances provide special handling for `#if COMPILER2_OR_JVMCI`, others only for `#if COMPILER2`, and sometimes there is no special handling at all. We should either have it everywhere or nowhere. Furthermore, I would prefer to see only one syntactical form, either `#if`, or `#ifdef` or `#if defined()`. Please note the subtle differences!

I added a few inline comments where the code needs some TLC.

src/hotspot/cpu/aarch64/frame_aarch64.inline.hpp line 52:

> 50:   assert(pauth_ptr_is_raw(pc), "cannot be signed");
> 51:   intptr_t a = intptr_t(sp);
> 52:   intptr_t b = intptr_t(fp);

What are these declarations good for?

src/hotspot/cpu/aarch64/frame_aarch64.inline.hpp line 58:

> 56:   _pc = pc;
> 57:   assert(pc != NULL, "no pc?");
> 58: if (!forSignalHandler) {

Indentation?

src/hotspot/cpu/aarch64/frame_aarch64.inline.hpp line 58:

> 56:   _pc = pc;
> 57:   assert(pc != NULL, "no pc?");
> 58: if (!forSignalHandler) {

Indentation?

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

Changes requested by lucy (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8061