{"thread":{"id":"60687","subject":"[PATCH] index-pack: spawn threads atomically","startedAt":"2024-01-05T08:50:36Z","lastAt":"2024-01-11T06:53:41Z","messageCount":12,"participants":["Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486339","messageId":"20240105085034.GA3078476@coredump.intra.peff.net","threadId":"60687","inReplyTo":null,"subject":"[PATCH] index-pack: spawn threads atomically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-05T08:50:34Z","receivedAt":"2024-01-05T08:50:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The t5309 script triggers a racy false positive with SANITIZE=leak on a\nmulti-core system. Running with \"--stress --run=6\" usually fails within\n10 seconds or so for me, complaining with something like:\n\n + git index-pack --fix-thin --stdin\n fatal: REF_DELTA at offset 46 already resolved (duplicate base 01d7713666f4de822776c7622c10f1b07de280dc?)\n\n =================================================================\n ==3904583==ERROR: LeakSanitizer: detected memory leaks\n\n Direct leak of 32 byte(s) in 1 object(s) allocated from:\n #0 0x7fa790d01986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n #1 0x7fa790add769 in __pthread_getattr_np nptl/pthread_getattr_np.c:180\n #2 0x7fa790d117c5 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150\n #3 0x7fa790d11957 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:598\n #4 0x7fa790d03fe8 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:51\n #5 0x7fa790d013fd in __lsan_thread_start_func ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:440\n #6 0x7fa790adc3eb in start_thread nptl/pthread_create.c:444\n #7 0x7fa790b5ca5b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81\n\n SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).\n Aborted\n\nWhat happens is this:\n\n 0. We construct a bogus pack with a duplicate object in it and trigger\n index-pack.\n\n 1. We spawn a bunch of worker threads to resolve deltas (on my system\n it is 16 threads).\n\n 2. One of the threads sees the duplicate object and bails by calling\n exit(), taking down all of the threads. This is expected and is the\n point of the test.\n\n 3. At the time exit() is called, we may still be spawning threads from\n the main process via pthread_create(). LSan hooks thread creation\n to update its book-keeping; it has to know where each thread's\n stack is (so it can find entry points for reachable memory). So it\n calls pthread_getattr_np() to get information about the new thread.\n That may allocate memory that must be freed with a matching call to\n pthread_attr_destroy(). Probably LSan does that immediately, but\n if you're unlucky enough, the exit() will happen while it's between\n those two calls, and the allocated pthread_attr_t appears as a\n leak.\n\nThis isn't a real leak. It's not even in our code, but rather in the\nLSan instrumentation code. So we could just ignore it. But the false\npositive can cause people to waste time tracking it down.\n\nIt's possibly something that LSan could protect against (e.g., cover the\ngetattr/destroy pair with a mutex, and then in the final post-exit()\ncheck for leaks try to take the same mutex). But I don't know enough\nabout LSan to say if that's a reasonable approach or not (or if my\nanalysis is even completely correct).\n\nIn the meantime, it's pretty easy to avoid the race by making creation\nof the worker threads \"atomic\". That is, we'll spawn all of them before\nletting any of them start to work. That's easy to do because we already\nhave a work_lock() mutex for handing out that work. If the main process\ntakes it, then all of the threads will immediately block until we've\nfinished spawning and released it.\n\nThis shouldn't make any practical difference for non-LSan runs. The\nthread spawning is quick, and could happen before any worker thread gets\nscheduled anyway.\n\nProbably other spots that use threads are subject to the same issues.\nBut since we have to manually insert locking (and since this really is\nkind of a hack), let's not bother with them unless somebody experiences\na similar racy false-positive in practice.\n\nSigned-off-by: Jeff King \n---\nRescuing this from:\n\n https://lore.kernel.org/git/20231221105124.GD570888@coredump.intra.peff.net/\n\nwhere it was buried deep in a thread. I still think it's kind of gross,\nbut it may be the least-bad thing.\n\n builtin/index-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex dda94a9f46..0e94819216 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1257,13 +1257,15 @@ static void resolve_deltas(void)\n \tbase_cache_limit = 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-- \n2.43.0.553.g8113c77dd0\n"},{"id":"486346","messageId":"ZZgvUyQK6X/MacDC@nand.local","threadId":"60687","inReplyTo":"20240105085034.GA3078476@coredump.intra.peff.net","subject":"Re: [PATCH] index-pack: spawn threads atomically","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-05T16:33:23Z","receivedAt":"2024-01-05T16:33:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jan 05, 2024 at 03:50:34AM -0500, Jeff King wrote:\n> The t5309 script triggers a racy false positive with SANITIZE=leak on a\n> multi-core system. Running with \"--stress --run=6\" usually fails within\n> 10 seconds or so for me, complaining with something like:\n>\n> + git index-pack --fix-thin --stdin\n> fatal: REF_DELTA at offset 46 already resolved (duplicate base 01d7713666f4de822776c7622c10f1b07de280dc?)\n>\n> =================================================================\n> ==3904583==ERROR: LeakSanitizer: detected memory leaks\n>\n> Direct leak of 32 byte(s) in 1 object(s) allocated from:\n> #0 0x7fa790d01986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n> #1 0x7fa790add769 in __pthread_getattr_np nptl/pthread_getattr_np.c:180\n> #2 0x7fa790d117c5 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150\n> #3 0x7fa790d11957 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:598\n> #4 0x7fa790d03fe8 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:51\n> #5 0x7fa790d013fd in __lsan_thread_start_func ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:440\n> #6 0x7fa790adc3eb in start_thread nptl/pthread_create.c:444\n> #7 0x7fa790b5ca5b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81\n>\n> SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).\n> Aborted\n\nWe discussed this in another thread (beginning here [1]), and I would be\nfine with this approach. I share your feeling that it is a little gross\nto have to work around LSan's implementation by tweaking production\ncode, but I think that this doesn't have to be the most pristine patch\never written, either ;-).\n\nJust playing devil's advocate for a moment, I wonder if another approach\nmight be to disable the threading altogether for the purposes of this\ntest. The performance difference is negligible, and I don't think we're\nexercising any interesting paths in this particular test that have to do\nwith pack.threads > 1 that aren't covered extensively elsewhere.\n\nSo, in other words, I think a reasonable approach would be to do\nsomething like:\n\n--- 8< ---\ndiff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh\nindex 4e910c5b9d..1d132b6324 100755\n--- a/t/t5309-pack-delta-cycles.sh\n+++ b/t/t5309-pack-delta-cycles.sh\n@@ -73,7 +73,7 @@ test_expect_success 'failover to a duplicate object in the same pack' '\n \t\tpack_obj $A\n \t} >recoverable.pack &&\n \tpack_trailer recoverable.pack &&\n-\ttest_must_fail git index-pack --fix-thin --stdin 8 ---\n\nAnd call it a day. I built with SANITIZE=leak, and then ran:\n\n $ GIT_TEST_PASSING_SANITIZE_LEAK=true ./t5309-pack-delta-cycles.sh --stress --run=6\n\nFor a while and didn't see any failures. That could be luck, of course,\nbut without the above patch I was seeing failures within a few seconds.\nI'm reasonably confident that this would do the trick.\n\nFor what it's worth, I'm fine with either approach, mostly to avoid\ntying up more of the list's time discussing the options. But I have a\nvague preference towards `--threads=1` since it doesn't require us to\ntouch production code.\n\n> Rescuing this from:\n>\n> https://lore.kernel.org/git/20231221105124.GD570888@coredump.intra.peff.net/\n>\n> where it was buried deep in a thread. I still think it's kind of gross,\n> but it may be the least-bad thing.\n\nIn either case, thanks for digging it back up :-).\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/xmqqbkasnxba.fsf@gitster.g/\n"},{"id":"486521","messageId":"20240110114456.GF16674@coredump.intra.peff.net","threadId":"60687","inReplyTo":"ZZgvUyQK6X/MacDC@nand.local","subject":"Re: [PATCH] index-pack: spawn threads atomically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-10T11:44:56Z","receivedAt":"2024-01-10T11:44:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 05, 2024 at 11:33:23AM -0500, Taylor Blau wrote:\n\n> -\ttest_must_fail git index-pack --fix-thin --stdin +\ttest_must_fail git index-pack --threads=1 --fix-thin --stdin [...]\n> For what it's worth, I'm fine with either approach, mostly to avoid\n> tying up more of the list's time discussing the options. But I have a\n> vague preference towards `--threads=1` since it doesn't require us to\n> touch production code.\n\nThat's quite tempting, actually. The flip side, though, is that the test\nno longer reflects the production code as well. That is, in the real\nworld we'd still call exit() from a thread. That obviously works OK now\n(modulo LSan), but if we ever had a regression where that left us in an\ninconsistent state, we'd be less likely to notice it. Feels kind of\nunlikely in practice, though.\n\nI dunno. I guess the real least-bad thing is seeing if LSan can be\nfixed to handle this atomically. I haven't even reported it there.\n\nIf do go with \"--threads=1\", I suspect several tests in that file need\nit.\n\n-Peff\n"},{"id":"486543","messageId":"ZZ7VEVXSg1T8ZkIK@nand.local","threadId":"60687","inReplyTo":"20240110114456.GF16674@coredump.intra.peff.net","subject":"Re: [PATCH] index-pack: spawn threads atomically","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-10T17:34:09Z","receivedAt":"2024-01-10T17:34:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 10, 2024 at 06:44:56AM -0500, Jeff King wrote:\n> On Fri, Jan 05, 2024 at 11:33:23AM -0500, Taylor Blau wrote:\n>\n> > -\ttest_must_fail git index-pack --fix-thin --stdin > +\ttest_must_fail git index-pack --threads=1 --fix-thin --stdin > [...]\n> > For what it's worth, I'm fine with either approach, mostly to avoid\n> > tying up more of the list's time discussing the options. But I have a\n> > vague preference towards `--threads=1` since it doesn't require us to\n> > touch production code.\n>\n> That's quite tempting, actually. The flip side, though, is that the test\n> no longer reflects the production code as well. That is, in the real\n> world we'd still call exit() from a thread. That obviously works OK now\n> (modulo LSan), but if we ever had a regression where that left us in an\n> inconsistent state, we'd be less likely to notice it. Feels kind of\n> unlikely in practice, though.\n>\n> I dunno. I guess the real least-bad thing is seeing if LSan can be\n> fixed to handle this atomically. I haven't even reported it there.\n\nIn the meantime, I think that the `--threads=1` approach feels less\ninvasive. I tend to agree that neither option is ideal, but that\n`--threads=1` is probably the least bad, and that failing to catch a\nregression there feels rather unlikely.\n\n> If do go with \"--threads=1\", I suspect several tests in that file need\n> it.\n\nYeah, there are a couple of others. I think the ones that need modifying\nare at the intersection of \"expected to fail\" and \"in a test which is\nexpected to pass leak-free\":\n\n $ grep -l 'TEST_PASSES_SANITIZE_LEAK=true' t????-*.sh |\n xargs grep -l 'test_must_fail git index-pack'\n t5302-pack-index.sh\n t5308-pack-detect-duplicates.sh\n t5309-pack-delta-cycles.sh\n t5313-pack-bounds-checks.sh\n t5325-reverse-index.sh\n\nI'll send a series shortly to tweak those test scripts to avoid this\nissue if you want to notify the LSan folks of this issue more generally.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"486547","messageId":"588de2e4f16ab090ff477088084e0b81d9615ec5.1704909216.git.me@ttaylorr.com","threadId":"60687","inReplyTo":"ZZ7VEVXSg1T8ZkIK@nand.local","subject":"[PATCH 1/5] t5309: run expected-to-fail `index-pack`s with `--threads=1`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-10T17:55:30Z","receivedAt":"2024-01-10T17:55:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The t5309 script triggers a racy false positive with SANITIZE=leak on a\nmulti-core system. Running with \"--stress --run=6\" usually fails within\n10 seconds or so for me, complaining with something like:\n\n + git index-pack --fix-thin --stdin\n fatal: REF_DELTA at offset 46 already resolved (duplicate base 01d7713666f4de822776c7622c10f1b07de280dc?)\n\n =================================================================\n ==3904583==ERROR: LeakSanitizer: detected memory leaks\n\n Direct leak of 32 byte(s) in 1 object(s) allocated from:\n #0 0x7fa790d01986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n #1 0x7fa790add769 in __pthread_getattr_np nptl/pthread_getattr_np.c:180\n #2 0x7fa790d117c5 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150\n #3 0x7fa790d11957 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:598\n #4 0x7fa790d03fe8 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:51\n #5 0x7fa790d013fd in __lsan_thread_start_func ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:440\n #6 0x7fa790adc3eb in start_thread nptl/pthread_create.c:444\n #7 0x7fa790b5ca5b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81\n\n SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).\n Aborted\n\nWhat happens is this:\n\n 0. We construct a bogus pack with a duplicate object in it and trigger\n index-pack.\n\n 1. We spawn a bunch of worker threads to resolve deltas (on my system\n it is 16 threads).\n\n 2. One of the threads sees the duplicate object and bails by calling\n exit(), taking down all of the threads. This is expected and is the\n point of the test.\n\n 3. At the time exit() is called, we may still be spawning threads from\n the main process via pthread_create(). LSan hooks thread creation\n to update its book-keeping; it has to know where each thread's\n stack is (so it can find entry points for reachable memory). So it\n calls pthread_getattr_np() to get information about the new thread.\n That may allocate memory that must be freed with a matching call to\n pthread_attr_destroy(). Probably LSan does that immediately, but\n if you're unlucky enough, the exit() will happen while it's between\n those two calls, and the allocated pthread_attr_t appears as a\n leak.\n\nThis isn't a real leak. It's not even in our code, but rather in the\nLSan instrumentation code. So we could just ignore it. But the false\npositive can cause people to waste time tracking it down.\n\nIt's possibly something that LSan could protect against (e.g., cover the\ngetattr/destroy pair with a mutex, and then in the final post-exit()\ncheck for leaks try to take the same mutex). But I don't know enough\nabout LSan to say if that's a reasonable approach or not (or if my\nanalysis is even completely correct).\n\nOne approach to papering over this issue (short of LSan fixing it\nupstream) is to make the creation of work threads \"atomic\", i.e. by\nspawning all of them before letting any of them start to work. This\nshouldn't make any practical difference for non-LSan runs. The thread\nspawning is quick, and could happen before any worker thread gets\nscheduled anyway.\n\nBut that requires us to tweak production code (albeit at a negligible\ncost) in order to appease LSan in this narrow circumstance. Another\napproach is to simply run these expected-to-fail `index-pack`\ninvocations with `--threads=1` so that we bypass the above issue\nentirely.\n\nThe downside of that approach is that the test doesn't match our\nproduction code as well as it did before (where we might have run those\nsame `index-pack` invocations with >1 thread, depending on how many CPUs\nthe testing machine has). The risk there is that we might miss a\nregression that would leave us in an inconsistent state. But that feels\nrather unlikely in practice, and there are many other tests related to\n`index-pack` in the suite.\n\nOriginal-patch-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n t/t5309-pack-delta-cycles.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh\nindex 4e910c5b9d..4100595c89 100755\n--- a/t/t5309-pack-delta-cycles.sh\n+++ b/t/t5309-pack-delta-cycles.sh\n@@ -44,7 +44,7 @@ test_expect_success 'index-pack detects missing base objects' '\n \t\tpack_obj $A $B\n \t} >missing.pack &&\n \tpack_trailer missing.pack &&\n-\ttest_must_fail git index-pack --fix-thin --stdin cycle.pack &&\n \tpack_trailer cycle.pack &&\n-\ttest_must_fail git index-pack --fix-thin --stdin recoverable.pack &&\n \tpack_trailer recoverable.pack &&\n-\ttest_must_fail git index-pack --fix-thin --stdin \n---\n t/t5302-pack-index.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex d88e6f1691..fdbcd80f89 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -290,7 +290,7 @@ test_expect_success 'too-large packs report the breach' '\n \tpack=$(git pack-objects --all pack err &&\n+\ttest_must_fail git index-pack --threads=1 --max-input-size=20 pack-$pack.pack 2>err &&\n \tgrep \"maximum allowed size (20 bytes)\" err\n '\n \n-- \n2.43.0.288.g906e6a084d\n\n"},{"id":"486549","messageId":"0170befabd160bd98a32372f14b5e1e9bbcb1069.1704909216.git.me@ttaylorr.com","threadId":"60687","inReplyTo":"ZZ7VEVXSg1T8ZkIK@nand.local","subject":"[PATCH 3/5] t5308: run expected-to-fail `index-pack`s with `--threads=1`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-10T17:55:37Z","receivedAt":"2024-01-10T17:55:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"For identical reasons as in the previous commit, apply the same\ntreatment to expected-to-fail `index-pack` invocations in t5308 with\n`--threads=1`.\n\nSigned-off-by: Taylor Blau \n---\n t/t5308-pack-detect-duplicates.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5308-pack-detect-duplicates.sh b/t/t5308-pack-detect-duplicates.sh\nindex 655cafa054..5df552a42e 100755\n--- a/t/t5308-pack-detect-duplicates.sh\n+++ b/t/t5308-pack-detect-duplicates.sh\n@@ -76,7 +76,7 @@ test_expect_success 'lookup in duplicated pack' '\n test_expect_success 'index-pack can reject packs with duplicates' '\n \tclear_packs &&\n \tcreate_pack dups.pack 2 &&\n-\ttest_must_fail git index-pack --strict --stdin \n---\n t/t5313-pack-bounds-checks.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh\nindex ceaa6700a2..e9829b6ef3 100755\n--- a/t/t5313-pack-bounds-checks.sh\n+++ b/t/t5313-pack-bounds-checks.sh\n@@ -91,7 +91,7 @@ test_expect_success 'pack/index object count mismatch' '\n \n \t# ...and make sure that index-pack --verify, which has its\n \t# own reading routines, does not segfault.\n-\ttest_must_fail git index-pack --verify $pack\n+\ttest_must_fail git index-pack --threads=1 --verify $pack\n '\n \n test_expect_success 'matched bogus object count' '\n@@ -111,7 +111,7 @@ test_expect_success 'matched bogus object count' '\n \tgit cat-file blob $object >actual &&\n \ttest_cmp file actual &&\n \n-\ttest_must_fail git index-pack --verify $pack\n+\ttest_must_fail git index-pack --threads=1 --verify $pack\n '\n \n # Note that we cannot check the fallback case for these\n@@ -128,7 +128,7 @@ test_expect_success 'bogus object offset (v1)' '\n \tmunge $idx $((4 * 256)) \"\\377\\0\\0\\0\" &&\n \tclear_base &&\n \ttest_must_fail git cat-file blob $object &&\n-\ttest_must_fail git index-pack --verify $pack\n+\ttest_must_fail git index-pack --threads=1 --verify $pack\n '\n \n test_expect_success 'bogus object offset (v2, no msb)' '\n@@ -136,7 +136,7 @@ test_expect_success 'bogus object offset (v2, no msb)' '\n \tmunge $idx $(ofs_table 1) \"\\0\\377\\0\\0\" &&\n \tclear_base &&\n \ttest_must_fail git cat-file blob $object &&\n-\ttest_must_fail git index-pack --verify $pack\n+\ttest_must_fail git index-pack --threads=1 --verify $pack\n '\n \n test_expect_success 'bogus offset into v2 extended table' '\n@@ -144,7 +144,7 @@ test_expect_success 'bogus offset into v2 extended table' '\n \tmunge $idx $(ofs_table 1) \"\\377\\0\\0\\0\" &&\n \tclear_base &&\n \ttest_must_fail git cat-file blob $object &&\n-\ttest_must_fail git index-pack --verify $pack\n+\ttest_must_fail git index-pack --threads=1 --verify $pack\n '\n \n test_expect_success 'bogus offset inside v2 extended table' '\n@@ -169,7 +169,7 @@ test_expect_success 'bogus offset inside v2 extended table' '\n \tmv tmp \"$idx\" &&\n \tclear_base &&\n \ttest_must_fail git cat-file blob $object &&\n-\ttest_must_fail git index-pack --verify $pack\n+\ttest_must_fail git index-pack --threads=1 --verify $pack\n '\n \n test_expect_success 'bogus OFS_DELTA in packfile' '\n-- \n2.43.0.288.g906e6a084d\n\n"},{"id":"486551","messageId":"7d0017342d4f9f81a1ec74ab02aac0d3d44d38ff.1704909216.git.me@ttaylorr.com","threadId":"60687","inReplyTo":"ZZ7VEVXSg1T8ZkIK@nand.local","subject":"[PATCH 5/5] t5325: run expected-to-fail `index-pack`s with `--threads=1`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-10T17:55:42Z","receivedAt":"2024-01-10T17:55:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"For identical reasons as in the previous commit, apply the same\ntreatment to expected-to-fail `index-pack` invocations in t5325 with\n`--threads=1`.\n\nSigned-off-by: Taylor Blau \n---\n t/t5325-reverse-index.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 431a603ca0..dc3d2235e8 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -64,7 +64,7 @@ test_expect_success 'index-pack can verify reverse indexes' '\n \tchmod u+w $rev &&\n \tprintf \"xxxx\" | dd of=$rev bs=1 count=4 conv=notrunc &&\n \n-\ttest_must_fail git index-pack --rev-index --verify \\\n+\ttest_must_fail git index-pack --threads=1 --rev-index --verify \\\n \t\t$packdir/pack-$pack.pack 2>err &&\n \tgrep \"validation error\" err\n '\n-- \n2.43.0.288.g906e6a084d\n"},{"id":"486571","messageId":"xmqqfrz496ib.fsf@gitster.g","threadId":"60687","inReplyTo":"588de2e4f16ab090ff477088084e0b81d9615ec5.1704909216.git.me@ttaylorr.com","subject":"Re: [PATCH 1/5] t5309: run expected-to-fail `index-pack`s with `--threads=1`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T22:18:52Z","receivedAt":"2024-01-10T22:18:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> But that requires us to tweak production code (albeit at a negligible\n> cost) in order to appease LSan in this narrow circumstance. Another\n> approach is to simply run these expected-to-fail `index-pack`\n> invocations with `--threads=1` so that we bypass the above issue\n> entirely.\n\nBut of course, multi-threaded operation that production folks use\nwill not be tested at all with the alternative.\n\n> The downside of that approach is that the test doesn't match our\n> production code as well as it did before (where we might have run those\n> same `index-pack` invocations with >1 thread, depending on how many CPUs\n> the testing machine has). The risk there is that we might miss a\n> regression that would leave us in an inconsistent state. But that feels\n> rather unlikely in practice, and there are many other tests related to\n> `index-pack` in the suite.\n\nAs long as \"make sure we spawn all of them atmically\" has negligible\ndownside, I'd rather take that approach. Buying predictability with\nminimum cost is quite attractive.\n\nThanks.\n"},{"id":"486572","messageId":"ZZ8ZTAj3E8eGJvDR@nand.local","threadId":"60687","inReplyTo":"xmqqfrz496ib.fsf@gitster.g","subject":"Re: [PATCH 1/5] t5309: run expected-to-fail `index-pack`s with `--threads=1`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-10T22:25:16Z","receivedAt":"2024-01-10T22:25:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 10, 2024 at 02:18:52PM -0800, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > But that requires us to tweak production code (albeit at a negligible\n> > cost) in order to appease LSan in this narrow circumstance. Another\n> > approach is to simply run these expected-to-fail `index-pack`\n> > invocations with `--threads=1` so that we bypass the above issue\n> > entirely.\n>\n> But of course, multi-threaded operation that production folks use\n> will not be tested at all with the alternative.\n\nJust the ones that we expect to fail *and* are in test scripts which are\nmarked as leak-free.\n\n> > The downside of that approach is that the test doesn't match our\n> > production code as well as it did before (where we might have run those\n> > same `index-pack` invocations with >1 thread, depending on how many CPUs\n> > the testing machine has). The risk there is that we might miss a\n> > regression that would leave us in an inconsistent state. But that feels\n> > rather unlikely in practice, and there are many other tests related to\n> > `index-pack` in the suite.\n>\n> As long as \"make sure we spawn all of them atmically\" has negligible\n> downside, I'd rather take that approach. Buying predictability with\n> minimum cost is quite attractive.\n\nLike I said earlier, I have no strong preference between either\napproach. If you want to pick up Peff's patch instead of these five,\nthat is fine with me :-).\n\nThanks,\nTaylor\n"},{"id":"486597","messageId":"20240111065338.GA48154@coredump.intra.peff.net","threadId":"60687","inReplyTo":"ZZ7VEVXSg1T8ZkIK@nand.local","subject":"Re: [PATCH] index-pack: spawn threads atomically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-11T06:53:38Z","receivedAt":"2024-01-11T06:53:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 10, 2024 at 12:34:09PM -0500, Taylor Blau wrote:\n\n> > If do go with \"--threads=1\", I suspect several tests in that file need\n> > it.\n> \n> Yeah, there are a couple of others. I think the ones that need modifying\n> are at the intersection of \"expected to fail\" and \"in a test which is\n> expected to pass leak-free\":\n> \n> $ grep -l 'TEST_PASSES_SANITIZE_LEAK=true' t????-*.sh |\n> xargs grep -l 'test_must_fail git index-pack'\n> t5302-pack-index.sh\n> t5308-pack-detect-duplicates.sh\n> t5309-pack-delta-cycles.sh\n> t5313-pack-bounds-checks.sh\n> t5325-reverse-index.sh\n\nI think that is more than we need. It's only a problem when we hit a\ndie() inside a thread, which happens only during delta resolution. So\nyour patch 2, for example, touches a test which triggers the\n--max-input-size check. But we would find that out on the initial\nunthreaded pass over the pack.\n\nThe one in patch 3 seems at first glance like it might be a problem\n(it's another duplicate-object case, like the one of them in 5309). But\nit isn't a problem because the duplicate object isn't a delta, so we\nnotice the problem in write_idx_file() from the main thread (which I\nverified by running it under gdb and setting a breakpoint at die()).\n\nI suspect patch 4 is the same, but didn't run gdb on each case. And\npatch 5 is about a corrupt reverse index, so almost certainly the main\nthread. So I suspect that patch 1 is the only one that matters here (and\nprobably all of those are needed, because it is all about broken\ndeltas).\n\nAll that said, I am on the fence between the two approaches. If Junio\nprefers the atomic-spawn direction, I'm fine with that, and there's not\nmuch point in polishing the --threads=1 approach further.\n\n-Peff\n"}]}