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

Lutz Schmidt lucy at openjdk.java.net
Mon May 2 15:55:04 UTC 2022


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...
> 
> There is a JEP (https://bugs.openjdk.java.net/browse/JDK-8284289) under way which is supposed to clean up...

I understand your approach - don't mess with things that are not directly affected. On the other hand: if you have to touch the code anyway, why not improve the maintainability? Maybe the JEP author has an opinion whether the cleanup could be part of the JEP. And you might get support by other reviewers as well.

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

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


More information about the hotspot-dev mailing list