[RFC/PATCH] transport-helper: initialize debug flag before starting threads

[RFC/PATCH] transport-helper: initialize debug flag before starting threads

[Jeff King]

The transport_debug() function uses a static variable to
lazily cache the boolean value of $TRANSPORT_DEBUG. If a
program uses transport-helper's bidirectional_transfer_loop
(as remote-ext and remote-fd do), then two threads may both
try to write the variable at the same time.

We can fix this by initializing the variable right before
starting the threads.

Noticed by "-fsanitize=thread". This probably isn't a
problem in practice, as both threads will write the same
value, and we are unlikely to see a torn write on an "int"
(i.e., on sane platforms a write to an int is atomic).

Signed-off-by: Jeff King <peff@peff.net>
---
I'm playing around with -fsanitize=thread and found this. The fix isn't
_too_ bad, but it's not the only case.

For example, "grep" may spawn many threads, each of which calls
want_color(), which does the same static cache-the-env trick. Should
grep call want_color() preemptively before staring threads? That's also
not too bad, but we're starting to cross a lot of module boundaries here
(i.e., it's a bit gross that grep needs to know how want_color() is
implemented).

As noted above, I think these cases are pretty benign. But it looks like
-fsanitize=thread would be a good way to find cases that are not (e.g.,
places in index-pack where we need to take the global lock but don't),
and it would be nice to keep things clean. The options for doing so (and
hence the RFC) are:

  1. Fixes like this (or grep calling want_color preemptively0. I'm not
     sure yet how many would be necessary, but probably a handful.

  2. Annotate sites like this (a single int read/write where all threads
     should get the same value) with "it's OK to race here" markers.

  3. Introduce locking into each site. This has performance
     implications, which makes me hesitate if this isn't a problem in
     practice. There's probably a simple lockless solution, though.

I think I'd favor (2) if we don't mind polluting our code with such
annotations. I haven't investigated exactly what they would look like,
though (there is a "dynamic annotation" that is pretty gross, as it
requires linking with extra code; but I think there may also be a
valgrind-suppression-like scheme that can live outside the code
completely).

 transport-helper.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 0224687..fc3756c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1058,17 +1058,23 @@ int transport_helper_init(struct transport *transport, const char *name)
 /* This should be enough to hold debugging message. */
 #define PBUFFERSIZE 8192
 
+static int transport_debug_enabled(void)
+{
+	static int debug_enabled = -1;
+
+	if (debug_enabled < 0)
+		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
+	return debug_enabled;
+}
+
 /* Print bidirectional transfer loop debug message. */
 __attribute__((format (printf, 1, 2)))
 static void transfer_debug(const char *fmt, ...)
 {
 	va_list args;
 	char msgbuf[PBUFFERSIZE];
-	static int debug_enabled = -1;
 
-	if (debug_enabled < 0)
-		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
-	if (!debug_enabled)
+	if (!transport_debug_enabled)
 		return;
 
 	va_start(args, fmt);
@@ -1237,6 +1243,10 @@ static int tloop_spawnwait_tasks(struct bidirectional_transfer_state *s)
 	pthread_t ptg_thread;
 	int err;
 	int ret = 0;
+
+	/* initialize static global debug flag as a side effect */
+	transport_debug_enabled();
+
 	err = pthread_create(&gtp_thread, NULL, udt_copy_task_routine,
 		&s->gtp);
 	if (err)
-- 
2.2.0.390.gf60752d

Re: [RFC/PATCH] transport-helper: initialize debug flag before starting threads

[Jeff King]

On Mon, Dec 08, 2014 at 03:22:06AM -0500, Jeff King wrote:

>  /* Print bidirectional transfer loop debug message. */
>  __attribute__((format (printf, 1, 2)))
>  static void transfer_debug(const char *fmt, ...)
>  {
>  	va_list args;
>  	char msgbuf[PBUFFERSIZE];
> -	static int debug_enabled = -1;
>  
> -	if (debug_enabled < 0)
> -		debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
> -	if (!debug_enabled)
> +	if (!transport_debug_enabled)
>  		return;

I think in my cover letter I made clear that this was not really meant
for inclusion as-is, but just in case: this obviously is missing "()" at
the end of the function call (interestingly gcc -Wall complains, but
clang does not).

-Peff