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

[Thomas Stuefe]

On Tue, 17 May 2022 14:54:23 GMT, Johannes Bechberger <duke at openjdk.java.net> wrote:

>> But you aren't keeping it on the stack versus TLS you are using a stack object to set the TLS. All we have done in this final form is replace:
>> 
>> 
>> thread->set_in_asgct(true);
>> <stuff>
>> thread->set_in_asgct(false);
>> 
>> with
>> 
>> 
>> ASGCTMark mark(thread);
>> <stuff>
>> 
>> but now ASGCTMark has to worry about whether `thread` is NULL, whether it is the current javaThread, when we've already done all that for `thread`.
>> 
>> And this is the only place we will ever use ASGCTMark. Now if we had lots of return points in `<stuff>` then RAII would definitely be better to ensure we can't forget to reset the state. But we don't. So use of RAII adds complexity cost now just in case in the future RAII might be useful.
>
> But the crash-handler does not prevent that `guarantee` aborts the VM, as we discussed already in my related PR. My comment was more a suggestion and related to "And this is the only place we will ever use ASGCTMark" by @dholmes-ora . I agree with you that changing JFR should be done in another PR.

> AFAIK, currently JFR is 'wrapping' the code in a crash-handler. We can take a look at reusing this approach there in some follow-up PR but for now I would really prefer getting this one merged without attaching any more bells&whistles.

@jbachorik No, please don't do that. That approach is very unsafe and should be used with extreme care. I would actually prefer for it to get removed completely, not to be reused.

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

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