On 20 August 2017 at 12:06, Jeff King <peff@xxxxxxxx> wrote: > On Tue, Aug 15, 2017 at 02:53:00PM +0200, Martin Ågren wrote: > >> I tried running the test suite on Git compiled with ThreadSanitizer >> (SANITIZE=thread). Maybe this series could be useful for someone else >> trying to do the same. I needed the first patch to avoid warnings when >> compiling, although it actually has nothing to do with threads. >> >> The last four patches are about avoiding some issues where >> ThreadSanitizer complains for reasonable reasons, but which to the best >> of my understanding are not real problems. These patches could be useful >> to make "actual" problems stand out more. Of course, if no-one ever runs >> ThreadSanitizer, they are of little to no (or even negative) value... > > I think it's a chicken-and-egg. I'd love to run the test suite with tsan > from time to time, but there's no point if it turns up a bunch of false > positives. > > The general direction here looks good to me (and I agree with the > comments made so far, especially that we should stop writing to > strbuf_slopbuf entirely). > >> ThreadSanitizer: add suppressions > > This one is the most interesting because it really is just papering over > the issues. I "solved" the transfer_debug one with actual code in: > > https://public-inbox.org/git/20170710133040.yom65mjol3nmf2it@xxxxxxxxxxxxxxxxxxxxx/ Hmm, I did search for related posts, but obviously not good enough. Sorry that I missed this. > but it just feels really dirty. I'd be inclined to go with suppressions > for now until somebody can demonstrate or argue for an actual breakage > (just because it makes the tool more useful for finding _real_ > problems). I actually had a more intrusive version of my patch, but which I didn't send, where I extracted transport_debug_enabled() exactly like that, except I did it in order to minimize the scope of the suppression. In the end, I figured the scope was already small enough. The obvious risk of introducing any kind of "temporary" suppression is that it remains forever... :-) I think I'll add a couple of NEEDSWORK in the code to make it a bit more likely that someone stumbles over the problem and addresses it. Thanks.
