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

[Jaroslav Bachorik]

On Wed, 4 May 2022 10:53:29 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Jaroslav Bachorik has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Extract common method for finding code blob in a signal handler
>
> Hi,
> 
> sorry for the late chiming in.
> 
> Could we find a better way to do this that to funnel "is_signal_handler" thru every layer? How about a thread-local state instead? E.g. "bool Thread::_unsafe_code_cache_lookup"? Or, for that matter, "bool Thread::in_agct"? Could be set and restored at the beginning resp. end of AGCT.
> 
> I dislike the name "in_signal_handler" since the fact that we are inside signal handling is incidental. IIUC this is probably much more often a thread-safety issue than a reentrance issue, right? You would have the same problem if you analyzed the code cache without getting the code cache lock, while it is concurrently modified? 
> 
> So if you want to keep the argument, how about renaming it to "unsafe", or "unsafe_access", or "unsafe_codecache_lookup"? We use "unsafe" in other places as well.
> 
> Cheers, Thomas

Hi @tstuefe ,

Thanks for the review.

> Could we find a better way to do this that to funnel "is_signal_handler" thru every layer? How about a thread-local state instead? E.g. "bool Thread::_unsafe_code_cache_lookup"? Or, for that matter, "bool Thread::in_agct"? Could be set and restored at the beginning resp. end of AGCT.

I wanted to avoid as much 'magic' as possible because usually I find such PRs as having higher chance of getting reviewed. But if using a TLS to avoid funnelling extra arg is something what would usually be done I will revisit the patch and use that instead.

> I dislike the name "in_signal_handler" since the fact that we are inside signal handling is incidental. IIUC this is probably much more often a thread-safety issue than a reentrance issue, right? You would have the same problem if you analyzed the code cache without getting the code cache lock, while it is concurrently modified?

> So if you want to keep the argument, how about renaming it to "unsafe", or "unsafe_access", or "unsafe_codecache_lookup"? We use "unsafe" in other places as well.

Interestingly enough, my first sketch of the patch was using the `unsafe_access` named argument :) Then I renamed it to look less scary. If the TLS approach won't work out and I need to pass in the extra argument I can definitely rename it `unsafe_access` if that is not going to freak out the reviewers :)

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

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