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

[Thomas Stuefe]

On Tue, 17 May 2022 08:13:11 GMT, Jaroslav Bachorik <jbachorik at openjdk.org> wrote:

>> src/hotspot/share/prims/forte.hpp line 53:
>> 
>>> 51:   ASGCTMark() : ASGCTMark(JavaThread::current()) {}
>>> 52:   ~ASGCTMark() {
>>> 53:     JavaThread::current()->set_in_asgct(false);
>> 
>> 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.
>
> 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

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

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