Re: SIGPROF + SIGCHLD and igc

[Eli Zaretskii <eliz@gnu.org>]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: pipcet@protonmail.com,  gerd.moellmann@gmail.com,  ofv@wanadoo.es,
>   emacs-devel@gnu.org,  acorallo@gnu.org
> Date: Sat, 28 Dec 2024 14:52:33 +0100
> 
> On Sat, Dec 28 2024, Eli Zaretskii wrote:
> 
> >> It seems that the statement
> >> 
> >>   block SIGPROF while MPS holds the lock
> >> 
> >> is logically equivalent to
> >> 
> >>   deliver SIGPROF only while MPS does not hold the lock.
> >
> > Not necessarily.  Blocking a signals delays its delivery until the
> > time the signal is unblocked.  By contrast, what you propose will be
> > unable to profile code which caused MPS to take the lock.
> 
> Hmm, I think I disagree.  But I'm not sure of what sequence of events
> you're thinking of.

I'm thinking about a situation where SIGPROF was delivered while it
was blocked.  In that case, it will be re-delivered once we unblock
it.

By contrast, if we avoid delivering SIGPROF in the first place, it
will never be delivered until the next time SIGPROF is due.

So imagine a function FUNC that conses some Lisp object.  This calls
into MPS, which blocks SIGPROF, takes the arena lock, then does its
thing, then releases the lock and unblocks SIGPROF.  If SIGPROF
happened while MPS was working with SIGPROF blocked, then the moment
SIGPROF is unblocked, the SIGPROF handler in the main thread will be
called, and will have the opportunity to see that we were executing
FUNC.  By contrast, if the profiler thread avoided delivering SIGPROF
because it saw the arena locked, the next time the profiler thread
decides to deliver SIGPROF, execution could have already left FUNC,
and thus FUNC will not be in the profile.

I hope I made myself more clear this time.

> >> This variant might be bit easier to implement.  The "while MPS does not
> >> hold the lock" part can be implemented by claiming the lock in the
> >> profiler thread like so:
> >> 
> >>   mps_arena_t arena = global_igc->arena;
> >>   ArenaEnter (arena);
> >>   ... deliver SIGPROF part goes here ...
> >>   ArenaLeave (arena);
> >
> > What happens if, when we call ArenaEnter, MPS already holds the arena
> > lock?
> 
> Since MPS holds the lock, it would run in a different thread.

Yes, of course: we are talking about an implementation where the
profiler thread is separate, so the above code, which AFAIU runs in
the profiler thread, will be in a thread separate from the one where
MPS runs.

> So the profiler thread blocks until MPS releases the lock.
> 
> ArenaEnter uses non-recursive locks.

Hm... if ArenaEnter uses non-recursive locks, how come we get aborts
if some code tries to lock the arena when it is already locked?  IOW,
how is this situation different from what we already saw several times
in the crashes related to having SIGPROF delivered while MPS holds the
arena lock?

> 
> >> The "deliver SIGPROF" part goes like this:
> >> 
> >> 1. The profiler thread calls pthread_kill (SIGPROF, <main_thread>) and
> >>    then waits (on a pipe or whatever).
> >> 
> >> 2. The SIGPROF handler gets called and immediately notifies the profiler
> >>    thread (without waiting for a reply).  After that, it continues as
> >>    usual calling get_backtrace etc.
> >> 
> >> 3. The profiler thread awakes and releases the lock.
> >
> > This leaves a small window between the time the SIGPROF handler writes
> > to the pipe and the time the profiler thread calls ArenaLeave.  During
> > that window, the arena is locked, AFAIU, and we can still have
> > recursive-lock situations, which cause an abort.  Am I missing
> > something?
> 
> During that time window, the lock is held by the profiler thread.  The
> SIGPROF handler runs in the main thread.  If the main thread tries to
> claim the lock, it will block until the profiler thread releases it.

See above: I thought that such a situation triggers crashes.  I'm
probably missing something.

> >> Regarding deadlocks: the profiler thread holds the lock while it waits.
> >> So MPS should not be able to stop the profiler thread there.
> >
> > Which means we don't register the profiler thread with MPS, right?
> 
> I'm not sure. It may not be safe to call ArenaEnter in non-registered
> threads.

But if we do register the thread, then MPS _will_ stop it, no?