Re: [PATCH] nptl: Fix invalid Systemtap probe in pthread_join [BZ #24211]

[Carlos O'Donell <carlos at redhat dot com>]

On 2/11/19 4:26 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> Why not move the probe up?
>>
>>  77   if (block)
>>  78     {
>>  79       /* If abstime is NULL we switch to asynchronous cancellation.  If we
>>  80          are cancelled then the thread we are waiting for must be marked as
>>  81          un-wait-ed for again.  */
>>  82       pthread_cleanup_push (cleanup, &pd->joinid);
>>  83 
>>  84       result = lll_wait_tid (pd->tid, abstime);
>>  85 
>>  86       pthread_cleanup_pop (0);
>>  87     }
>>  88 
>>
>> LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
>> ^^^^
> 
> I think it should come after the stack has been freed.  It's explicitly
> documented as a return probe (“probe for pthread_join return”).

Can you tell the difference?

The compiler might move all sorts of things after the probe that will
happen before the final return insruction.

>> At this point the join is complete.
>>
>> We have the threadid (constant).
>>
>> We have the result (just returned from lll_wait_tid).
>>
>> We have pd->result (the thread returned).
>>
>> We aren't waiting for anything else?
> 
> Something might be interested in the fact that the stack is actually
> gone.  I'm not sure if this is a change we should make when backporting.

Nobody guarantees the stack is gone, and in fact with the stack cache,
and a future configurable stack cache, the cache may be around forever
until the program exits.

The probes are explicitly off the list of things which are ABI/API unless
we document them as such. Therefore I think they can reasonable change
in their exact triggering position. If you change anything else, like
semantics or arguments then you need a new name, and you can do that if
you want, because this master is development for glibc.

-- 
Cheers,
Carlos.