RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]

[Jaroslav Bachorik]

On Wed, 27 Apr 2022 07:57:19 GMT, Lutz Schmidt <lucy at openjdk.org> wrote:

>> Jaroslav Bachorik has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix indentation
>
> 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.

@RealLucy 
I deliberately left out any attempts on consolidation as it might make the change affecting even more files across the whole repo. From my experience it would lower the chance of getting the fix reviewed and approved significantly so I opted for the minimum number of required changes that are preventing this particular crash.

There is a JEP (https://bugs.openjdk.java.net/browse/JDK-8284289) under way which is supposed to clean up and unify the implementations for obtaining profiling stacktraces. I would prefer having the sweeping changes being rather a part of the JEP effort then attaching them to this PR. I can create a follow up JIRA ticket for the tasks you have mentioned and link it with the JEP so your input does not get lost.

> 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?

You mean `bool forSignalHandler`?

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

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