bug#75275: 30.0.92; `make-thread` bug on macOS 15.2

["Gerd Möllmann" <gerd.moellmann@gmail.com>]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: stefankangas@gmail.com,  alan@idiocy.org,  75275@debbugs.gnu.org
>> Date: Thu, 02 Jan 2025 09:41:38 +0100
>> 
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>> 
>> > Eli Zaretskii <eliz@gnu.org> writes:
>> >
>> >> So should we add a condition before calling [NSApp run] that we are in
>> >> the main thread?
>> >
>> > ATM, I don't understand how we land in that line in ns_select_1 if not
>> > [NSThread isMainThread]. Maybe I need new glasses. I asked Stefan if he
>> > can see something in LLDB.
>> 
>> It must something in here:
>> 
>>   if (![NSThread isMainThread]
>>       || (timeout && timeout->tv_sec == 0 && timeout->tv_nsec == 0))
>>     thread_select (pselect, nfds, readfds, writefds,
>> 		   exceptfds, timeout, sigmask);
>> 
>> Should we return here?
>
> I don't know.  Is there anything in the following code that can be
> relevant to a non-main thread?  Note that non-main threads can
> legitimately call wait_reading_process_output, which calls ns_select.
> For example, what happens if a non-main Lisp thread starts a
> sub-process? we do expect to be able to read the output from that
> sub-process.

Really hard to tell. Perhaps someone could try to follow what I write
below and tell if it makes sense? Everything in ns_select_1.

1. I think this code must run in a non-main thread:

  if (nr > 0)
    {
      pthread_mutex_lock (&select_mutex);
      ... set some variables ...
      /* Inform fd_handler that select should be called.  */
      c = 'g';
      emacs_write_sig (selfds[1], &c, 1);
    }

selfds is apparently some pipe, NS-specific. The function fd_handler is
called when writing to the pipe I assume. fd_handler is set up like
this

  [NSThread detachNewThreadSelector:@selector (fd_handler:)
                           toTarget:NSApp
                         withObject:nil];

Looks to me like it runs in a thread of its own. fd_handler then
pselects on the fd sets set in the if above. That looks like it is
relevant to reading process output. And that means we may _not_ return
from ns_select_1 early when ![NSThread isMainThread].

  else if (nr == 0 && timeout)
    {
      /* No file descriptor, just a timeout, no need to wake fd_handler.  */
      double time = timespectod (*timeout);
      timed_entry = [[NSTimer scheduledTimerWithTimeInterval: time
                                                      target: NSApp
                                                    selector:
                                  @selector (timeout_handler:)
                                                    userInfo: 0
                                                     repeats: NO]
                      retain];
    }

2. This code

  else if (nr == 0 && timeout)
    {
      /* No file descriptor, just a timeout, no need to wake fd_handler.  */
      double time = timespectod (*timeout);
      timed_entry = [[NSTimer scheduledTimerWithTimeInterval: time
                                                      target: NSApp
                                                    selector:
                                  @selector (timeout_handler:)
                                                    userInfo: 0
                                                     repeats: NO]
                      retain];
    }

  means basically only to send an app-defined event after a timeout. I
  interpret this as "leave the NS event loop to let Emacs do things
  after a timeout". Looks okay to me.

3. This

  else /* No timeout and no file descriptors, can this happen?  */
    {
      /* Send appdefined so we exit from the loop.  */
      ns_send_appdefined (-1);
    }

  is likely also okay because send_app_defined has code checking for
  being in the main thread.

4. The [NSApp run] follows, and it can under no circumstances be done
   in a mon-main thread. We should put that in an if for sure.

  if ([NSThread isMainThread]) [NSApp run];

5. The code below is another enigma.

  I can't figure out why that is done, and what
  last_appdefined_event_data is for. But since it is run today, I'd
  propose to just let it run. I don't see that it does immediate harm.

:-/