{"thread":{"id":"38140","subject":"[RFC/PATCH] transport-helper: initialize debug flag before starting threads","startedAt":"2014-12-08T08:22:06Z","lastAt":"2014-12-08T10:36:24Z","messageCount":2,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"253295","messageId":"20141208082206.GA28302@peff.net","threadId":"38140","inReplyTo":null,"subject":"[RFC/PATCH] transport-helper: initialize debug flag before starting threads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-08T08:22:06Z","receivedAt":"2014-12-08T08:22:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The transport_debug() function uses a static variable to\nlazily cache the boolean value of $TRANSPORT_DEBUG. If a\nprogram uses transport-helper's bidirectional_transfer_loop\n(as remote-ext and remote-fd do), then two threads may both\ntry to write the variable at the same time.\n\nWe can fix this by initializing the variable right before\nstarting the threads.\n\nNoticed by \"-fsanitize=thread\". This probably isn't a\nproblem in practice, as both threads will write the same\nvalue, and we are unlikely to see a torn write on an \"int\"\n(i.e., on sane platforms a write to an int is atomic).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm playing around with -fsanitize=thread and found this. The fix isn't\n_too_ bad, but it's not the only case.\n\nFor example, \"grep\" may spawn many threads, each of which calls\nwant_color(), which does the same static cache-the-env trick. Should\ngrep call want_color() preemptively before staring threads? That's also\nnot too bad, but we're starting to cross a lot of module boundaries here\n(i.e., it's a bit gross that grep needs to know how want_color() is\nimplemented).\n\nAs noted above, I think these cases are pretty benign. But it looks like\n-fsanitize=thread would be a good way to find cases that are not (e.g.,\nplaces in index-pack where we need to take the global lock but don't),\nand it would be nice to keep things clean. The options for doing so (and\nhence the RFC) are:\n\n  1. Fixes like this (or grep calling want_color preemptively0. I'm not\n     sure yet how many would be necessary, but probably a handful.\n\n  2. Annotate sites like this (a single int read/write where all threads\n     should get the same value) with \"it's OK to race here\" markers.\n\n  3. Introduce locking into each site. This has performance\n     implications, which makes me hesitate if this isn't a problem in\n     practice. There's probably a simple lockless solution, though.\n\nI think I'd favor (2) if we don't mind polluting our code with such\nannotations. I haven't investigated exactly what they would look like,\nthough (there is a \"dynamic annotation\" that is pretty gross, as it\nrequires linking with extra code; but I think there may also be a\nvalgrind-suppression-like scheme that can live outside the code\ncompletely).\n\n transport-helper.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 0224687..fc3756c 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1058,17 +1058,23 @@ int transport_helper_init(struct transport *transport, const char *name)\n /* This should be enough to hold debugging message. */\n #define PBUFFERSIZE 8192\n \n+static int transport_debug_enabled(void)\n+{\n+\tstatic int debug_enabled = -1;\n+\n+\tif (debug_enabled < 0)\n+\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n+\treturn debug_enabled;\n+}\n+\n /* Print bidirectional transfer loop debug message. */\n __attribute__((format (printf, 1, 2)))\n static void transfer_debug(const char *fmt, ...)\n {\n \tva_list args;\n \tchar msgbuf[PBUFFERSIZE];\n-\tstatic int debug_enabled = -1;\n \n-\tif (debug_enabled < 0)\n-\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n-\tif (!debug_enabled)\n+\tif (!transport_debug_enabled)\n \t\treturn;\n \n \tva_start(args, fmt);\n@@ -1237,6 +1243,10 @@ static int tloop_spawnwait_tasks(struct bidirectional_transfer_state *s)\n \tpthread_t ptg_thread;\n \tint err;\n \tint ret = 0;\n+\n+\t/* initialize static global debug flag as a side effect */\n+\ttransport_debug_enabled();\n+\n \terr = pthread_create(&gtp_thread, NULL, udt_copy_task_routine,\n \t\t&s->gtp);\n \tif (err)\n-- \n2.2.0.390.gf60752d\n"},{"id":"253297","messageId":"20141208103624.GA11737@peff.net","threadId":"38140","inReplyTo":"20141208082206.GA28302@peff.net","subject":"Re: [RFC/PATCH] transport-helper: initialize debug flag before starting threads","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-08T10:36:24Z","receivedAt":"2014-12-08T10:36:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 08, 2014 at 03:22:06AM -0500, Jeff King wrote:\n\n>  /* Print bidirectional transfer loop debug message. */\n>  __attribute__((format (printf, 1, 2)))\n>  static void transfer_debug(const char *fmt, ...)\n>  {\n>  \tva_list args;\n>  \tchar msgbuf[PBUFFERSIZE];\n> -\tstatic int debug_enabled = -1;\n>  \n> -\tif (debug_enabled < 0)\n> -\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n> -\tif (!debug_enabled)\n> +\tif (!transport_debug_enabled)\n>  \t\treturn;\n\nI think in my cover letter I made clear that this was not really meant\nfor inclusion as-is, but just in case: this obviously is missing \"()\" at\nthe end of the function call (interestingly gcc -Wall complains, but\nclang does not).\n\n-Peff\n"}]}