Re: [BUG] resolved deltas

From: René Scharfe <l.s.r@web.de>
Date: Aug 25, 2014, 16:39 UTC
Message-ID: <53FB66D1.709@web.de>
In-Reply-To: <20140823111804.GA17335@peff.net>

Am 23.08.2014 um 13:18 schrieb Jeff King:

Show 38 quoted lines

> On Sat, Aug 23, 2014 at 07:04:59AM -0400, Jeff King wrote:
> 
>> On Sat, Aug 23, 2014 at 06:56:40AM -0400, Jeff King wrote:
>>
>>> So I think your patch is doing the right thing.
>>
>> By the way, if you want to add a test to your patch, there is
>> infrastructure in t5308 to create packs with duplicate objects. If I
>> understand the problem correctly, you could trigger this by having a
>> delta object whose base is duplicated, even without the missing object.
> 
> This actually turned out to be really easy. The test below fails without
> your patch and passes with it. Please feel free to squash it in.
> 
> diff --git a/t/t5308-pack-detect-duplicates.sh b/t/t5308-pack-detect-duplicates.sh
> index 9c5a876..50f7a69 100755
> --- a/t/t5308-pack-detect-duplicates.sh
> +++ b/t/t5308-pack-detect-duplicates.sh
> @@ -77,4 +77,19 @@ test_expect_success 'index-pack can reject packs with duplicates' '
>   	test_expect_code 1 git cat-file -e $LO_SHA1
>   '
>   
> +test_expect_success 'duplicated delta base does not trigger assert' '
> +	clear_packs &&
> +	{
> +		A=01d7713666f4de822776c7622c10f1b07de280dc &&
> +		B=e68fe8129b546b101aee9510c5328e7f21ca1d18 &&
> +		pack_header 3 &&
> +		pack_obj $A $B &&
> +		pack_obj $B &&
> +		pack_obj $B
> +	} >dups.pack &&
> +	pack_trailer dups.pack &&
> +	git index-pack --stdin <dups.pack &&
> +	test_must_fail git index-pack --stdin --strict <dups.pack
> +'
> +
>   test_done

Thanks, that looks good. But while preparing the patch I noticed that the added test sometimes fails. Helgrind pointed outet a race condition. It is not caused by the patch to turn the asserts into regular ifs, however -- here's a Helgrind report for the original code with the new test:

==34949== Helgrind, a thread error detector ==34949== Copyright (C) 2007-2013, and GNU GPL'd, by OpenWorks LLP et al. ==34949== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info ==34949== Command: /home/lsr/src/git/t/../bin-wrappers/git index-pack --stdin ==34949== ==34949== Helgrind, a thread error detector ==34949== Copyright (C) 2007-2013, and GNU GPL'd, by OpenWorks LLP et al. ==34949== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info ==34949== Command: /home/lsr/src/git/git index-pack --stdin ==34949== ==34949== ---Thread-Announcement------------------------------------------ ==34949== ==34949== Thread #3 was created ==34949== at 0x594DF7E: clone (clone.S:74) ==34949== by 0x544A2B9: do_clone.constprop.3 (createthread.c:75) ==34949== by 0x544B762: pthread_create@@GLIBC_2.2.5 (createthread.c:245) ==34949== by 0x4C2D55D: pthread_create_WRK (hg_intercepts.c:269) ==34949== by 0x43ABB8: cmd_index_pack (index-pack.c:1097) ==34949== by 0x405B6A: handle_builtin (git.c:351) ==34949== by 0x404CE8: main (git.c:575) ==34949== ==34949== ---Thread-Announcement------------------------------------------ ==34949== ==34949== Thread #2 was created ==34949== at 0x594DF7E: clone (clone.S:74) ==34949== by 0x544A2B9: do_clone.constprop.3 (createthread.c:75) ==34949== by 0x544B762: pthread_create@@GLIBC_2.2.5 (createthread.c:245) ==34949== by 0x4C2D55D: pthread_create_WRK (hg_intercepts.c:269) ==34949== by 0x43ABB8: cmd_index_pack (index-pack.c:1097) ==34949== by 0x405B6A: handle_builtin (git.c:351) ==34949== by 0x404CE8: main (git.c:575) ==34949== ==34949== ---------------------------------------------------------------- ==34949== ==34949== Possible data race during read of size 4 at 0x5E15910 by thread #3 ==34949== Locks held: none ==34949== at 0x439327: find_unresolved_deltas (index-pack.c:918) ==34949== by 0x439666: threaded_second_pass (index-pack.c:1002) ==34949== by 0x4C2D6F6: mythread_wrapper (hg_intercepts.c:233) ==34949== by 0x544B0A3: start_thread (pthread_create.c:309) ==34949== ==34949== This conflicts with a previous write of size 4 by thread #2 ==34949== Locks held: none ==34949== at 0x4390E2: resolve_delta (index-pack.c:865) ==34949== by 0x439340: find_unresolved_deltas (index-pack.c:919) ==34949== by 0x439666: threaded_second_pass (index-pack.c:1002) ==34949== by 0x4C2D6F6: mythread_wrapper (hg_intercepts.c:233) ==34949== by 0x544B0A3: start_thread (pthread_create.c:309) ==34949== ==34949== Address 0x5E15910 is 48 bytes inside a block of size 256 alloc'd ==34949== at 0x4C2A7D0: calloc (vg_replace_malloc.c:618) ==34949== by 0x50CA83: xcalloc (wrapper.c:119) ==34949== by 0x439AF6: cmd_index_pack (index-pack.c:1643) ==34949== by 0x405B6A: handle_builtin (git.c:351) ==34949== by 0x404CE8: main (git.c:575) ==34949== git: builtin/index-pack.c:918: find_unresolved_deltas_1: Assertion `child->real_type == OBJ_REF_DELTA' failed. ==34949== ==34949== For counts of detected and suppressed errors, rerun with: -v ==34949== Use --history-level=approx or =none to gain increased speed, at ==34949== the cost of reduced accuracy of conflicting-access information ==34949== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 15 from 12) Killed

It only happens in one of ten cases or so. The write at index-pack.c line 865 is in resolve_delta() and sets real_type of a delta object to the real_type of its base. It seems that threads can end up with work items that end up requiring to touch another thread's part of the pie if there are duplicates.

René