git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff Hostetler <git@jeffhostetler.com>
To: Jeff King <peff@peff.net>,
	Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>
Cc: "Jeff Hostetler" <jeffhost@microsoft.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH 0/1] trace2: fix tracing when NO_PTHREADS is defined
Date: Wed, 22 May 2019 09:23:39 -0400	[thread overview]
Message-ID: <97796007-db6e-f2ea-91ae-3113b74e4ae9@jeffhostetler.com> (raw)
In-Reply-To: <20190521212744.GC14807@sigill.intra.peff.net>



On 5/21/2019 5:27 PM, Jeff King wrote:
> On Tue, May 21, 2019 at 12:33:58PM -0700, Jeff Hostetler via GitGitGadget wrote:
> 
>> As Duy suggested, pthread_getspecific() just returns NULL when NO_PTHREADS
>> is defined. And pthread_setspecific() silently does not nothing. So this
>> problem was hidden from view.
>>
>> I have to wonder if we should update pthread_*specific() to call BUG() when
>> NO_PTHREADS is defined as a way to catch unguarded usages easier or make
>> this issue more clear.
> 
> I think it should actually store the data asked for by the caller, as if
> we were the single thread running. We discussed this as the time of
> refactoring NO_PTHREADS, but there was only one caller that would have
> benefited. Now there are two. ;)
> 
> Discussion in the subthread of this patch:
> 
>    https://public-inbox.org/git/20181027071003.1347-2-pclouds@gmail.com/
> 
> -Peff
> 

I was wondering about that too as the proper long term solution.
We would need (as the discussion suggests [1]) to properly
respect/represent the pthread_key_t argument.

For now, I've guarded my usage of pthread_getspecific() in the trace2
(similar to what index-pack does), so its not urgent that we update it.
And I'd rather we take this simple trace2 fix now and not try to combine
it with fixes for the pthread macros.  Especially now as we're in the RC
cycle for 2.22.

I'll make a note to revisit the pthread code after 2.22.

Thanks
Jeff


[1] 
https://public-inbox.org/git/CACsJy8DLW_smOJd6aCoRcJZxQ2Lzut5US=sPadj7=fhne0UHGg@mail.gmail.com/


  reply	other threads:[~2019-05-22 13:23 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21 19:33 Jeff Hostetler via GitGitGadget
2019-05-21 19:33 ` [PATCH 1/1] " Jeff Hostetler via GitGitGadget
2019-05-21 21:27 ` [PATCH 0/1] " Jeff King
2019-05-22 13:23   ` Jeff Hostetler [this message]
2019-05-23  5:51     ` Jeff King
2019-05-23 13:55       ` Jeff Hostetler
2019-05-25 10:43       ` Duy Nguyen
2019-05-28  6:28         ` Jeff King
2019-05-28 14:57           ` Jeff Hostetler
2019-05-28 17:14     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=97796007-db6e-f2ea-91ae-3113b74e4ae9@jeffhostetler.com \
    --to=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH 0/1] trace2: fix tracing when NO_PTHREADS is defined' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).