{"thread":{"id":"62711","subject":"[PATCH 0/5] fixing thread races in linux-leaks CI","startedAt":"2024-12-30T04:23:29Z","lastAt":"2025-01-03T06:45:51Z","messageCount":11,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"509714","messageId":"20241230042325.GA112439@coredump.intra.peff.net","threadId":"62711","inReplyTo":null,"subject":"[PATCH 0/5] fixing thread races in linux-leaks CI","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-30T04:23:25Z","receivedAt":"2024-12-30T04:23:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series should fix the races we see in linux-leaks CI jobs. Or at\nleast some of them. We tried previously in 993d38a066 (index-pack: spawn\nthreads atomically, 2024-01-05), but it wasn't enough. This series\ncovers index-pack as well as git-grep, which are the two I've seen in\npractice. We may run into more, but the general pattern should be\napplicable if we do.\n\nThis series takes the minimal approach, and only provides a knob to use\npthread_barrier_t if your system supports it (and then turns it on for\nour CI jobs). The obvious alternative is to actually implement wrappers\nfor other systems, and then use it everywhere. That doesn't buy us much\nfor this problem, but it's possible we'd find a use for barriers\nelsewhere.\n\nThe even farther alternatives are:\n\n  1. Do nothing. This is arguably an LSan bug, which should be doing its\n     own locking to synchronize the at-exit leak check with the\n     per-thread setup code. I don't think we've even reported it there,\n     so one option would be to work with them. Even if we do that, it\n     might be nice to remove the false positive pain in the meantime\n     with a series like this (and certainly patches 1 and 2 here are\n     worth applying independently).\n\n  2. Change our posture on thread exit. In index-pack, the issue is that\n     one worker thread calls exit() via die(), taking down the whole\n     program (including other worker threads in unknown states). If it\n     installed a die() handler that called pthread_exit() instead, then\n     the main thread could see that failure and cancel/join the\n     remaining threads.\n\n     But I suspect it's not so simple:\n\n       a. We still want the error in the worker thread to cancel ongoing\n\t  work immediately. Right now index-pack just called\n\t  pthread_join() in sequence to wait for all threads to finish.\n\t  But instead we'd need to use some synchronization primitives\n\t  to report the error. Probably not too hard, but new\n\t  potentially tricky code.\n\n       b. In the git-grep case, it's actually the main thread that calls\n\t  die(). So the rule is not really \"threads should install a die\n\t  handler that calls pthread_exit()\", but _any_ die() call when\n\t  threads are established would need to cleanly ask all threads\n\t  to stop (whether by signaling them via the work queue, or just\n\t  hitting them with pthread_cancel; and that's assuming that\n\t  there's no race between LSan's setup code and cancel).\n\n       c. When we call die(), all bets are off about what state various\n          data structures are in. For example, a thread could be holding\n\t  a lock, and if it's cancelled by another thread, that lock\n\t  will remain. So it's not safe to do any real work in the other\n\t  threads, unless we start setting up pthread_cleanup handlers,\n\t  etc. That seems like a recipe for obscure, racy deadlocks.\n\n     I think that may be an appealing direction in the long run,\n     especially since die() itself is not thread-safe. But coupled with\n     libification, we probably want less \"threads can die() cleanly\" and\n     more \"threads pass errors up the stack and report the problem back\n     to the work queue\". Either way, though, it's a much bigger approach\n     change than I think we want just to try to address LSan races.\n\nSo I think we want something like this (either this, or the variant\nwhere we really implement barriers everywhere) in the near-term.\n\n  [1/5]: test-lib: use individual lsan dir for --stress runs\n  [2/5]: Revert \"index-pack: spawn threads atomically\"\n  [3/5]: thread-utils: introduce optional barrier type\n  [4/5]: index-pack: work around LSan threading race with barrier\n  [5/5]: grep: work around LSan threading race with barrier\n\n Makefile             |  7 +++++++\n builtin/grep.c       |  8 ++++++++\n builtin/index-pack.c |  8 ++++++--\n ci/lib.sh            |  1 +\n t/test-lib.sh        |  2 +-\n thread-utils.h       | 17 +++++++++++++++++\n 6 files changed, 40 insertions(+), 3 deletions(-)\n"},{"id":"509715","messageId":"20241230042401.GA113400@coredump.intra.peff.net","threadId":"62711","inReplyTo":"20241230042325.GA112439@coredump.intra.peff.net","subject":"[PATCH 1/5] test-lib: use individual lsan dir for --stress runs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-30T04:24:01Z","receivedAt":"2024-12-30T04:24:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When storing output in test-results/, we usually give each numbered run\nin a --stress set its own output file. But we don't do that for storing\nLSan logs, so something like:\n\n  ./t0003-attributes.sh --stress\n\nwill have many scripts simultaneously creating, writing to, and deleting\nthe test-results/t0003-attributes.leak directory. This can cause logs\nfrom one run to be attributed to another, spurious failures when\ncreation and deletion race, and so on.\n\nThis has always been broken, but nobody noticed because it's rare to do\na --stress run with LSan (since the point is for the code to run quickly\nmany times in order to hit races). But if you're trying to find a race\nin the leak sanitizing code, it makes sense to use these together.\n\nWe can fix it by using $TEST_RESULTS_BASE, which already incorporates\nthe stress job suffix.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 1a67adb207..96f2dfb69d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -331,7 +331,7 @@ TEST_RESULTS_BASE=\"$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX\"\n TEST_RESULTS_SAN_FILE_PFX=trace\n TEST_RESULTS_SAN_DIR_SFX=leak\n TEST_RESULTS_SAN_FILE=\n-TEST_RESULTS_SAN_DIR=\"$TEST_RESULTS_DIR/$TEST_NAME.$TEST_RESULTS_SAN_DIR_SFX\"\n+TEST_RESULTS_SAN_DIR=\"$TEST_RESULTS_BASE.$TEST_RESULTS_SAN_DIR_SFX\"\n TRASH_DIRECTORY=\"trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX\"\n test -n \"$root\" && TRASH_DIRECTORY=\"$root/$TRASH_DIRECTORY\"\n case \"$TRASH_DIRECTORY\" in\n-- \nbar\n\n"},{"id":"509716","messageId":"20241230042610.GB113400@coredump.intra.peff.net","threadId":"62711","inReplyTo":"20241230042325.GA112439@coredump.intra.peff.net","subject":"[PATCH 2/5] Revert \"index-pack: spawn threads atomically\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-30T04:26:10Z","receivedAt":"2024-12-30T04:26:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit 993d38a0669a8056d496797516e743e26b6b8b54.\n\nThat commit was trying to solve a race between LSan setting up the\nthreads stack and another thread calling exit(), by making sure that all\npthread_create() calls have finished before doing any work that might\ntrigger the exit().\n\nBut that isn't sufficient. The setup code actually runs in the\nindividual threads themselves, not in the spawning thread's call to\npthread_create(). So while it may have improved the race a bit, you can\nstill trigger it pretty quickly with:\n\n  make SANITIZE=leak\n  cd t\n  ./t5309-pack-delta-cycles.sh --stress\n\nLet's back out that failed attempt so we can try again.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm still puzzled why I ever thought 93d38a066 solved this, because the\nstress test above is exactly what I was using back then to test it.\nPossibly LSan changed in some way, or possibly I just got lucky in my\nstress run, but the more likely reason is that I simply bungled the\ntesting (e.g., if you accidentally use a non-LSan build, then it\nnaturally appears to work).\n\n builtin/index-pack.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex d773809c4c..0b62b2589f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1336,15 +1336,13 @@ static void resolve_deltas(struct pack_idx_option *opts)\n \tbase_cache_limit = opts->delta_base_cache_limit * nr_threads;\n \tif (nr_threads > 1 || getenv(\"GIT_FORCE_THREADS\")) {\n \t\tinit_thread();\n-\t\twork_lock();\n \t\tfor (i = 0; i < nr_threads; i++) {\n \t\t\tint ret = pthread_create(&thread_data[i].thread, NULL,\n \t\t\t\t\t\t threaded_second_pass, thread_data + i);\n \t\t\tif (ret)\n \t\t\t\tdie(_(\"unable to create thread: %s\"),\n \t\t\t\t    strerror(ret));\n \t\t}\n-\t\twork_unlock();\n \t\tfor (i = 0; i < nr_threads; i++)\n \t\t\tpthread_join(thread_data[i].thread, NULL);\n \t\tcleanup_thread();\n-- \nbar\n\n"},{"id":"509717","messageId":"20241230042830.GC113400@coredump.intra.peff.net","threadId":"62711","inReplyTo":"20241230042325.GA112439@coredump.intra.peff.net","subject":"[PATCH 3/5] thread-utils: introduce optional barrier type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-30T04:28:30Z","receivedAt":"2024-12-30T04:28:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One thread primitive we don't yet support is a barrier: it waits for all\nthreads to reach a synchronization point before letting any of them\ncontinue. This would be useful for avoiding the LSan race we see in\nindex-pack (and other places) by having all threads complete their\ninitialization before any of them start to do real work.\n\nPOSIX introduced a pthread_barrier_t in 2004, which does what we want.\nBut if we want to rely on it:\n\n  1. Our Windows pthread emulation would need a new set of wrapper\n     functions. There's a Synchronization Barrier primitive there, which\n     was introduced in Windows 8 (which is old enough for us to depend\n     on).\n\n  2. macOS (and possibly other systems) has pthreads but not\n     pthread_barrier_t. So there we'd have to implement our own barrier\n     based on the mutex and cond primitives.\n\nThose are do-able, but since we only care about avoiding races in our\nLSan builds, there's an easier way: make it a noop on systems without a\nnative pthread barrier.\n\nThis patch introduces a \"maybe_thread_barrier\" API. The clunky name\n(rather than just using pthread_barrier directly) should hopefully clue\npeople in that on some systems it will do nothing. It's wired to a\nMakefile knob which has to be triggered manually, and we enable it for\nthe linux-leaks CI jobs (since we know we'll have it there).\n\nThere are some other possible options:\n\n  - we could turn it on all the time for Linux systems based on uname.\n    But we really only care about it for LSan builds, and there is no\n    need to add extra code to regular builds.\n\n  - we could turn it on only for LSan builds. But that would break\n    builds on non-Linux platforms (like macOS) that otherwise should\n    support sanitizers.\n\n  - we could trigger only on the combination of Linux and LSan together.\n    This isn't too hard to do, but the uname check isn't completely\n    accurate. It is really about what your libc supports, and non-glibc\n    systems might not have it (though at least musl seems to).\n\n    So we'd risk breaking builds on those systems, which would need to\n    add a new knob. Though the upside would be that running local \"make\n    SANITIZE=leak test\" would be protected automatically.\n\nAnd of course none of this protects LSan runs from races on systems\nwithout pthread barriers. It's probably OK in practice to protect only\nour CI jobs, though. The race is rare-ish and most leak-checking happens\nthrough CI.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile       |  7 +++++++\n ci/lib.sh      |  1 +\n thread-utils.h | 17 +++++++++++++++++\n 3 files changed, 25 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 97e8385b66..2c6dad8a75 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -141,6 +141,10 @@ include shared.mak\n #\n # Define NO_PTHREADS if you do not have or do not want to use Pthreads.\n #\n+# Define THREAD_BARRIER_PTHREAD if your system has pthread_barrier_t. Barrier\n+# support is optional and is only helpful when building with SANITIZE=leak, as\n+# it is used to eliminate some races in the leak-checker.\n+#\n # Define NO_PREAD if you have a problem with pread() system call (e.g.\n # cygwin1.dll before v1.5.22).\n #\n@@ -2079,6 +2083,9 @@ ifdef NO_PTHREADS\n else\n \tBASIC_CFLAGS += $(PTHREAD_CFLAGS)\n \tEXTLIBS += $(PTHREAD_LIBS)\n+\tifdef THREAD_BARRIER_PTHREAD\n+\t\tBASIC_CFLAGS += -DTHREAD_BARRIER_PTHREAD\n+\tendif\n endif\n \n ifdef HAVE_PATHS_H\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 8885ee3c3f..6a1267fbcb 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -385,6 +385,7 @@ linux-musl)\n \t;;\n linux-leaks|linux-reftable-leaks)\n \texport SANITIZE=leak\n+\texport THREAD_BARRIER_PTHREAD=1\n \t;;\n linux-asan-ubsan)\n \texport SANITIZE=address,undefined\ndiff --git a/thread-utils.h b/thread-utils.h\nindex 4961487ed9..3df5be9916 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -53,5 +53,22 @@ int dummy_pthread_init(void *);\n int online_cpus(void);\n int init_recursive_mutex(pthread_mutex_t*);\n \n+#ifdef THREAD_BARRIER_PTHREAD\n+#define maybe_thread_barrier_t pthread_barrier_t\n+#define maybe_thread_barrier_init pthread_barrier_init\n+#define maybe_thread_barrier_wait pthread_barrier_wait\n+#define maybe_thread_barrier_destroy pthread_barrier_destroy\n+#else\n+#define maybe_thread_barrier_t int\n+static inline int maybe_thread_barrier_init(maybe_thread_barrier_t *b UNUSED,\n+\t\t\t\t\t    void *attr UNUSED,\n+\t\t\t\t\t    unsigned nr UNUSED)\n+{\n+\terrno = ENOSYS;\n+\treturn -1;\n+}\n+#define maybe_thread_barrier_wait(barrier)\n+#define maybe_thread_barrier_destroy(barrier)\n+#endif\n \n #endif /* THREAD_COMPAT_H */\n-- \nbar\n\n"},{"id":"509718","messageId":"20241230042938.GD113400@coredump.intra.peff.net","threadId":"62711","inReplyTo":"20241230042325.GA112439@coredump.intra.peff.net","subject":"[PATCH 4/5] index-pack: work around LSan threading race with barrier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-30T04:29:38Z","receivedAt":"2024-12-30T04:29:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We sometimes get false positives from our linux-leaks CI job because of\na race in LSan itself. The problem is that one thread is still\ninitializing its stack in LSan's code (and allocating memory to do so)\nwhile anothe thread calls die(), taking down the whole process and\ntriggering a leak check.\n\nThe problem is described in more detail in 993d38a066 (index-pack: spawn\nthreads atomically, 2024-01-05), which tried to fix it by pausing worker\nthreads until all calls to pthread_create() had completed. But that's\nnot enough to fix the problem, because the LSan setup code runs in the\nthreads themselves. So even though pthread_create() has returned, we\nhave no idea if all threads actually finished their setup before letting\nany of them do real work.\n\nWe can fix that by using a barrier inside the threads themselves,\nwaiting for all of them to hit the start of their main function before\nany of them proceed.\n\nYou can test for the race by running:\n\n  make SANITIZE=leak THREAD_BARRIER_PTHREAD=YesOnLinux\n  cd t\n  ./t5309-pack-delta-cycles.sh --stress\n\nwhich fails quickly before this patch, and should run indefinitely\nwithout it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 0b62b2589f..27b120f26c 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -185,6 +185,8 @@ static pthread_mutex_t deepest_delta_mutex;\n \n static pthread_key_t key;\n \n+static maybe_thread_barrier_t start_barrier;\n+\n static inline void lock_mutex(pthread_mutex_t *mutex)\n {\n \tif (threads_active)\n@@ -209,6 +211,7 @@ static void init_thread(void)\n \tif (show_stat)\n \t\tpthread_mutex_init(&deepest_delta_mutex, NULL);\n \tpthread_key_create(&key, NULL);\n+\tmaybe_thread_barrier_init(&start_barrier, NULL, nr_threads);\n \tCALLOC_ARRAY(thread_data, nr_threads);\n \tfor (i = 0; i < nr_threads; i++) {\n \t\tthread_data[i].pack_fd = xopen(curr_pack, O_RDONLY);\n@@ -231,6 +234,7 @@ static void cleanup_thread(void)\n \tfor (i = 0; i < nr_threads; i++)\n \t\tclose(thread_data[i].pack_fd);\n \tpthread_key_delete(key);\n+\tmaybe_thread_barrier_destroy(&start_barrier);\n \tfree(thread_data);\n }\n \n@@ -1100,6 +1104,8 @@ static int compare_ref_delta_entry(const void *a, const void *b)\n \n static void *threaded_second_pass(void *data)\n {\n+\tif (threads_active)\n+\t\tmaybe_thread_barrier_wait(&start_barrier);\n \tif (data)\n \t\tset_thread_data(data);\n \tfor (;;) {\n-- \nbar\n\n"},{"id":"509719","messageId":"20241230043026.GE113400@coredump.intra.peff.net","threadId":"62711","inReplyTo":"20241230042325.GA112439@coredump.intra.peff.net","subject":"[PATCH 5/5] grep: work around LSan threading race with barrier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-30T04:30:26Z","receivedAt":"2024-12-30T04:30:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's a race with LSan when spawning threads and one of the threads\ncalls die(). We worked around one such problem with index-pack in the\nprevious commit, but it exists in git-grep, too. You can see it with:\n\n  make SANITIZE=leak THREAD_BARRIER_PTHREAD=YesOnLinux\n  cd t\n  ./t0003-attributes.sh --stress\n\nwhich fails pretty quickly with:\n\n  ==git==4096424==ERROR: LeakSanitizer: detected memory leaks\n\n  Direct leak of 32 byte(s) in 1 object(s) allocated from:\n      #0 0x7f906de14556 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n      #1 0x7f906dc9d2c1 in __pthread_getattr_np nptl/pthread_getattr_np.c:180\n      #2 0x7f906de2500d in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150\n      #3 0x7f906de25187 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:614\n      #4 0x7f906de17d18 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:53\n      #5 0x7f906de143a9 in ThreadStartFunc<false> ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:431\n      #6 0x7f906dc9bf51 in start_thread nptl/pthread_create.c:447\n      #7 0x7f906dd1a677 in __clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78\n\nAs with the previous commit, we can fix this by inserting a barrier that\nmakes sure all threads have finished their setup before continuing. But\nthere's one twist in this case: the thread which calls die() is not one\nof the worker threads, but the main thread itself!\n\nSo we need the main thread to wait in the barrier, too, until all\nthreads have gotten to it. And thus we initialize the barrier for\nnum_threads+1, to account for all of the worker threads plus the main\none.\n\nIf we then test as above, t0003 should run indefinitely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/grep.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d00ee76f24..61b2c27490 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -101,6 +101,9 @@ static pthread_cond_t cond_write;\n /* Signalled when we are finished with everything. */\n static pthread_cond_t cond_result;\n \n+/* Synchronize the start of all threads */\n+static maybe_thread_barrier_t start_barrier;\n+\n static int skip_first_line;\n \n static void add_work(struct grep_opt *opt, struct grep_source *gs)\n@@ -198,6 +201,8 @@ static void *run(void *arg)\n \tint hit = 0;\n \tstruct grep_opt *opt = arg;\n \n+\tmaybe_thread_barrier_wait(&start_barrier);\n+\n \twhile (1) {\n \t\tstruct work_item *w = get_work();\n \t\tif (!w)\n@@ -229,6 +234,7 @@ static void start_threads(struct grep_opt *opt)\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n \tpthread_cond_init(&cond_result, NULL);\n+\tmaybe_thread_barrier_init(&start_barrier, NULL, num_threads + 1);\n \tgrep_use_locks = 1;\n \tenable_obj_read_lock();\n \n@@ -248,6 +254,7 @@ static void start_threads(struct grep_opt *opt)\n \t\t\tdie(_(\"grep: failed to create thread: %s\"),\n \t\t\t    strerror(err));\n \t}\n+\tmaybe_thread_barrier_wait(&start_barrier);\n }\n \n static int wait_all(void)\n@@ -284,6 +291,7 @@ static int wait_all(void)\n \tpthread_cond_destroy(&cond_add);\n \tpthread_cond_destroy(&cond_write);\n \tpthread_cond_destroy(&cond_result);\n+\tmaybe_thread_barrier_destroy(&start_barrier);\n \tgrep_use_locks = 0;\n \tdisable_obj_read_lock();\n \n-- \nbar\n"},{"id":"509721","messageId":"20241230043419.GB112402@coredump.intra.peff.net","threadId":"62711","inReplyTo":"20241230042401.GA113400@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] test-lib: use individual lsan dir for --stress runs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-30T04:34:19Z","receivedAt":"2024-12-30T04:34:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 29, 2024 at 11:24:01PM -0500, Jeff King wrote:\n\n> -- \n> bar\n\nBTW, in case anybody is curious about this, it's because I still had:\n\n  GIT_VERSION = bar\n\nin my config.mak from testing Patrick's GIT-VERSION-GEN series. :)\n\nNaturally \"foo\" was taken from testing:\n\n  make GIT_VERSION=foo\n\n-Peff\n"},{"id":"509723","messageId":"Z3JFyniivEd716aX@pks.im","threadId":"62711","inReplyTo":"20241230042830.GC113400@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] thread-utils: introduce optional barrier type","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T07:03:38Z","receivedAt":"2024-12-30T07:03:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Dec 29, 2024 at 11:28:30PM -0500, Jeff King wrote:\n> One thread primitive we don't yet support is a barrier: it waits for all\n> threads to reach a synchronization point before letting any of them\n> continue. This would be useful for avoiding the LSan race we see in\n> index-pack (and other places) by having all threads complete their\n> initialization before any of them start to do real work.\n> \n> POSIX introduced a pthread_barrier_t in 2004, which does what we want.\n> But if we want to rely on it:\n> \n>   1. Our Windows pthread emulation would need a new set of wrapper\n>      functions. There's a Synchronization Barrier primitive there, which\n>      was introduced in Windows 8 (which is old enough for us to depend\n>      on).\n> \n>   2. macOS (and possibly other systems) has pthreads but not\n>      pthread_barrier_t. So there we'd have to implement our own barrier\n>      based on the mutex and cond primitives.\n> \n> Those are do-able, but since we only care about avoiding races in our\n> LSan builds, there's an easier way: make it a noop on systems without a\n> native pthread barrier.\n\nI think this is fine for a first iteration. If we ever feel the need for\nhaving barriers anywhere else for actual correctness we can iterate on\nthe solution and provide wrappers for those platforms.\n\n> This patch introduces a \"maybe_thread_barrier\" API. The clunky name\n> (rather than just using pthread_barrier directly) should hopefully clue\n> people in that on some systems it will do nothing. It's wired to a\n> Makefile knob which has to be triggered manually, and we enable it for\n> the linux-leaks CI jobs (since we know we'll have it there).\n> \n> There are some other possible options:\n> \n>   - we could turn it on all the time for Linux systems based on uname.\n\nTiniest of nits: these are all full sentences, which should start with\nan upper-case letter.\n\n>     But we really only care about it for LSan builds, and there is no\n>     need to add extra code to regular builds.\n> \n>   - we could turn it on only for LSan builds. But that would break\n>     builds on non-Linux platforms (like macOS) that otherwise should\n>     support sanitizers.\n\nMh. I'm not a huge fan of having extra code for just a subset of our\nbuilds and think that having the code generally enabled on platforms\nthat support it is preferable to reduce the number of build variants.\nBut...\n\n>   - we could trigger only on the combination of Linux and LSan together.\n>     This isn't too hard to do, but the uname check isn't completely\n>     accurate. It is really about what your libc supports, and non-glibc\n>     systems might not have it (though at least musl seems to).\n> \n>     So we'd risk breaking builds on those systems, which would need to\n>     add a new knob. Though the upside would be that running local \"make\n>     SANITIZE=leak test\" would be protected automatically.\n\n... this is a fair remark. So I dunno.\n\n> And of course none of this protects LSan runs from races on systems\n> without pthread barriers. It's probably OK in practice to protect only\n> our CI jobs, though. The race is rare-ish and most leak-checking happens\n> through CI.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  Makefile       |  7 +++++++\n>  ci/lib.sh      |  1 +\n>  thread-utils.h | 17 +++++++++++++++++\n>  3 files changed, 25 insertions(+)\n> \n> diff --git a/Makefile b/Makefile\n> index 97e8385b66..2c6dad8a75 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -141,6 +141,10 @@ include shared.mak\n>  #\n>  # Define NO_PTHREADS if you do not have or do not want to use Pthreads.\n>  #\n> +# Define THREAD_BARRIER_PTHREAD if your system has pthread_barrier_t. Barrier\n> +# support is optional and is only helpful when building with SANITIZE=leak, as\n> +# it is used to eliminate some races in the leak-checker.\n> +#\n>  # Define NO_PREAD if you have a problem with pread() system call (e.g.\n>  # cygwin1.dll before v1.5.22).\n>  #\n> @@ -2079,6 +2083,9 @@ ifdef NO_PTHREADS\n>  else\n>  \tBASIC_CFLAGS += $(PTHREAD_CFLAGS)\n>  \tEXTLIBS += $(PTHREAD_LIBS)\n> +\tifdef THREAD_BARRIER_PTHREAD\n> +\t\tBASIC_CFLAGS += -DTHREAD_BARRIER_PTHREAD\n> +\tendif\n>  endif\n>  \n>  ifdef HAVE_PATHS_H\n\nOkay. The Meson equivalent would be:\n\ndiff --git a/meson.build b/meson.build\nindex a0654a3f24..db4c1e6929 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -788,6 +788,10 @@ threads = dependency('threads', required: false)\n if threads.found()\n   libgit_dependencies += threads\n   build_options_config.set('NO_PTHREADS', '')\n+\n+  if get_option('b_sanitize').contains('leak') and compiler.has_function('pthread_barrier_init', dependencies: threads)\n+    libgit_c_args += '-DTHREAD_BARRIER_PTHREAD'\n+  endif\n else\n   libgit_c_args += '-DNO_PTHREADS'\n   build_options_config.set('NO_PTHREADS', '1')\n\n> diff --git a/thread-utils.h b/thread-utils.h\n> index 4961487ed9..3df5be9916 100644\n> --- a/thread-utils.h\n> +++ b/thread-utils.h\n> @@ -53,5 +53,22 @@ int dummy_pthread_init(void *);\n>  int online_cpus(void);\n>  int init_recursive_mutex(pthread_mutex_t*);\n>  \n> +#ifdef THREAD_BARRIER_PTHREAD\n> +#define maybe_thread_barrier_t pthread_barrier_t\n> +#define maybe_thread_barrier_init pthread_barrier_init\n> +#define maybe_thread_barrier_wait pthread_barrier_wait\n> +#define maybe_thread_barrier_destroy pthread_barrier_destroy\n> +#else\n> +#define maybe_thread_barrier_t int\n\nOut of curiosity: why did you pick a define here and not a typedef?\n\n> +static inline int maybe_thread_barrier_init(maybe_thread_barrier_t *b UNUSED,\n> +\t\t\t\t\t    void *attr UNUSED,\n> +\t\t\t\t\t    unsigned nr UNUSED)\n> +{\n> +\terrno = ENOSYS;\n> +\treturn -1;\n> +}\n> +#define maybe_thread_barrier_wait(barrier)\n> +#define maybe_thread_barrier_destroy(barrier)\n\nSo the way these wrappers are implemented it is not possible to check\nfor errors of `pthread_barrier_init()` et al. When the implementation\nexists we do have return codes, but if it's stubbed out we don't.\n\nI think we should align these two implementations so that it does become\npossible to check for errors, or otherwise we wouldn't be using the\npthread APIs correctly. It does raise the question though whether we\nshould really return `-1` in the stubbed-out variant or whether we\nshould instead pretend as if things were alright.\n\nAn alternative would be to die in case the pthread-functions return an\nerror.\n\nPatrick\n"},{"id":"509748","messageId":"xmqqcyh9yu7f.fsf@gitster.g","threadId":"62711","inReplyTo":"20241230042401.GA113400@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] test-lib: use individual lsan dir for --stress runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-30T17:08:36Z","receivedAt":"2024-12-30T17:08:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When storing output in test-results/, we usually give each numbered run\n> in a --stress set its own output file. But we don't do that for storing\n> LSan logs, so something like:\n>\n>   ./t0003-attributes.sh --stress\n>\n> will have many scripts simultaneously creating, writing to, and deleting\n> the test-results/t0003-attributes.leak directory. This can cause logs\n> from one run to be attributed to another, spurious failures when\n> creation and deletion race, and so on.\n>\n> This has always been broken, but nobody noticed because it's rare to do\n> a --stress run with LSan (since the point is for the code to run quickly\n> many times in order to hit races). But if you're trying to find a race\n> in the leak sanitizing code, it makes sense to use these together.\n\nMakes sense.\n\n> We can fix it by using $TEST_RESULTS_BASE, which already incorporates\n> the stress job suffix.\n\nThanks.  Queued.\n"},{"id":"509774","messageId":"20250101182826.GB1391912@coredump.intra.peff.net","threadId":"62711","inReplyTo":"Z3JFyniivEd716aX@pks.im","subject":"Re: [PATCH 3/5] thread-utils: introduce optional barrier type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-01T18:28:26Z","receivedAt":"2025-01-01T18:28:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 30, 2024 at 08:03:38AM +0100, Patrick Steinhardt wrote:\n\n> >   - we could turn it on only for LSan builds. But that would break\n> >     builds on non-Linux platforms (like macOS) that otherwise should\n> >     support sanitizers.\n> \n> Mh. I'm not a huge fan of having extra code for just a subset of our\n> builds and think that having the code generally enabled on platforms\n> that support it is preferable to reduce the number of build variants.\n> But...\n> \n> >   - we could trigger only on the combination of Linux and LSan together.\n> >     This isn't too hard to do, but the uname check isn't completely\n> >     accurate. It is really about what your libc supports, and non-glibc\n> >     systems might not have it (though at least musl seems to).\n> > \n> >     So we'd risk breaking builds on those systems, which would need to\n> >     add a new knob. Though the upside would be that running local \"make\n> >     SANITIZE=leak test\" would be protected automatically.\n> \n> ... this is a fair remark. So I dunno.\n\nYeah. I do not like having the behavior differ only for LSan, or only on\nLinux. But I also do not like having tricky threading code that we do\nnot otherwise need in all of the other builds. So it is really about\npicking the least-bad option, and they all seem similarly bad to me. So\nI picked the one that involved writing the least amount of code. ;)\n\n> Okay. The Meson equivalent would be:\n> [...]\n\nYeah, I figured it would need something similar (but for our CI it does\nnot yet matter). Do you want to prepare that as a patch on top? (Though\nalso see the message I'm about to send that we might be able to avoid\nthis series entirely!).\n\n> > +#ifdef THREAD_BARRIER_PTHREAD\n> > +#define maybe_thread_barrier_t pthread_barrier_t\n> > +#define maybe_thread_barrier_init pthread_barrier_init\n> > +#define maybe_thread_barrier_wait pthread_barrier_wait\n> > +#define maybe_thread_barrier_destroy pthread_barrier_destroy\n> > +#else\n> > +#define maybe_thread_barrier_t int\n> \n> Out of curiosity: why did you pick a define here and not a typedef?\n\nThat's what we do for all of the NO_PTHREADS fallbacks, so I was just\nfollowing that style. I suspect it matters more there, because you would\nnot want to typedef pthread_t on a system that is building with\nNO_PTHREADS but actually does define that type (whereas the \"maybe\"\nvariants are our own invention, so we can be confident those names won't\nconflict).\n\nI don't think it matters too much either way.\n\n> > +static inline int maybe_thread_barrier_init(maybe_thread_barrier_t *b UNUSED,\n> > +\t\t\t\t\t    void *attr UNUSED,\n> > +\t\t\t\t\t    unsigned nr UNUSED)\n> > +{\n> > +\terrno = ENOSYS;\n> > +\treturn -1;\n> > +}\n> > +#define maybe_thread_barrier_wait(barrier)\n> > +#define maybe_thread_barrier_destroy(barrier)\n> \n> So the way these wrappers are implemented it is not possible to check\n> for errors of `pthread_barrier_init()` et al. When the implementation\n> exists we do have return codes, but if it's stubbed out we don't.\n\nYeah, again I was following the NO_PTHREADS fallbacks defined above.\nPerhaps those are different in that we wouldn't generally expect to ever\ncall them if we don't have threads at all (whereas these \"maybe\" ones\nare meant to be quiet noops).\n\n> I think we should align these two implementations so that it does become\n> possible to check for errors, or otherwise we wouldn't be using the\n> pthread APIs correctly. It does raise the question though whether we\n> should really return `-1` in the stubbed-out variant or whether we\n> should instead pretend as if things were alright.\n\nYou'd have to return a fake success if you expect to check errors, I'd\nthink. Unless you want to introduce a bunch of conditional code to say\n\"well, we tried to initialize a barrier but it didn't work, so let's not\nactually wait\".\n\nFor all of the existing pthread calls, we simply assume init stuff like\npthread_mutex_init() doesn't fail. Possibly we should change that\nglobally, but this is just following the same pattern.\n\n> An alternative would be to die in case the pthread-functions return an\n> error.\n\nThat's certainly akin to xmalloc(). But probably we don't want to go\nthat direction, simply because it works against libification in the long\nrun.\n\n-Peff\n"},{"id":"509823","messageId":"Z3eHm-X39LcE8vMy@pks.im","threadId":"62711","inReplyTo":"20250101182826.GB1391912@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] thread-utils: introduce optional barrier type","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T06:45:47Z","receivedAt":"2025-01-03T06:45:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 01, 2025 at 01:28:26PM -0500, Jeff King wrote:\n> On Mon, Dec 30, 2024 at 08:03:38AM +0100, Patrick Steinhardt wrote:\n> > Okay. The Meson equivalent would be:\n> > [...]\n> \n> Yeah, I figured it would need something similar (but for our CI it does\n> not yet matter). Do you want to prepare that as a patch on top? (Though\n> also see the message I'm about to send that we might be able to avoid\n> this series entirely!).\n\nLooks like your version was reverted in favor of the new version you\nhave sent. So I'll refrain from doing that :)\n\nPatrick\n"}]}