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

[Florian Weimer <fweimer@redhat.com>]

* 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.

>>> +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?

>>> +  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.

>>> +/* 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.

>> 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.

Thanks,
Florian