Re: [PATCH glibc 3/3] rseq registration tests (v10)

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On May 26, 2020, at 8:47 AM, Florian Weimer fweimer@redhat.com wrote:

> * Mathieu Desnoyers:
> 
>>> The present code does not wait until all threads have entered their
>>> cancellation region, so I'm not sure if the test object is actually met
>>> here.
>>
>> We're only cancelling the first thread in the test, which is the intent.
>> In terms of barrier, it's a barrier involving only 2 threads.
> 
> Huh.  I need to look at the version with the real barrier.

Useful bits:

static void
cancel_routine (void *arg)
{
  if (!rseq_thread_registered ())
    {
      printf ("error: rseq not registered in cancel routine\n");
      support_record_failure ();
    }
}

static pthread_barrier_t cancel_thread_barrier;
static pthread_cond_t cancel_thread_cond = PTHREAD_COND_INITIALIZER;
static pthread_mutex_t cancel_thread_mutex = PTHREAD_MUTEX_INITIALIZER;

static void
test_cancel_thread (void)
{
  pthread_cleanup_push (cancel_routine, NULL);
  (void) xpthread_barrier_wait (&cancel_thread_barrier);
  /* Wait forever until cancellation.  */
  xpthread_cond_wait (&cancel_thread_cond, &cancel_thread_mutex);
  pthread_cleanup_pop (0);
}

static void *
thread_function (void * arg)
{
  int i = (int) (intptr_t) arg;

  xraise (SIGUSR1);
  if (i == 0)
    test_cancel_thread ();
  TEST_COMPARE (pthread_setspecific (rseq_test_key, (void *) 1l), 0);
  return rseq_thread_registered () ? NULL : (void *) 1l;
}

static int
do_rseq_threads_test (int nr_threads)
{
  pthread_t th[nr_threads];
  int i;
  int result = 0;

  xpthread_barrier_init (&cancel_thread_barrier, NULL, 2);

  for (i = 0; i < nr_threads; ++i)
    th[i] = xpthread_create (NULL, thread_function,
                             (void *) (intptr_t) i);

  (void) xpthread_barrier_wait (&cancel_thread_barrier);

  xpthread_cancel (th[0]);

  for (i = 0; i < nr_threads; ++i)
    {
      void *v;

      v = xpthread_join (th[i]);
      if (i != 0 && v != NULL)
        {
          printf ("error: join %d successful, but child failed\n", i);
          result = 1;
        }
      else if (i == 0 && v == NULL)
        {
          printf ("error: join %d successful, child did not fail as expected\n", i);
          result = 1;
        }
    }

  xpthread_barrier_destroy (&cancel_thread_barrier);

  return result;
}

> 
>>>> +static int
>>>> +rseq_available (void)
>>>> +{
>>>> +  int rc;
>>>> +
>>>> +  rc = sys_rseq (NULL, 0, 0, 0);
>>>> +  if (rc != -1)
>>>> +    FAIL_EXIT1 ("Unexpected rseq return value %d", rc);
>>>> +  switch (errno)
>>>> +    {
>>>> +    case ENOSYS:
>>>> +      return 0;
>>>> +    case EINVAL:
>>>> +      return 1;
>>>> +    default:
>>>> +      FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno));
>>>> +    }
>>>> +}
>>> 
>>> Maybe add a comment to explain what EINVAL means in this context?
>>
>> For instance:
>>
>> /* rseq is implemented, but detected an invalid parameter.  */
> 
> Ah, so 0 is an invalid operation?

So the @flags parameter is 0, which is fine.

It's the @rseq_len parameter being 0 (which differs from sizeof(struct rseq))
which returns -EINVAL. I will clarify this in the comment:

/* rseq is implemented, but detected an invalid rseq_len parameter.  */

> 
>>>> +  retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
>>>> +  if (retpid != pid)
>>>> +    {
>>>> +      FAIL_EXIT1 ("waitpid returned %ld, expected %ld",
>>>> +                  (long int) retpid, (long int) pid);
>>>> +    }
>>> 
>>> Hmm.  Is the TEMP_FAILURE_RETRY really needed?  Our xwaitpid does not
>>> have this.
>>
>> Then how does it deal with a signal interrupting the system call performing
>> the waitpid (EINTR) ? I do not see WNOHANG being used.
> 
> It obscures spurious signals.  In most test cases, if an unexpected
> signal is delivered, something is quite wrong indeed.  This is why we
> don't generally hide EINTR errors.

So it means you may have trouble using tools like strace and gdb on those
tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for
profilers, those usually rely on a timer-driven signal.

> 
>>>> +/* Test C++ destructor called at thread and process exit.  */
>>>> +void
>>>> +__call_tls_dtors (void)
>>>> +{
>>>> +  /* Cannot use deferred failure reporting after main () returns.  */
>>>> +  if (!rseq_thread_registered ())
>>>> +    FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor");
>>>> +}
>>> 
>>> Uhm, what is this supposed to accomplish, under the __call_tls_dtors
>>> name in particular?  I don't think this gets ever called.
>>> 
>>> It may make sense to have a separate, smaller C++ test to cover this
>>> (perhaps as a separate patch).
>>
>> Hrm, the intent was to implement __call_tls_dtors locally so it would
>> be invoked by libc on thread/process exit, but looking deeper into
>> stdlib/cxa_thread_atexit_impl.c I suspect the hidden _call_tls_dtors
>> defined there will be used.
> 
> Right, it's not an interposable symbol.
> 
>> Indeed, a separate C++ test for this would be better. Could be done in a
>> follow up patch later perhaps ?
> 
> Yes, let's remove this and add a real C++ test later.

OK

> 
>>> C++ test could exercise the thread exit path via thread_local, without
>>> linking against libpthread.
>>
>> Should we keep this for a future patch ?
> 
> Yes, please.

OK, thanks!

Mathieu

> 
> Thanks,
> Florian

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com