[PATCH] nptl: futex_lock_pi deadlock detection provides valuable information but it is turned into a rather cryptic assertion failure

[Carlos O'Donell]

On 4/14/26 4:14 PM, Adhemerval Zanella Netto wrote:
> 
> 
> On 08/04/26 17:07, Moritz KLAMMLER (FERCHAU) wrote:
>> Thanks for your detailed review.  Those additional test support helpers
>> I haven't been aware of do help simplify the code a great amount indeed.
>>
>>>> +#ifndef TST_DEADLK_MUTEX_PI
>>>> +#  define TST_DEADLK_MUTEX_PI 0
>>>> +#endif
>>>
>>> Just use the TST_DEADLK_MUTEX_PI=1 and move this test to
>>> tst-deadlk-pi.c.
>>
>> Ah, sorry, I forgot to explain that point in my previous message.  I had
>> originally added this configurability based on the assumption that we
>> might want to use it to also test that non-PI mutexes actually do
>> deadlock by making these XFAIL tests, but didn't went as far as actually
>> doing that.  However, based on your other feedback ...
>>
>>>> The added test case currently only checks for EDEADLK to be returned,
>>>> but doesn't cover the cases where we still expect the deadlock to
>>>> happen.  I could add these as well, but it would introduce an
>>>> (unreasonably?) long delay, waiting for the alarm clock to go off
>>>> eventually.  Would you rather take this delay over the lack of test
>>>> coverage?  Please also let me know whether having one test executable
>>>> that loops over the various types (current patch) or one executable
>>>> per type would be preferred.
>>>
>>> I would prefer the later (one less binary to build and run). For the
>>> deadlock to happen, it would be better to spawn a new process with
>>> support_capture_subprocess, trigger the deadlock, and wait it with
>>> short delayed_exit value.
>>
>> ... I went ahead and merged everything into a single tst-deadlk.c file
>> which now checks PI and non-PI mutexes alike.  Since the number of
>> combinations became quite large, I refactored the test code to remove
>> the explicit list in lieu of some nested loops and a simple
>> should_detect_deadlock predicate function.
>>
>> I've picked 3 s for a delayed_exit value, hoping that this wold be a
>> sensible choice, given that nptl/tst-eintr1.c uses the same (and seems
>> to be the only other NPTL test case currently using delayed_exit).
>> Alas, this makes the test register approx. 30 s on the clock.  I've
>> raised the overall timeout (which should never be hit) to 60 s
>> accordingly.  Are you okay which such a slow test or do we need to
>> improve on this?  We could, for example, do all the waiting for all
>> the subprocesses in parallel, but I'm afraid that it would add quite
>> some complexity.  A lower-hanging fruit might be to reduce the delay
>> from 3 s to 1 s, which has worked well for my computer even with high
>> concurrent CPU load, but I'm not sure how universally acceptable it
>> might be.
>>
>> I found that still using alarm(3) in the subprocess and then expecting
>> -SIGALRM in the parent would result in slightly simpler code compared
>> to the delayed_exit but since you recommended it and I've also seen
>> that other test cases have been refactored in the past specifically to
>> replace alarm with delayed_exit, I assume that this is preferred way
>> to go.
>>
>> Looking forward to your feedback on the updated patch.
> Carlos, you brought to my attention that glibc current Error Handling
> guideline [1] states:
> 
>    "If it's user code invoking undefined behavior, then it should fail
>     early and catastrophically so that developers don't get the false
>     impression that their code is OK when it happens not to break the
>     use cases they test adequately."
> 
> However, this patch changes the ERRORCHECK and RECURSIVE pthread types
> and, although POSIX only states a *shall fail* for self-deadlock, it does
> permit an error return for multi-mutex deadlock in the *may fail*.
> 
> This change still has the potential to break code that does not handle
> EDEADLK in such cases; however this currently aborts the process anyway.
> So different than I has said (sorry if I forgot all the details on Monday's
> call), this is *not* UB and I think that issuing a fatal error like
> futex_fatal_error is not the course of action here.
> 
> The assert really has not place since kernel does allow to have fast deadlock
> check and POSIX allows it.

If we aren't triggering undefined behaviour then I think it's perfectly
acceptable to return a "may fail" error to the caller.

-- 
Cheers,
Carlos.