Merge branch 'ma/ts-cleanups' into next

Assorted bugfixes and clean-ups.

* ma/ts-cleanups:
ThreadSanitizer: add suppressions
strbuf_setlen: don't write to strbuf_slopbuf
pack-objects: take lock before accessing `remaining`
convert: always initialize attr_action in convert_attrs

diff --git a/.tsan-suppressions b/.tsan-suppressions
new file mode 100644 (file)
index 0000000..8c85014
--- /dev/null
+++ b/.tsan-suppressions
@@ -0,0 +1,10 @@
+# Suppressions for ThreadSanitizer (tsan).
+#
+# This file is used by setting the environment variable TSAN_OPTIONS to, e.g.,
+# "suppressions=$(pwd)/.tsan-suppressions". Observe that relative paths such as
+# ".tsan-suppressions" might not work.
+
+# A static variable is written to racily, but we always write the same value, so
+# in practice it (hopefully!) doesn't matter.
+race:^want_color$
+race:^transfer_debug$

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 82ad6e0c813375b480129c22035afc4d6674992a..a57b4f058dff3ceec80355560bf6a856f130b8c0 100644 (file)
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2171,7 +2171,10 @@ static void *threaded_find_deltas(void *arg)
 {
        struct thread_params *me = arg;
 
+       progress_lock();
        while (me->remaining) {
+               progress_unlock();
+
                find_deltas(me->list, &me->remaining,
                            me->window, me->depth, me->processed);
 
@@ -2193,7 +2196,10 @@ static void *threaded_find_deltas(void *arg)
                        pthread_cond_wait(&me->cond, &me->mutex);
                me->data_ready = 0;
                pthread_mutex_unlock(&me->mutex);
+
+               progress_lock();
        }
+       progress_unlock();
        /* leave ->working 1 so that this doesn't get more work assigned */
        return NULL;
 }

diff --git a/color.c b/color.c
index 7aa8b076f045e5c0b74826153030e2923c0fc56e..9ccd954d6ba5cea1dd275cac23bb19966b9c6386 100644 (file)
--- a/color.c
+++ b/color.c
@@ -338,6 +338,13 @@ static int check_auto_color(void)
 
 int want_color(int var)
 {
+       /*
+        * NEEDSWORK: This function is sometimes used from multiple threads, and
+        * we end up using want_auto racily. That "should not matter" since
+        * we always write the same value, but it's still wrong. This function
+        * is listed in .tsan-suppressions for the time being.
+        */
+
        static int want_auto = -1;
 
        if (var < 0)

diff --git a/convert.c b/convert.c
index c5f0b210370c0ee83ef59298fa2260d627332399..02962261c58f71485a27f9799a5db29ec77e0bc7 100644 (file)
--- a/convert.c
+++ b/convert.c
@@ -1040,7 +1040,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
                ca->crlf_action = git_path_check_crlf(ccheck + 4);
                if (ca->crlf_action == CRLF_UNDEFINED)
                        ca->crlf_action = git_path_check_crlf(ccheck + 0);
-               ca->attr_action = ca->crlf_action;
                ca->ident = git_path_check_ident(ccheck + 1);
                ca->drv = git_path_check_convert(ccheck + 2);
                if (ca->crlf_action != CRLF_BINARY) {
@@ -1054,12 +1053,14 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
                        else if (eol_attr == EOL_CRLF)
                                ca->crlf_action = CRLF_TEXT_CRLF;
                }
-               ca->attr_action = ca->crlf_action;
        } else {
                ca->drv = NULL;
                ca->crlf_action = CRLF_UNDEFINED;
                ca->ident = 0;
        }
+
+       /* Save attr and make a decision for action */
+       ca->attr_action = ca->crlf_action;
        if (ca->crlf_action == CRLF_TEXT)
                ca->crlf_action = text_eol_is_crlf() ? CRLF_TEXT_CRLF : CRLF_TEXT_INPUT;
        if (ca->crlf_action == CRLF_UNDEFINED && auto_crlf == AUTO_CRLF_FALSE)

diff --git a/strbuf.h b/strbuf.h
index e705b94db55578aabb2063f7364220978cea5a40..7496cb8ec5a1f8baeda90f8653b2fcbbe2527390 100644 (file)
--- a/strbuf.h
+++ b/strbuf.h
@@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
        if (len > (sb->alloc ? sb->alloc - 1 : 0))
                die("BUG: strbuf_setlen() beyond buffer");
        sb->len = len;
-       sb->buf[len] = '\0';
+       if (sb->buf != strbuf_slopbuf)
+               sb->buf[len] = '\0';
+       else
+               assert(!strbuf_slopbuf[0]);
 }
 
 /**

diff --git a/transport-helper.c b/transport-helper.c
index 8f68d69a86bd919162b62514af0b1d1d55e204a1..f50b34df2dba4827db6a96cf97d1c4ac7ca811a0 100644 (file)
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1117,6 +1117,13 @@ int transport_helper_init(struct transport *transport, const char *name)
 __attribute__((format (printf, 1, 2)))
 static void transfer_debug(const char *fmt, ...)
 {
+       /*
+        * NEEDSWORK: This function is sometimes used from multiple threads, and
+        * we end up using debug_enabled racily. That "should not matter" since
+        * we always write the same value, but it's still wrong. This function
+        * is listed in .tsan-suppressions for the time being.
+        */
+
        va_list args;
        char msgbuf[PBUFFERSIZE];
        static int debug_enabled = -1;