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

[David Holmes]

On Tue, 17 May 2022 09:07:26 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Ok, I fixed the `ASGCTMark` to be safe to use from a signal handler.
>> 
>> I have no strong opinion about whether we should keep it or not - it makes the code in `forte.cpp` slightly more resilient in case of further modifications for the price of more complexity introduced by the mark class ��
>
> Hi David,
> 
>> You can't call `JavaThread::current()` in any of this code as it is not safe from a signal handling context. (There is an existing use in ASGCT that is also unsafe.) I suggest not having the no-arg constructor and require the current JavaThread, or null, to be passed in (having already been safely obtained by the caller). You can then assert using `Thread::current_or_null_safe()`.
>> 
>> Personally I find the ASGCTMark complete overkill for this situation as there is only ever going to be a single use - sorry @tstuefe - it is just complicating things IMO.
> 
> No problem, that's why we talk :)
> 
> To make an argument for this: if you compare the first iterations of this patch to the current one, the current one is a lot simpler. I really dislike funneling arguments through many layers down to a utility function, only to fine control one specific behavioral aspect of that tiny function, and that aspect is only relevant for one outlier use case. I dislike this more than the RAII object.
> 
> Look at it this way. We have a thread-specific state (we are in AGCT and deep down somewhere we want to do something different depending on that state). We need to pass that information down. The only difference is where we keep the "we are in thread-specific state" info - on the stack as an argument chain, or anchored to TLS.
> 
> Putting it into arguments is one way, but I find this rather ugly. Now every caller of this function has to care. With the RAII, we have the information as a mark exactly where it matters - in the entrance frame of AGCT - and no outside code needs to care.
> 
> Cheers, Thomas

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.

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

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