{"thread":{"id":"46590","subject":"[PATCH/RFC 0/5] Some ThreadSanitizer-results","startedAt":"2017-08-15T12:53:33Z","lastAt":"2017-09-06T15:44:16Z","messageCount":64,"participants":["Martin Ågren","Torsten Bögershausen","Stefan Beller","Jeff Hostetler","Junio C Hamano","Johannes Sixt","Jeff King","Brandon Casey","Johannes Schindelin","Jonathan Nieder","Simon Ruderich"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"326391","messageId":"cover.1502780343.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":null,"subject":"[PATCH/RFC 0/5] Some ThreadSanitizer-results","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T12:53:00Z","receivedAt":"2017-08-15T12:53:33Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"I tried running the test suite on Git compiled with ThreadSanitizer\n(SANITIZE=thread). Maybe this series could be useful for someone else\ntrying to do the same. I needed the first patch to avoid warnings when\ncompiling, although it actually has nothing to do with threads.\n\nThe last four patches are about avoiding some issues where\nThreadSanitizer complains for reasonable reasons, but which to the best\nof my understanding are not real problems. These patches could be useful\nto make \"actual\" problems stand out more. Of course, if no-one ever runs\nThreadSanitizer, they are of little to no (or even negative) value...\n\nI'll follow up with the two remaining issues that I found but which I do\nnot try to address in this series.\n\nMartin\n\nMartin Ågren (5):\n convert: initialize attr_action in convert_attrs\n pack-objects: take lock before accessing `remaining`\n Makefile: define GIT_THREAD_SANITIZER\n strbuf_reset: don't write to slopbuf with ThreadSanitizer\n ThreadSanitizer: add suppressions\n\n strbuf.h | 12 ++++++++++++\n builtin/pack-objects.c | 6 ++++++\n convert.c | 2 +-\n Makefile | 3 +++\n .tsan-suppressions | 12 ++++++++++++\n 5 files changed, 34 insertions(+), 1 deletion(-)\n create mode 100644 .tsan-suppressions\n\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326392","messageId":"0fd7f3184d285df8867ea44dd1adf418ebfc5ef3.1502780344.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1502780343.git.martin.agren@gmail.com","subject":"[PATCH 1/5] convert: initialize attr_action in convert_attrs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T12:53:01Z","receivedAt":"2017-08-15T12:53:38Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"convert_attrs populates a struct conv_attrs. The field attr_action is\nnot set in all code paths, but still one caller unconditionally reads\nit. Since git_check_attr always returns the same value, we'll always end\nup in the same code path and there is no problem right now. But\nconvert_attrs is obviously trying not to rely on such an\nimplementation-detail of another component.\n\nInitialize attr_action to CRLF_UNDEFINED in the dead code path.\n\nActually, in the code path that /is/ taken, the variable is assigned to\ntwice and the first assignment has no effect. That's not wrong, but\nlet's remove that first assignment while we're here.\n\nSigned-off-by: Martin Ågren \n---\nI hit a warning about attr_action possibly being uninitialized when\nbuilding with SANITIZE=thread. I guess it's some random interaction\nbetween code added by tsan, the optimizer (-O3) and the warning\nmachinery. (This was with gcc 5.4.0.)\n\n convert.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/convert.c b/convert.c\nindex 1012462e3..943d957b4 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1040,7 +1040,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t\tca->crlf_action = git_path_check_crlf(ccheck + 4);\n \t\tif (ca->crlf_action == CRLF_UNDEFINED)\n \t\t\tca->crlf_action = git_path_check_crlf(ccheck + 0);\n-\t\tca->attr_action = ca->crlf_action;\n \t\tca->ident = git_path_check_ident(ccheck + 1);\n \t\tca->drv = git_path_check_convert(ccheck + 2);\n \t\tif (ca->crlf_action != CRLF_BINARY) {\n@@ -1058,6 +1057,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t} else {\n \t\tca->drv = NULL;\n \t\tca->crlf_action = CRLF_UNDEFINED;\n+\t\tca->attr_action = CRLF_UNDEFINED;\n \t\tca->ident = 0;\n \t}\n \tif (ca->crlf_action == CRLF_TEXT)\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326393","messageId":"5815ea4f27226b604751961c8b70355a8925f0c5.1502780344.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1502780343.git.martin.agren@gmail.com","subject":"[PATCH 2/5] pack-objects: take lock before accessing `remaining`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T12:53:02Z","receivedAt":"2017-08-15T12:53:39Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When checking the conditional of \"while (me->remaining)\", we did not\nhold the lock. Calling find_deltas would still be safe, since it checks\n\"remaining\" (after taking the lock) and is able to handle all values. In\nfact, this could (currently) not trigger any bug: a bug could happen if\n`remaining` transitioning from zero to non-zero races with the evaluation\nof the while-condition, but these are always separated by the\ndata_ready-mechanism.\n\nMake sure we have the lock when we read `remaining`. This does mean we\nrelease it just so that find_deltas can take it immediately again. We\ncould tweak the contract so that the lock should be taken before calling\nfind_deltas, but let's defer that until someone can actually show that\n\"unlock+lock\" has a measurable negative impact.\n\nSigned-off-by: Martin Ågren \n---\nI don't think this corrects any real error. The benefits of this patch\nwould be \"future-proofs things slightly\" and \"silences tsan, so that\nother errors don't drown in noise\". Feel free to tell me those benefits\nare negligible and that this change actually hurts.\n\n builtin/pack-objects.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c753e9237..bd391e97a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2170,7 +2170,10 @@ static void *threaded_find_deltas(void *arg)\n {\n \tstruct thread_params *me = arg;\n \n+\tprogress_lock();\n \twhile (me->remaining) {\n+\t\tprogress_unlock();\n+\n \t\tfind_deltas(me->list, &me->remaining,\n \t\t\t me->window, me->depth, me->processed);\n \n@@ -2192,7 +2195,10 @@ static void *threaded_find_deltas(void *arg)\n \t\t\tpthread_cond_wait(&me->cond, &me->mutex);\n \t\tme->data_ready = 0;\n \t\tpthread_mutex_unlock(&me->mutex);\n+\n+\t\tprogress_lock();\n \t}\n+\tprogress_unlock();\n \t/* leave ->working 1 so that this doesn't get more work assigned */\n \treturn NULL;\n }\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326394","messageId":"51dfd9e8ac7ec1d9019342bda89466c8fe133106.1502780344.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1502780343.git.martin.agren@gmail.com","subject":"[PATCH 5/5] ThreadSanitizer: add suppressions","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T12:53:05Z","receivedAt":"2017-08-15T12:53:52Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Add .tsan-suppressions for want_color() and transfer_debug(). Both of\nthese use the pattern\n\n\tstatic int foo = -1;\n\tif (foo == -1)\n\t\tfoo = func();\n\nwhere func always returns the same value. This can cause ThreadSanitizer\nto diagnose a race when foo is written from two threads, although it\ndoesn't matter in practice since it's always the same value that is\nwritten.\n\nThe suppressions-file is used by setting the environment variable\nTSAN_OPTIONS to, e.g., \"suppressions=$(pwd)/.tsan-suppressions\". Observe\nthat relative paths such as \".tsan-suppressions\" might not work.\n\nSigned-off-by: Martin Ågren \n---\nI am no memory-model expert. Maybe (aligned) stores and loads of int are\nnot actually atomic on all the various hardware that Git wants to run\non. Or maybe the compiler is allowed to compile them into 4 1-byte\naccesses anyway...\n\n .tsan-suppressions | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n create mode 100644 .tsan-suppressions\n\ndiff --git a/.tsan-suppressions b/.tsan-suppressions\nnew file mode 100644\nindex 000000000..910c02e59\n--- /dev/null\n+++ b/.tsan-suppressions\n@@ -0,0 +1,12 @@\n+# Suppressions for ThreadSanitizer (tsan).\n+#\n+# This file is used by setting the environment variable TSAN_OPTIONS to, e.g.,\n+# \"suppressions=$(pwd)/.tsan-suppressions\". Observe that relative paths such as\n+# \".tsan-suppressions\" might not work.\n+#\n+# These suppressions can be, e.g., that a static variable is written to and it\n+# is always the same value being written, so it doesn't really matter that two\n+# or more such writes race.\n+\n+race:^want_color$\n+race:^transfer_debug$\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326395","messageId":"adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1502780343.git.martin.agren@gmail.com","subject":"tsan: t3008: hashmap_add touches size from multiple threads","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T12:53:06Z","receivedAt":"2017-08-15T12:53:56Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Using SANITIZE=thread made t3008-ls-files-lazy-init-name-hash.sh hit\nthe potential race below.\n\nWhat seems to happen is, threaded_lazy_init_name_hash ends up using\nhashmap_add on the_index.dir_hash from two threads in a way that tsan\nconsiders racy. While the buckets each have their own mutex, the \"size\"\ndoes not. So it might end up with the wrong (lower) value. The size is\nused to determine when to resize, but that should be fine, since\nresizing is turned off due to threading anyway.\n\nIf the size is ever used for something else, the consequences might\nrange from cosmetic error to devastating. I have a \"feeling\" the size is\nnot used at the time, although maybe it is, in some roundabout way which\nI'm not seeing.\n\nMartin\n\nWARNING: ThreadSanitizer: data race (pid=10554)\n Read of size 4 at 0x00000082d488 by thread T2 (mutexes: write M16):\n #0 hashmap_add hashmap.c:209 (test-lazy-init-name-hash+0x000000438b7d)\n #1 hash_dir_entry_with_parent_and_prefix name-hash.c:302 (test-lazy-init-name-hash+0x00000043ea6c)\n #2 handle_range_dir name-hash.c:347 (test-lazy-init-name-hash+0x00000043ea6c)\n #3 handle_range_1 name-hash.c:415 (test-lazy-init-name-hash+0x00000043ea6c)\n #4 lazy_dir_thread_proc name-hash.c:471 (test-lazy-init-name-hash+0x00000043ea6c)\n #5 (libtsan.so.0+0x0000000230d9)\n\n Previous write of size 4 at 0x00000082d488 by thread T1 (mutexes: write M31):\n #0 hashmap_add hashmap.c:209 (test-lazy-init-name-hash+0x000000438b90)\n #1 hash_dir_entry_with_parent_and_prefix name-hash.c:302 (test-lazy-init-name-hash+0x00000043e0f2)\n #2 handle_range_dir name-hash.c:347 (test-lazy-init-name-hash+0x00000043e0f2)\n #3 handle_range_1 name-hash.c:415 (test-lazy-init-name-hash+0x00000043e0f2)\n #4 handle_range_dir name-hash.c:380 (test-lazy-init-name-hash+0x00000043e709)\n #5 handle_range_1 name-hash.c:415 (test-lazy-init-name-hash+0x00000043e709)\n #6 lazy_dir_thread_proc name-hash.c:471 (test-lazy-init-name-hash+0x00000043e709)\n #7 (libtsan.so.0+0x0000000230d9)\n\n Location is global 'the_index' of size 208 at 0x00000082d400 (test-lazy-init-name-hash+0x00000082d488)\n\n Mutex M16 (0x7d640001a5b8) created at:\n #0 pthread_mutex_init (libtsan.so.0+0x0000000280b5)\n #1 init_recursive_mutex thread-utils.c:73 (test-lazy-init-name-hash+0x0000004d829b)\n #2 init_dir_mutex name-hash.c:241 (test-lazy-init-name-hash+0x00000043d714)\n #3 threaded_lazy_init_name_hash name-hash.c:526 (test-lazy-init-name-hash+0x00000043d714)\n #4 lazy_init_name_hash name-hash.c:588 (test-lazy-init-name-hash+0x00000043d714)\n #5 lazy_init_name_hash name-hash.c:581 (test-lazy-init-name-hash+0x00000043ec34)\n #6 test_lazy_init_name_hash name-hash.c:613 (test-lazy-init-name-hash+0x00000043ec34)\n #7 time_runs t/helper/test-lazy-init-name-hash.c:74 (test-lazy-init-name-hash+0x000000405ac2)\n #8 cmd_main t/helper/test-lazy-init-name-hash.c:261 (test-lazy-init-name-hash+0x0000004066c1)\n #9 main common-main.c:43 (test-lazy-init-name-hash+0x000000404f91)\n\n Mutex M31 (0x7d640001a810) created at:\n #0 pthread_mutex_init (libtsan.so.0+0x0000000280b5)\n #1 init_recursive_mutex thread-utils.c:73 (test-lazy-init-name-hash+0x0000004d829b)\n #2 init_dir_mutex name-hash.c:241 (test-lazy-init-name-hash+0x00000043d714)\n #3 threaded_lazy_init_name_hash name-hash.c:526 (test-lazy-init-name-hash+0x00000043d714)\n #4 lazy_init_name_hash name-hash.c:588 (test-lazy-init-name-hash+0x00000043d714)\n #5 lazy_init_name_hash name-hash.c:581 (test-lazy-init-name-hash+0x00000043ec34)\n #6 test_lazy_init_name_hash name-hash.c:613 (test-lazy-init-name-hash+0x00000043ec34)\n #7 time_runs t/helper/test-lazy-init-name-hash.c:74 (test-lazy-init-name-hash+0x000000405ac2)\n #8 cmd_main t/helper/test-lazy-init-name-hash.c:261 (test-lazy-init-name-hash+0x0000004066c1)\n #9 main common-main.c:43 (test-lazy-init-name-hash+0x000000404f91)\n\n Thread T2 (tid=10557, running) created by main thread at:\n #0 pthread_create (libtsan.so.0+0x000000027577)\n #1 threaded_lazy_init_name_hash name-hash.c:541 (test-lazy-init-name-hash+0x00000043d7ab)\n #2 lazy_init_name_hash name-hash.c:588 (test-lazy-init-name-hash+0x00000043d7ab)\n #3 lazy_init_name_hash name-hash.c:581 (test-lazy-init-name-hash+0x00000043ec34)\n #4 test_lazy_init_name_hash name-hash.c:613 (test-lazy-init-name-hash+0x00000043ec34)\n #5 time_runs t/helper/test-lazy-init-name-hash.c:74 (test-lazy-init-name-hash+0x000000405ac2)\n #6 cmd_main t/helper/test-lazy-init-name-hash.c:261 (test-lazy-init-name-hash+0x0000004066c1)\n #7 main common-main.c:43 (test-lazy-init-name-hash+0x000000404f91)\n\n Thread T1 (tid=10556, finished) created by main thread at:\n #0 pthread_create (libtsan.so.0+0x000000027577)\n #1 threaded_lazy_init_name_hash name-hash.c:541 (test-lazy-init-name-hash+0x00000043d7ab)\n #2 lazy_init_name_hash name-hash.c:588 (test-lazy-init-name-hash+0x00000043d7ab)\n #3 lazy_init_name_hash name-hash.c:581 (test-lazy-init-name-hash+0x00000043ec34)\n #4 test_lazy_init_name_hash name-hash.c:613 (test-lazy-init-name-hash+0x00000043ec34)\n #5 time_runs t/helper/test-lazy-init-name-hash.c:74 (test-lazy-init-name-hash+0x000000405ac2)\n #6 cmd_main t/helper/test-lazy-init-name-hash.c:261 (test-lazy-init-name-hash+0x0000004066c1)\n #7 main common-main.c:43 (test-lazy-init-name-hash+0x000000404f91)\n\nSUMMARY: ThreadSanitizer: data race hashmap.c:209 hashmap_add\n\n"},{"id":"326396","messageId":"939b37f809dd9e1526593c02154fae14b369c73a.1502780344.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1502780343.git.martin.agren@gmail.com","subject":"tsan: t5400: set_try_to_free_routine","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T12:53:07Z","receivedAt":"2017-08-15T12:53:57Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Using SANITIZE=thread made t5400-send-pack.sh hit the potential race\nbelow.\n\nThis is set_try_to_free_routine in wrapper.c. The race relates to the\nreading of the \"old\" value. The caller doesn't care about the \"old\"\nvalue, so this should be harmless right now. But it seems that using\nthis mechanism from multiple threads and restoring the earlier value\nwill probably not work out every time. (Not necessarily because of the\nrace in set_try_to_free_routine, but, e.g., because callers might not\nrestore the function pointer in the reverse order of how they\noriginally set it.)\n\nProperly \"fixing\" this for thread-safety would probably require some\nredesigning, which at the time might not be warranted. I'm just posting\nthis for completeness.\n\nMartin\n\nWARNING: ThreadSanitizer: data race (pid=21382)\n Read of size 8 at 0x000000979970 by thread T1:\n #0 set_try_to_free_routine wrapper.c:35 (git+0x0000006cde1c)\n #1 prepare_trace_line trace.c:105 (git+0x0000006a3bf0)\n #2 trace_strbuf_fl trace.c:185 (git+0x0000006a3bf0)\n #3 packet_trace pkt-line.c:80 (git+0x0000005f9f43)\n #4 packet_read pkt-line.c:309 (git+0x0000005fbe10)\n #5 recv_sideband sideband.c:37 (git+0x000000684c5e)\n #6 sideband_demux send-pack.c:216 (git+0x00000065a38c)\n #7 run_thread run-command.c:933 (git+0x000000655a93)\n #8 (libtsan.so.0+0x0000000230d9)\n\n Previous write of size 8 at 0x000000979970 by main thread:\n #0 set_try_to_free_routine wrapper.c:38 (git+0x0000006cde39)\n #1 prepare_trace_line trace.c:105 (git+0x0000006a3bf0)\n #2 trace_strbuf_fl trace.c:185 (git+0x0000006a3bf0)\n #3 packet_trace pkt-line.c:80 (git+0x0000005f9f43)\n #4 packet_read pkt-line.c:324 (git+0x0000005fc0bb)\n #5 packet_read_line_generic pkt-line.c:332 (git+0x0000005fc0bb)\n #6 packet_read_line pkt-line.c:342 (git+0x0000005fc0bb)\n #7 receive_unpack_status send-pack.c:149 (git+0x00000065c1e4)\n #8 send_pack send-pack.c:581 (git+0x00000065c1e4)\n #9 git_transport_push transport.c:574 (git+0x0000006ab2c1)\n #10 transport_push transport.c:1068 (git+0x0000006ae5d5)\n #11 push_with_options builtin/push.c:336 (git+0x00000049d580)\n #12 do_push builtin/push.c:419 (git+0x00000049f57d)\n #13 cmd_push builtin/push.c:591 (git+0x00000049f57d)\n #14 run_builtin git.c:330 (git+0x00000040949e)\n #15 handle_builtin git.c:538 (git+0x00000040949e)\n #16 run_argv git.c:590 (git+0x000000409a0e)\n #17 cmd_main git.c:667 (git+0x000000409a0e)\n #18 main common-main.c:43 (git+0x0000004079d1)\n\n Location is global 'try_to_free_routine' of size 8 at 0x000000979970 (git+0x000000979970)\n\n Thread T1 (tid=21405, running) created by main thread at:\n #0 pthread_create (libtsan.so.0+0x000000027577)\n #1 start_async run-command.c:1130 (git+0x0000006582e7)\n #2 send_pack send-pack.c:562 (git+0x00000065b7c8)\n #3 git_transport_push transport.c:574 (git+0x0000006ab2c1)\n #4 transport_push transport.c:1068 (git+0x0000006ae5d5)\n #5 push_with_options builtin/push.c:336 (git+0x00000049d580)\n #6 do_push builtin/push.c:419 (git+0x00000049f57d)\n #7 cmd_push builtin/push.c:591 (git+0x00000049f57d)\n #8 run_builtin git.c:330 (git+0x00000040949e)\n #9 handle_builtin git.c:538 (git+0x00000040949e)\n #10 run_argv git.c:590 (git+0x000000409a0e)\n #11 cmd_main git.c:667 (git+0x000000409a0e)\n #12 main common-main.c:43 (git+0x0000004079d1)\n\nSUMMARY: ThreadSanitizer: data race wrapper.c:35 set_try_to_free_routine\n\n"},{"id":"326397","messageId":"931ffb00319f40e3c9e099f17eeae6a0c1de41ea.1502780344.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1502780343.git.martin.agren@gmail.com","subject":"[PATCH 4/5] strbuf_reset: don't write to slopbuf with ThreadSanitizer","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T12:53:04Z","receivedAt":"2017-08-15T12:54:09Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If two threads have one freshly initialized string buffer each and call\nstrbuf_reset on them at roughly the same time, both threads will be\nwriting a '\\0' to strbuf_slopbuf. That is not a problem in practice\nsince it doesn't matter in which order the writes happen. But\nThreadSanitizer will consider this a race.\n\nWhen compiling with GIT_THREAD_SANITIZER, avoid writing to\nstrbuf_slopbuf. Let's instead assert on the first byte of strbuf_slopbuf\nbeing '\\0', since it ensures the promised invariant of \"buf[len] ==\n'\\0'\". (Writing to strbuf_slopbuf is normally bad, but could become even\nmore bad if we stop covering it up in strbuf_reset.)\n\nSigned-off-by: Martin Ågren \n---\n strbuf.h | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex e705b94db..295654d39 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -153,7 +153,19 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n /**\n * Empty the buffer by setting the size of it to zero.\n */\n+#ifdef GIT_THREAD_SANITIZER\n+#define strbuf_reset(sb)\t\t\t\t\t\t\\\n+\tdo {\t\t\t\t\t\t\t\t\\\n+\t\tstruct strbuf *_sb = sb; \t\t\t\t\\\n+\t\t_sb->len = 0;\t\t\t\t\t\t\\\n+\t\tif (_sb->buf == strbuf_slopbuf)\t\t\t\t\\\n+\t\t\tassert(!strbuf_slopbuf[0]);\t\t\t\\\n+\t\telse\t\t\t\t\t\t\t\\\n+\t\t\t_sb->buf[0] = '\\0';\t\t\t\t\\\n+\t} while (0)\n+#else\n #define strbuf_reset(sb) strbuf_setlen(sb, 0)\n+#endif\n \n \n /**\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326398","messageId":"258b5b68f29d680fea0a35b5db40783819df222e.1502780344.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1502780343.git.martin.agren@gmail.com","subject":"[PATCH 3/5] Makefile: define GIT_THREAD_SANITIZER","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T12:53:03Z","receivedAt":"2017-08-15T12:54:28Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When using ThreadSanitizer (tsan), it can be useful to avoid triggering\nfalse or irrelevant alarms. This should obviously be done with care and\nnot to paper over real problems. Detecting that tsan is in use in a\ncross-compiler way is non-trivial.\n\nTie into our existing SANITIZE=...-framework and define\nGIT_THREAD_SANITIZER when we are compiling for tsan.\n\nSigned-off-by: Martin Ågren \n---\n Makefile | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 461c845d3..e77a60d1f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1033,6 +1033,9 @@ BASIC_CFLAGS += -fno-omit-frame-pointer\n ifneq ($(filter undefined,$(SANITIZERS)),)\n BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n endif\n+ifneq ($(filter thread,$(SANITIZERS)),)\n+BASIC_CFLAGS += -DGIT_THREAD_SANITIZER\n+endif\n endif\n \n ifndef sysconfdir\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326400","messageId":"20170815141734.GA4916@tor.lan","threadId":"46590","inReplyTo":"0fd7f3184d285df8867ea44dd1adf418ebfc5ef3.1502780344.git.martin.agren@gmail.com","subject":"Re: [PATCH 1/5] convert: initialize attr_action in convert_attrs","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-08-15T14:17:34Z","receivedAt":"2017-08-15T14:17:41Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Aug 15, 2017 at 02:53:01PM +0200, Martin Ågren wrote:\n> convert_attrs populates a struct conv_attrs. The field attr_action is\n> not set in all code paths, but still one caller unconditionally reads\n> it. Since git_check_attr always returns the same value, we'll always end\n> up in the same code path and there is no problem right now. But\n> convert_attrs is obviously trying not to rely on such an\n> implementation-detail of another component.\n> \n> Initialize attr_action to CRLF_UNDEFINED in the dead code path.\n> \n> Actually, in the code path that /is/ taken, the variable is assigned to\n> twice and the first assignment has no effect. That's not wrong, but\n> let's remove that first assignment while we're here.\n> \n> Signed-off-by: Martin Ågren \n> ---\n> I hit a warning about attr_action possibly being uninitialized when\n> building with SANITIZE=thread. I guess it's some random interaction\n> between code added by tsan, the optimizer (-O3) and the warning\n> machinery. (This was with gcc 5.4.0.)\n> \n> convert.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/convert.c b/convert.c\n> index 1012462e3..943d957b4 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -1040,7 +1040,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n> \t\tca->crlf_action = git_path_check_crlf(ccheck + 4);\n> \t\tif (ca->crlf_action == CRLF_UNDEFINED)\n> \t\t\tca->crlf_action = git_path_check_crlf(ccheck + 0);\n> -\t\tca->attr_action = ca->crlf_action;\n\nI don't think the removal of that line is correct.\n\n> \t\tca->ident = git_path_check_ident(ccheck + 1);\n> \t\tca->drv = git_path_check_convert(ccheck + 2);\n> \t\tif (ca->crlf_action != CRLF_BINARY) {\n> @@ -1058,6 +1057,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n> \t} else {\n> \t\tca->drv = NULL;\n> \t\tca->crlf_action = CRLF_UNDEFINED;\n> +\t\tca->attr_action = CRLF_UNDEFINED;\n\nBut this one can be avoided, when the line\nca->attr_action = ca->crlf_action;\nwould move completely out of the \"if/else\" block.\n\n> \t\tca->ident = 0;\n> \t}\n> \tif (ca->crlf_action == CRLF_TEXT)\n> -- \n> 2.14.1.151.gdfeca7a7e\n> \n\nThanks for spotting my mess.\nWhat do you think about the following:\n\n\ndiff --git a/convert.c b/convert.c\nindex 1012462e3c..fd91b91ada 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1040,7 +1040,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t\tca->crlf_action = git_path_check_crlf(ccheck + 4);\n \t\tif (ca->crlf_action == CRLF_UNDEFINED)\n \t\t\tca->crlf_action = git_path_check_crlf(ccheck + 0);\n-\t\tca->attr_action = ca->crlf_action;\n \t\tca->ident = git_path_check_ident(ccheck + 1);\n \t\tca->drv = git_path_check_convert(ccheck + 2);\n \t\tif (ca->crlf_action != CRLF_BINARY) {\n@@ -1060,6 +1059,8 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t\tca->crlf_action = CRLF_UNDEFINED;\n \t\tca->ident = 0;\n \t}\n+\t/* Save attr and make a decision for action */\n+\tca->attr_action = ca->crlf_action;\n \tif (ca->crlf_action == CRLF_TEXT)\n \t\tca->crlf_action = text_eol_is_crlf() ? CRLF_TEXT_CRLF : CRLF_TEXT_INPUT;\n \tif (ca->crlf_action == CRLF_UNDEFINED && auto_crlf == AUTO_CRLF_FALSE)\n"},{"id":"326403","messageId":"20170815142922.GB4916@tor.lan","threadId":"46590","inReplyTo":"20170815141734.GA4916@tor.lan","subject":"Re: [PATCH 1/5] convert: initialize attr_action in convert_attrs","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-08-15T14:29:22Z","receivedAt":"2017-08-15T14:29:29Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n> > diff --git a/convert.c b/convert.c\n> > index 1012462e3..943d957b4 100644\n> > --- a/convert.c\n> > +++ b/convert.c\n> > @@ -1040,7 +1040,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n> > \t\tca->crlf_action = git_path_check_crlf(ccheck + 4);\n> > \t\tif (ca->crlf_action == CRLF_UNDEFINED)\n> > \t\t\tca->crlf_action = git_path_check_crlf(ccheck + 0);\n> > -\t\tca->attr_action = ca->crlf_action;\n> \n> I don't think the removal of that line is correct.\n\nSorry, that went wrong, should have been:\n I think that the line can be removed here.\n\n"},{"id":"326405","messageId":"CAN0heSpF5OszFabkFC7Rp8XykUYdFc0PXWzOmVJEHcuFxJPxyg@mail.gmail.com","threadId":"46590","inReplyTo":"20170815141734.GA4916@tor.lan","subject":"Re: [PATCH 1/5] convert: initialize attr_action in convert_attrs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T14:40:46Z","receivedAt":"2017-08-15T14:40:52Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 15 August 2017 at 16:17, Torsten Bögershausen wrote:\n> On Tue, Aug 15, 2017 at 02:53:01PM +0200, Martin Ågren wrote:\n>> convert_attrs populates a struct conv_attrs. The field attr_action is\n>> not set in all code paths, but still one caller unconditionally reads\n>> it. Since git_check_attr always returns the same value, we'll always end\n>> up in the same code path and there is no problem right now. But\n>> convert_attrs is obviously trying not to rely on such an\n>> implementation-detail of another component.\n>>\n>> Initialize attr_action to CRLF_UNDEFINED in the dead code path.\n>>\n>> Actually, in the code path that /is/ taken, the variable is assigned to\n>> twice and the first assignment has no effect. That's not wrong, but\n>> let's remove that first assignment while we're here.\n>>\n>> Signed-off-by: Martin Ågren \n>> ---\n>> I hit a warning about attr_action possibly being uninitialized when\n>> building with SANITIZE=thread. I guess it's some random interaction\n>> between code added by tsan, the optimizer (-O3) and the warning\n>> machinery. (This was with gcc 5.4.0.)\n>>\n>> convert.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/convert.c b/convert.c\n>> index 1012462e3..943d957b4 100644\n>> --- a/convert.c\n>> +++ b/convert.c\n>> @@ -1040,7 +1040,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n>> ca->crlf_action = git_path_check_crlf(ccheck + 4);\n>> if (ca->crlf_action == CRLF_UNDEFINED)\n>> ca->crlf_action = git_path_check_crlf(ccheck + 0);\n>> - ca->attr_action = ca->crlf_action;\n>\n> I don't think the removal of that line is correct.\n\n(Thanks for confirming in a follow-up mail that you meant that you /do/\nthink the removal is correct.)\n\n>\n>> ca->ident = git_path_check_ident(ccheck + 1);\n>> ca->drv = git_path_check_convert(ccheck + 2);\n>> if (ca->crlf_action != CRLF_BINARY) {\n>> @@ -1058,6 +1057,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n>> } else {\n>> ca->drv = NULL;\n>> ca->crlf_action = CRLF_UNDEFINED;\n>> + ca->attr_action = CRLF_UNDEFINED;\n>\n> But this one can be avoided, when the line\n> ca->attr_action = ca->crlf_action;\n> would move completely out of the \"if/else\" block.\n>\n>> ca->ident = 0;\n>> }\n>> if (ca->crlf_action == CRLF_TEXT)\n>> --\n>> 2.14.1.151.gdfeca7a7e\n>>\n>\n> Thanks for spotting my mess.\n> What do you think about the following:\n>\n>\n> diff --git a/convert.c b/convert.c\n> index 1012462e3c..fd91b91ada 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -1040,7 +1040,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n> ca->crlf_action = git_path_check_crlf(ccheck + 4);\n> if (ca->crlf_action == CRLF_UNDEFINED)\n> ca->crlf_action = git_path_check_crlf(ccheck + 0);\n> - ca->attr_action = ca->crlf_action;\n> ca->ident = git_path_check_ident(ccheck + 1);\n> ca->drv = git_path_check_convert(ccheck + 2);\n> if (ca->crlf_action != CRLF_BINARY) {\n> @@ -1060,6 +1059,8 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n> ca->crlf_action = CRLF_UNDEFINED;\n> ca->ident = 0;\n> }\n> + /* Save attr and make a decision for action */\n> + ca->attr_action = ca->crlf_action;\n> if (ca->crlf_action == CRLF_TEXT)\n> ca->crlf_action = text_eol_is_crlf() ? CRLF_TEXT_CRLF : CRLF_TEXT_INPUT;\n> if (ca->crlf_action == CRLF_UNDEFINED && auto_crlf == AUTO_CRLF_FALSE)\n\nYeah, makes lots of sense. Then we could also remove the second\nassignment to attr_action. That is, this function would set attr_action\nat one place, always. I'll do this in a v2.\n\nThanks.\n"},{"id":"326417","messageId":"CAGZ79kYbPvmqV1PR_NjxLbp2vqC6=JdP+-hzdwQvUmVLZU_Wtg@mail.gmail.com","threadId":"46590","inReplyTo":"939b37f809dd9e1526593c02154fae14b369c73a.1502780344.git.martin.agren@gmail.com","subject":"Re: tsan: t5400: set_try_to_free_routine","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T17:35:00Z","receivedAt":"2017-08-15T17:35:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 5:53 AM, Martin Ågren wrote:\n> Using SANITIZE=thread made t5400-send-pack.sh hit the potential race\n> below.\n>\n> This is set_try_to_free_routine in wrapper.c. The race relates to the\n> reading of the \"old\" value. The caller doesn't care about the \"old\"\n> value, so this should be harmless right now. But it seems that using\n> this mechanism from multiple threads and restoring the earlier value\n> will probably not work out every time. (Not necessarily because of the\n> race in set_try_to_free_routine, but, e.g., because callers might not\n> restore the function pointer in the reverse order of how they\n> originally set it.)\n>\n> Properly \"fixing\" this for thread-safety would probably require some\n> redesigning, which at the time might not be warranted. I'm just posting\n> this for completeness.\n\nMaybe related read (also error handling in threads):\nhttps://public-inbox.org/git/20170619220036.22656-1-avarab@gmail.com/\n\n>\n> Martin\n>\n> WARNING: ThreadSanitizer: data race (pid=21382)\n> Read of size 8 at 0x000000979970 by thread T1:\n> #0 set_try_to_free_routine wrapper.c:35 (git+0x0000006cde1c)\n> #1 prepare_trace_line trace.c:105 (git+0x0000006a3bf0)\n> #2 trace_strbuf_fl trace.c:185 (git+0x0000006a3bf0)\n> #3 packet_trace pkt-line.c:80 (git+0x0000005f9f43)\n> #4 packet_read pkt-line.c:309 (git+0x0000005fbe10)\n> #5 recv_sideband sideband.c:37 (git+0x000000684c5e)\n> #6 sideband_demux send-pack.c:216 (git+0x00000065a38c)\n> #7 run_thread run-command.c:933 (git+0x000000655a93)\n> #8 (libtsan.so.0+0x0000000230d9)\n>\n> Previous write of size 8 at 0x000000979970 by main thread:\n> #0 set_try_to_free_routine wrapper.c:38 (git+0x0000006cde39)\n> #1 prepare_trace_line trace.c:105 (git+0x0000006a3bf0)\n> #2 trace_strbuf_fl trace.c:185 (git+0x0000006a3bf0)\n> #3 packet_trace pkt-line.c:80 (git+0x0000005f9f43)\n> #4 packet_read pkt-line.c:324 (git+0x0000005fc0bb)\n> #5 packet_read_line_generic pkt-line.c:332 (git+0x0000005fc0bb)\n> #6 packet_read_line pkt-line.c:342 (git+0x0000005fc0bb)\n> #7 receive_unpack_status send-pack.c:149 (git+0x00000065c1e4)\n> #8 send_pack send-pack.c:581 (git+0x00000065c1e4)\n> #9 git_transport_push transport.c:574 (git+0x0000006ab2c1)\n> #10 transport_push transport.c:1068 (git+0x0000006ae5d5)\n> #11 push_with_options builtin/push.c:336 (git+0x00000049d580)\n> #12 do_push builtin/push.c:419 (git+0x00000049f57d)\n> #13 cmd_push builtin/push.c:591 (git+0x00000049f57d)\n> #14 run_builtin git.c:330 (git+0x00000040949e)\n> #15 handle_builtin git.c:538 (git+0x00000040949e)\n> #16 run_argv git.c:590 (git+0x000000409a0e)\n> #17 cmd_main git.c:667 (git+0x000000409a0e)\n> #18 main common-main.c:43 (git+0x0000004079d1)\n>\n> Location is global 'try_to_free_routine' of size 8 at 0x000000979970 (git+0x000000979970)\n>\n> Thread T1 (tid=21405, running) created by main thread at:\n> #0 pthread_create (libtsan.so.0+0x000000027577)\n> #1 start_async run-command.c:1130 (git+0x0000006582e7)\n> #2 send_pack send-pack.c:562 (git+0x00000065b7c8)\n> #3 git_transport_push transport.c:574 (git+0x0000006ab2c1)\n> #4 transport_push transport.c:1068 (git+0x0000006ae5d5)\n> #5 push_with_options builtin/push.c:336 (git+0x00000049d580)\n> #6 do_push builtin/push.c:419 (git+0x00000049f57d)\n> #7 cmd_push builtin/push.c:591 (git+0x00000049f57d)\n> #8 run_builtin git.c:330 (git+0x00000040949e)\n> #9 handle_builtin git.c:538 (git+0x00000040949e)\n> #10 run_argv git.c:590 (git+0x000000409a0e)\n> #11 cmd_main git.c:667 (git+0x000000409a0e)\n> #12 main common-main.c:43 (git+0x0000004079d1)\n>\n> SUMMARY: ThreadSanitizer: data race wrapper.c:35 set_try_to_free_routine\n>\n"},{"id":"326423","messageId":"cff383c2-ca57-caba-5a46-7dec4abc25a4@jeffhostetler.com","threadId":"46590","inReplyTo":"adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com","subject":"Re: tsan: t3008: hashmap_add touches size from multiple threads","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-08-15T17:59:22Z","receivedAt":"2017-08-15T17:59:28Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/15/2017 8:53 AM, Martin Ågren wrote:\n> Using SANITIZE=thread made t3008-ls-files-lazy-init-name-hash.sh hit\n> the potential race below.\n> \n> What seems to happen is, threaded_lazy_init_name_hash ends up using\n> hashmap_add on the_index.dir_hash from two threads in a way that tsan\n> considers racy. While the buckets each have their own mutex, the \"size\"\n> does not. So it might end up with the wrong (lower) value. The size is\n> used to determine when to resize, but that should be fine, since\n> resizing is turned off due to threading anyway.\n >\n> If the size is ever used for something else, the consequences might\n> range from cosmetic error to devastating. I have a \"feeling\" the size is\n> not used at the time, although maybe it is, in some roundabout way which\n> I'm not seeing.\n\nGood catch! Yes, the size field is unguarded. The only references to\nit that I found were used in _add() and _remove() in the need-to-rehash\ncheck.\n\nSince auto-rehash is turned off, this shouldn't be a problem, but it\ndoes feel odd to have a possibly incorrect size due to races.\n\nRather than adding something like (a cross-platform version of)\nInterlockedIncrement(), I'm wondering if it would be better to\ndisable/invalidate the size field when auto-rehash is turned off\nso we don't have to worry about it.\n\nSomething like this:\n\n\n$ git diff\ndiff --git a/hashmap.c b/hashmap.c\nindex 9b6a12859b..be97642daa 100644\n--- a/hashmap.c\n+++ b/hashmap.c\n@@ -205,6 +205,9 @@ void hashmap_add(struct hashmap *map, void *entry)\n ((struct hashmap_entry *) entry)->next = map->table[b];\n map->table[b] = entry;\n\n+ if (map->disallow_rehash)\n+ return;\n+\n /* fix size and rehash if appropriate */\n map->size++;\n if (map->size > map->grow_at)\n@@ -223,6 +226,9 @@ void *hashmap_remove(struct hashmap *map, const void \n*key, const void *keydata)\n *e = old->next;\n old->next = NULL;\n\n+ if (map->disallow_rehash)\n+ return;\n+\n /* fix size and rehash if appropriate */\n map->size--;\n if (map->size < map->shrink_at)\ndiff --git a/hashmap.h b/hashmap.h\nindex 7a8fa7fa3d..ec9541b981 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -183,7 +183,8 @@ struct hashmap {\n const void *cmpfn_data;\n\n /* total number of entries (0 means the hashmap is empty) */\n- unsigned int size;\n+ /* -1 means size is unknown for threading reasons */\n+ int size;\n\n /*\n * tablesize is the allocated size of the hash table. A non-0 value\n@@ -360,6 +361,15 @@ int hashmap_bucket(const struct hashmap *map, \nunsigned int hash);\n static inline void hashmap_disallow_rehash(struct hashmap *map, \nunsigned value)\n {\n map->disallow_rehash = value;\n+ if (value) {\n+ /*\n+ * Assume threaded operations starting, so don't\n+ * try to keep size current.\n+ */\n+ size = -1;\n+ } else {\n+ /* TODO count items in all buckets and set size. */\n+ }\n }\n\n /*\n\n\nJeff\n"},{"id":"326425","messageId":"CAGZ79kbf52Uu-Th9W20QZV204A81kOAPTj2x6JkEP1rN=GTYtw@mail.gmail.com","threadId":"46590","inReplyTo":"cff383c2-ca57-caba-5a46-7dec4abc25a4@jeffhostetler.com","subject":"Re: tsan: t3008: hashmap_add touches size from multiple threads","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T18:17:05Z","receivedAt":"2017-08-15T18:17:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 10:59 AM, Jeff Hostetler wrote:\n>\n>\n> On 8/15/2017 8:53 AM, Martin Ågren wrote:\n>>\n>> Using SANITIZE=thread made t3008-ls-files-lazy-init-name-hash.sh hit\n>> the potential race below.\n>>\n>> What seems to happen is, threaded_lazy_init_name_hash ends up using\n>> hashmap_add on the_index.dir_hash from two threads in a way that tsan\n>> considers racy. While the buckets each have their own mutex, the \"size\"\n>> does not. So it might end up with the wrong (lower) value. The size is\n>> used to determine when to resize, but that should be fine, since\n>> resizing is turned off due to threading anyway.\n>\n>>\n>>\n>> If the size is ever used for something else, the consequences might\n>> range from cosmetic error to devastating. I have a \"feeling\" the size is\n>> not used at the time, although maybe it is, in some roundabout way which\n>> I'm not seeing.\n>\n>\n> Good catch! Yes, the size field is unguarded. The only references to\n> it that I found were used in _add() and _remove() in the need-to-rehash\n> check.\n>\n> Since auto-rehash is turned off, this shouldn't be a problem, but it\n> does feel odd to have a possibly incorrect size due to races.\n>\n> Rather than adding something like (a cross-platform version of)\n> InterlockedIncrement(), I'm wondering if it would be better to\n> disable/invalidate the size field when auto-rehash is turned off\n> so we don't have to worry about it.\n>\n> Something like this:\n>\n>\n> $ git diff\n> diff --git a/hashmap.c b/hashmap.c\n> index 9b6a12859b..be97642daa 100644\n> --- a/hashmap.c\n> +++ b/hashmap.c\n> @@ -205,6 +205,9 @@ void hashmap_add(struct hashmap *map, void *entry)\n> ((struct hashmap_entry *) entry)->next = map->table[b];\n> map->table[b] = entry;\n>\n> + if (map->disallow_rehash)\n> + return;\n> +\n> /* fix size and rehash if appropriate */\n> map->size++;\n> if (map->size > map->grow_at)\n> @@ -223,6 +226,9 @@ void *hashmap_remove(struct hashmap *map, const void\n> *key, const void *keydata)\n> *e = old->next;\n> old->next = NULL;\n>\n> + if (map->disallow_rehash)\n> + return;\n> +\n\n\nOnce we have these two checks, the check in rehash itself becomes\nredundant (as any code path leading to the check in rehash already\nhad the check).\n\nWhich leads me to wonder if we want to make the size (in/de)crease\npart of the rehash function, which could be\n\n adjust_size(struct hashmap *map, int n)\n\nwith `n` either +1 or -1 (the increase value). Depending on 'n', we could\ncompute the newsize for the potential rehash in that function as well.\n\nNote sure if that is a cleaner internal API.\n\n> /* fix size and rehash if appropriate */\n> map->size--;\n> if (map->size < map->shrink_at)\n> diff --git a/hashmap.h b/hashmap.h\n> index 7a8fa7fa3d..ec9541b981 100644\n> --- a/hashmap.h\n> +++ b/hashmap.h\n> @@ -183,7 +183,8 @@ struct hashmap {\n> const void *cmpfn_data;\n>\n> /* total number of entries (0 means the hashmap is empty) */\n> - unsigned int size;\n> + /* -1 means size is unknown for threading reasons */\n> + int size;\n\nThis double-encodes the state of disallow_rehash (i.e. if we had\nsigned size, then the invariant disallow_rehash === (size < 0)\nis true, such that we could omit either the flag and just check for\nsize < 0 or we do not need the negative size as any user would\nneed to check disallow_rehash first. Not sure which API is harder\nto misuse. I'd think just having the size and getting rid of\ndisallow_rehash might be hard to to reused.\n\n>\n> /*\n> * tablesize is the allocated size of the hash table. A non-0 value\n> @@ -360,6 +361,15 @@ int hashmap_bucket(const struct hashmap *map, unsigned\n> int hash);\n> static inline void hashmap_disallow_rehash(struct hashmap *map, unsigned\n> value)\n> {\n> map->disallow_rehash = value;\n> + if (value) {\n> + /*\n> + * Assume threaded operations starting, so don't\n> + * try to keep size current.\n> + */\n> + size = -1;\n> + } else {\n> + /* TODO count items in all buckets and set size. */\n> + }\n> }\n>\n> /*\n>\n>\n> Jeff\n"},{"id":"326433","messageId":"CAN0heSoXysu=6E_ScfWQVLOk805V=j7AYJi=z62SmNkP5U=A9Q@mail.gmail.com","threadId":"46590","inReplyTo":"CAGZ79kbf52Uu-Th9W20QZV204A81kOAPTj2x6JkEP1rN=GTYtw@mail.gmail.com","subject":"Re: tsan: t3008: hashmap_add touches size from multiple threads","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T18:40:37Z","receivedAt":"2017-08-15T18:40:49Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 15 August 2017 at 20:17, Stefan Beller wrote:\n> On Tue, Aug 15, 2017 at 10:59 AM, Jeff Hostetler wrote:\n>>\n>>\n>> On 8/15/2017 8:53 AM, Martin Ågren wrote:\n>>>\n>>> Using SANITIZE=thread made t3008-ls-files-lazy-init-name-hash.sh hit\n>>> the potential race below.\n>>>\n>>> What seems to happen is, threaded_lazy_init_name_hash ends up using\n>>> hashmap_add on the_index.dir_hash from two threads in a way that tsan\n>>> considers racy. While the buckets each have their own mutex, the \"size\"\n>>> does not. So it might end up with the wrong (lower) value. The size is\n>>> used to determine when to resize, but that should be fine, since\n>>> resizing is turned off due to threading anyway.\n>>\n>>>\n>>>\n>>> If the size is ever used for something else, the consequences might\n>>> range from cosmetic error to devastating. I have a \"feeling\" the size is\n>>> not used at the time, although maybe it is, in some roundabout way which\n>>> I'm not seeing.\n>>\n>>\n>> Good catch! Yes, the size field is unguarded. The only references to\n>> it that I found were used in _add() and _remove() in the need-to-rehash\n>> check.\n>>\n>> Since auto-rehash is turned off, this shouldn't be a problem, but it\n>> does feel odd to have a possibly incorrect size due to races.\n>>\n>> Rather than adding something like (a cross-platform version of)\n>> InterlockedIncrement(), I'm wondering if it would be better to\n>> disable/invalidate the size field when auto-rehash is turned off\n>> so we don't have to worry about it.\n>>\n>> Something like this:\n>>\n>>\n>> $ git diff\n>> diff --git a/hashmap.c b/hashmap.c\n>> index 9b6a12859b..be97642daa 100644\n>> --- a/hashmap.c\n>> +++ b/hashmap.c\n>> @@ -205,6 +205,9 @@ void hashmap_add(struct hashmap *map, void *entry)\n>> ((struct hashmap_entry *) entry)->next = map->table[b];\n>> map->table[b] = entry;\n>>\n>> + if (map->disallow_rehash)\n>> + return;\n>> +\n>> /* fix size and rehash if appropriate */\n>> map->size++;\n>> if (map->size > map->grow_at)\n>> @@ -223,6 +226,9 @@ void *hashmap_remove(struct hashmap *map, const void\n>> *key, const void *keydata)\n>> *e = old->next;\n>> old->next = NULL;\n>>\n>> + if (map->disallow_rehash)\n>> + return;\n>> +\n>\n>\n> Once we have these two checks, the check in rehash itself becomes\n> redundant (as any code path leading to the check in rehash already\n> had the check).\n>\n> Which leads me to wonder if we want to make the size (in/de)crease\n> part of the rehash function, which could be\n>\n> adjust_size(struct hashmap *map, int n)\n>\n> with `n` either +1 or -1 (the increase value). Depending on 'n', we could\n> compute the newsize for the potential rehash in that function as well.\n>\n> Note sure if that is a cleaner internal API.\n>\n>> /* fix size and rehash if appropriate */\n>> map->size--;\n>> if (map->size < map->shrink_at)\n>> diff --git a/hashmap.h b/hashmap.h\n>> index 7a8fa7fa3d..ec9541b981 100644\n>> --- a/hashmap.h\n>> +++ b/hashmap.h\n>> @@ -183,7 +183,8 @@ struct hashmap {\n>> const void *cmpfn_data;\n>>\n>> /* total number of entries (0 means the hashmap is empty) */\n>> - unsigned int size;\n>> + /* -1 means size is unknown for threading reasons */\n>> + int size;\n>\n> This double-encodes the state of disallow_rehash (i.e. if we had\n> signed size, then the invariant disallow_rehash === (size < 0)\n> is true, such that we could omit either the flag and just check for\n> size < 0 or we do not need the negative size as any user would\n> need to check disallow_rehash first. Not sure which API is harder\n> to misuse. I'd think just having the size and getting rid of\n> disallow_rehash might be hard to to reused.\n\n(Do you mean \"might be hard to be misused\"?)\n\nOne good thing about turning off the size-tracking with threading is\nthat someone who later wants to know the size in a threaded application\nwill not introduce any subtle bugs by misusing size, but will be forced\nto provide and use some sort of InterlockedIncrement(). When/if that\nchange happens, it would be nice if no-one relied on the value of size\nto say anything about threading. So it might make sense to have an\nimplementation-independent way of accessing disallow_rehash a.k.a.\n(size < 0).\n\nFor example a function hashmap_disallow_rehash(), except that's\nobviously taken. :-) Maybe the existing function would then be\nhashmap_set_disallow_rehash(). Oh well..\n\n>\n>>\n>> /*\n>> * tablesize is the allocated size of the hash table. A non-0 value\n>> @@ -360,6 +361,15 @@ int hashmap_bucket(const struct hashmap *map, unsigned\n>> int hash);\n>> static inline void hashmap_disallow_rehash(struct hashmap *map, unsigned\n>> value)\n>> {\n>> map->disallow_rehash = value;\n>> + if (value) {\n>> + /*\n>> + * Assume threaded operations starting, so don't\n>> + * try to keep size current.\n>> + */\n>> + size = -1;\n>> + } else {\n>> + /* TODO count items in all buckets and set size. */\n>> + }\n>> }\n>>\n>> /*\n>>\n>>\n>> Jeff\n"},{"id":"326434","messageId":"xmqqk224r7rv.fsf@gitster.mtv.corp.google.com","threadId":"46590","inReplyTo":"931ffb00319f40e3c9e099f17eeae6a0c1de41ea.1502780344.git.martin.agren@gmail.com","subject":"Re: [PATCH 4/5] strbuf_reset: don't write to slopbuf with ThreadSanitizer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T18:43:48Z","receivedAt":"2017-08-15T18:44:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> If two threads have one freshly initialized string buffer each and call\n> strbuf_reset on them at roughly the same time, both threads will be\n> writing a '\\0' to strbuf_slopbuf. That is not a problem in practice\n> since it doesn't matter in which order the writes happen. But\n> ThreadSanitizer will consider this a race.\n>\n> When compiling with GIT_THREAD_SANITIZER, avoid writing to\n> strbuf_slopbuf. Let's instead assert on the first byte of strbuf_slopbuf\n> being '\\0', since it ensures the promised invariant of \"buf[len] ==\n> '\\0'\". (Writing to strbuf_slopbuf is normally bad, but could become even\n> more bad if we stop covering it up in strbuf_reset.)\n>\n> Signed-off-by: Martin Ågren \n> ---\n> strbuf.h | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/strbuf.h b/strbuf.h\n> index e705b94db..295654d39 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -153,7 +153,19 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n> /**\n> * Empty the buffer by setting the size of it to zero.\n> */\n> +#ifdef GIT_THREAD_SANITIZER\n> +#define strbuf_reset(sb)\t\t\t\t\t\t\\\n> +\tdo {\t\t\t\t\t\t\t\t\\\n> +\t\tstruct strbuf *_sb = sb; \t\t\t\t\\\n> +\t\t_sb->len = 0;\t\t\t\t\t\t\\\n> +\t\tif (_sb->buf == strbuf_slopbuf)\t\t\t\t\\\n> +\t\t\tassert(!strbuf_slopbuf[0]);\t\t\t\\\n> +\t\telse\t\t\t\t\t\t\t\\\n> +\t\t\t_sb->buf[0] = '\\0';\t\t\t\t\\\n> +\t} while (0)\n> +#else\n> #define strbuf_reset(sb) strbuf_setlen(sb, 0)\n> +#endif\n> \n> \n> /**\n\nThe strbuf_slopbuf[] is a shared resource that is expected by\neverybody to stay a holder of a NUL. Even though it is defined as\n\"char [1]\", it in spirit ought to be considered const. And from\nthat point of view, your new definition that is conditionally used\nonly when sanitizer is in use _is_ the more correct one than the\ncurrent \"we do not care if it is slopbuf, we are writing \\0 so it\nwill be no-op anyway\" code.\n\nI wonder if we excessively call strbuf_reset() in the real code to\nmake your version unacceptably expensive? If not, I somehow feel\nthat using this version unconditionally may be a better approach.\n\nWhat happens when a caller calls \"strbuf_setlen(&sb, 0)\" on a strbuf\nthat happens to have nothing and whose buffer still points at the\nslopbuf (instead of calling _reset())? Shouldn't your patch fix\nthat function instead, i.e. something like the following without the\nabove? Is that make things noticeably and measurably too expensive?\n\nThanks.\n\n strbuf.h | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 2075384e0b..1a77fe146a 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n \tif (len > (sb->alloc ? sb->alloc - 1 : 0))\n \t\tdie(\"BUG: strbuf_setlen() beyond buffer\");\n \tsb->len = len;\n-\tsb->buf[len] = '\\0';\n+\tif (sb->buf != strbuf_slopbuf)\n+\t\tsb->buf[len] = '\\0';\n+\telse\n+\t\tassert(!strbuf_slopbuf[0]);\n }\n \n /**\n"},{"id":"326435","messageId":"CAN0heSpPihOcXgP-iCkBFvvP2x-8=oxYGUFJbUhF8mwcYfgoxA@mail.gmail.com","threadId":"46590","inReplyTo":"CAGZ79kYbPvmqV1PR_NjxLbp2vqC6=JdP+-hzdwQvUmVLZU_Wtg@mail.gmail.com","subject":"Re: tsan: t5400: set_try_to_free_routine","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T18:44:24Z","receivedAt":"2017-08-15T18:44:30Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 15 August 2017 at 19:35, Stefan Beller wrote:\n> On Tue, Aug 15, 2017 at 5:53 AM, Martin Ågren wrote:\n>> Using SANITIZE=thread made t5400-send-pack.sh hit the potential race\n>> below.\n>>\n>> This is set_try_to_free_routine in wrapper.c. The race relates to the\n>> reading of the \"old\" value. The caller doesn't care about the \"old\"\n>> value, so this should be harmless right now. But it seems that using\n>> this mechanism from multiple threads and restoring the earlier value\n>> will probably not work out every time. (Not necessarily because of the\n>> race in set_try_to_free_routine, but, e.g., because callers might not\n>> restore the function pointer in the reverse order of how they\n>> originally set it.)\n>>\n>> Properly \"fixing\" this for thread-safety would probably require some\n>> redesigning, which at the time might not be warranted. I'm just posting\n>> this for completeness.\n>\n> Maybe related read (also error handling in threads):\n> https://public-inbox.org/git/20170619220036.22656-1-avarab@gmail.com/\n\nThanks for the pointer. Threading is tricky business, indeed. I still\nhaven't entirely groked all the users of set_try_to_free_routine, i.e.,\nwhat they want to avoid and what they want to achieve. I'm just happy\nthat the only user which cares about the old value is not threaded, to\nthe best of my understanding.\n"},{"id":"326437","messageId":"CAGZ79kb-1S9F4Pp0dzkDX488uiZ8Zu_1m2U=hQ1CcsgSu314rQ@mail.gmail.com","threadId":"46590","inReplyTo":"CAN0heSoXysu=6E_ScfWQVLOk805V=j7AYJi=z62SmNkP5U=A9Q@mail.gmail.com","subject":"Re: tsan: t3008: hashmap_add touches size from multiple threads","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T18:48:28Z","receivedAt":"2017-08-15T18:48:33Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">>> /* total number of entries (0 means the hashmap is empty) */\n>>> - unsigned int size;\n>>> + /* -1 means size is unknown for threading reasons */\n>>> + int size;\n>>\n>> This double-encodes the state of disallow_rehash (i.e. if we had\n>> signed size, then the invariant disallow_rehash === (size < 0)\n>> is true, such that we could omit either the flag and just check for\n>> size < 0 or we do not need the negative size as any user would\n>> need to check disallow_rehash first. Not sure which API is harder\n>> to misuse. I'd think just having the size and getting rid of\n>> disallow_rehash might be hard to to reused.\n>\n> (Do you mean \"might be hard to be misused\"?)\n\nyes, I do.\n\n> One good thing about turning off the size-tracking with threading is\n> that someone who later wants to know the size in a threaded application\n> will not introduce any subtle bugs by misusing size, but will be forced\n> to provide and use some sort of InterlockedIncrement().\n\nagreed.\n\n> When/if that\n> change happens, it would be nice if no-one relied on the value of size\n> to say anything about threading. So it might make sense to have an\n> implementation-independent way of accessing disallow_rehash a.k.a.\n> (size < 0).\n\nYes, and my point was whether we want to keep disallow_rehash around,\nas when a patch as this is applied, we'd have it encoded twice,\nboth size < 0 as well as disallow_rehash set indicate the rehashing\ndisabled.\n\nIf we were to reduce it to one, we would not have \"invalid\" state possible\nsuch as size < 0 and disallow_rehash = 0.\n\nIn the future we may have more options that make size impossible to\ncompute efficiently, such that in that case we'd want to know which\ncondition lead to it. In that case we'd want to have the flags around.\n\n> For example a function hashmap_disallow_rehash(), except that's\n> obviously taken. :-) Maybe the existing function would then be\n> hashmap_set_disallow_rehash(). Oh well..\n\nNot sure I understand this one.\n"},{"id":"326442","messageId":"CAN0heSqwBNqrQPxOFZPCdFDA58P0JsKUqrw-KhVCcE1WKFTKbA@mail.gmail.com","threadId":"46590","inReplyTo":"xmqqk224r7rv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/5] strbuf_reset: don't write to slopbuf with ThreadSanitizer","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T19:06:13Z","receivedAt":"2017-08-15T19:06:19Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 15 August 2017 at 20:43, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> If two threads have one freshly initialized string buffer each and call\n>> strbuf_reset on them at roughly the same time, both threads will be\n>> writing a '\\0' to strbuf_slopbuf. That is not a problem in practice\n>> since it doesn't matter in which order the writes happen. But\n>> ThreadSanitizer will consider this a race.\n>>\n>> When compiling with GIT_THREAD_SANITIZER, avoid writing to\n>> strbuf_slopbuf. Let's instead assert on the first byte of strbuf_slopbuf\n>> being '\\0', since it ensures the promised invariant of \"buf[len] ==\n>> '\\0'\". (Writing to strbuf_slopbuf is normally bad, but could become even\n>> more bad if we stop covering it up in strbuf_reset.)\n>>\n>> Signed-off-by: Martin Ågren \n>> ---\n>> strbuf.h | 12 ++++++++++++\n>> 1 file changed, 12 insertions(+)\n>>\n>> diff --git a/strbuf.h b/strbuf.h\n>> index e705b94db..295654d39 100644\n>> --- a/strbuf.h\n>> +++ b/strbuf.h\n>> @@ -153,7 +153,19 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n>> /**\n>> * Empty the buffer by setting the size of it to zero.\n>> */\n>> +#ifdef GIT_THREAD_SANITIZER\n>> +#define strbuf_reset(sb) \\\n>> + do { \\\n>> + struct strbuf *_sb = sb; \\\n>> + _sb->len = 0; \\\n>> + if (_sb->buf == strbuf_slopbuf) \\\n>> + assert(!strbuf_slopbuf[0]); \\\n>> + else \\\n>> + _sb->buf[0] = '\\0'; \\\n>> + } while (0)\n>> +#else\n>> #define strbuf_reset(sb) strbuf_setlen(sb, 0)\n>> +#endif\n>>\n>>\n>> /**\n>\n> The strbuf_slopbuf[] is a shared resource that is expected by\n> everybody to stay a holder of a NUL. Even though it is defined as\n> \"char [1]\", it in spirit ought to be considered const. And from\n> that point of view, your new definition that is conditionally used\n> only when sanitizer is in use _is_ the more correct one than the\n> current \"we do not care if it is slopbuf, we are writing \\0 so it\n> will be no-op anyway\" code.\n>\n> I wonder if we excessively call strbuf_reset() in the real code to\n> make your version unacceptably expensive? If not, I somehow feel\n> that using this version unconditionally may be a better approach.\n>\n> What happens when a caller calls \"strbuf_setlen(&sb, 0)\" on a strbuf\n> that happens to have nothing and whose buffer still points at the\n> slopbuf (instead of calling _reset())? Shouldn't your patch fix\n> that function instead, i.e. something like the following without the\n> above? Is that make things noticeably and measurably too expensive?\n\nGood thinking. There are about 300 users of strbuf_reset and 10 users of\nstrbuf_setlen(., 0) with a literal zero. Obviously, there might be more\nusers which end up setting the length to 0 for some reason or other. So\nyour idea seems the better one. I would assume that whoever resets a\nbuffer is about to add something to it, which should be more expensive,\nbut that's obviously just hand-waving. I'll see if I can find some\ninteresting caller and/or performance numbers.\n\n> strbuf.h | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/strbuf.h b/strbuf.h\n> index 2075384e0b..1a77fe146a 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n> if (len > (sb->alloc ? sb->alloc - 1 : 0))\n> die(\"BUG: strbuf_setlen() beyond buffer\");\n> sb->len = len;\n> - sb->buf[len] = '\\0';\n> + if (sb->buf != strbuf_slopbuf)\n> + sb->buf[len] = '\\0';\n> + else\n> + assert(!strbuf_slopbuf[0]);\n> }\n>\n> /**\n\nWhen writing my patch, I used assert() and figured that with tsan, we're\nin some sort of \"debug\"-mode anyway. If we decide to always do the\ncheck, would it make sense to do \"else if (strbuf_slopbuf[0]) BUG(..);\"\ninstead of the assert? Or, if we do prefer the assert, would the\nperformance-worry be moot?\n\nThanks for the feedback.\n"},{"id":"326443","messageId":"xmqq7ey4r64l.fsf@gitster.mtv.corp.google.com","threadId":"46590","inReplyTo":"CAN0heSqwBNqrQPxOFZPCdFDA58P0JsKUqrw-KhVCcE1WKFTKbA@mail.gmail.com","subject":"Re: [PATCH 4/5] strbuf_reset: don't write to slopbuf with ThreadSanitizer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T19:19:22Z","receivedAt":"2017-08-15T19:19:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n>> diff --git a/strbuf.h b/strbuf.h\n>> index 2075384e0b..1a77fe146a 100644\n>> --- a/strbuf.h\n>> +++ b/strbuf.h\n>> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n>> if (len > (sb->alloc ? sb->alloc - 1 : 0))\n>> die(\"BUG: strbuf_setlen() beyond buffer\");\n>> sb->len = len;\n>> - sb->buf[len] = '\\0';\n>> + if (sb->buf != strbuf_slopbuf)\n>> + sb->buf[len] = '\\0';\n>> + else\n>> + assert(!strbuf_slopbuf[0]);\n>> }\n>>\n>> /**\n>\n> When writing my patch, I used assert() and figured that with tsan, we're\n> in some sort of \"debug\"-mode anyway. If we decide to always do the\n> check, would it make sense to do \"else if (strbuf_slopbuf[0]) BUG(..);\"\n> instead of the assert? Or, if we do prefer the assert, would the\n> performance-worry be moot?\n\nI wasn't thinking about performance impact of having an assert();\nthe use of it in the above was merely copied from yours ;-)\n"},{"id":"326444","messageId":"CAN0heSr-OcLJU54acTdXWx8NAo=nPD=9+DfexWZ0F7NRgRB9Dg@mail.gmail.com","threadId":"46590","inReplyTo":"CAGZ79kb-1S9F4Pp0dzkDX488uiZ8Zu_1m2U=hQ1CcsgSu314rQ@mail.gmail.com","subject":"Re: tsan: t3008: hashmap_add touches size from multiple threads","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-15T19:21:12Z","receivedAt":"2017-08-15T19:21:18Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 15 August 2017 at 20:48, Stefan Beller wrote:\n>>>> /* total number of entries (0 means the hashmap is empty) */\n>>>> - unsigned int size;\n>>>> + /* -1 means size is unknown for threading reasons */\n>>>> + int size;\n>>>\n>>> This double-encodes the state of disallow_rehash (i.e. if we had\n>>> signed size, then the invariant disallow_rehash === (size < 0)\n>>> is true, such that we could omit either the flag and just check for\n>>> size < 0 or we do not need the negative size as any user would\n>>> need to check disallow_rehash first. Not sure which API is harder\n>>> to misuse. I'd think just having the size and getting rid of\n>>> disallow_rehash might be hard to to reused.\n>>\n>> (Do you mean \"might be hard to be misused\"?)\n>\n> yes, I do.\n>\n>> One good thing about turning off the size-tracking with threading is\n>> that someone who later wants to know the size in a threaded application\n>> will not introduce any subtle bugs by misusing size, but will be forced\n>> to provide and use some sort of InterlockedIncrement().\n>\n> agreed.\n>\n>> When/if that\n>> change happens, it would be nice if no-one relied on the value of size\n>> to say anything about threading. So it might make sense to have an\n>> implementation-independent way of accessing disallow_rehash a.k.a.\n>> (size < 0).\n>\n> Yes, and my point was whether we want to keep disallow_rehash around,\n> as when a patch as this is applied, we'd have it encoded twice,\n> both size < 0 as well as disallow_rehash set indicate the rehashing\n> disabled.\n>\n> If we were to reduce it to one, we would not have \"invalid\" state possible\n> such as size < 0 and disallow_rehash = 0.\n\nAgreed.\n\n> In the future we may have more options that make size impossible to\n> compute efficiently, such that in that case we'd want to know which\n> condition lead to it. In that case we'd want to have the flags around.\n\nGood point.\n\n>> For example a function hashmap_disallow_rehash(), except that's\n>> obviously taken. :-) Maybe the existing function would then be\n>> hashmap_set_disallow_rehash(). Oh well..\n>\n> Not sure I understand this one.\n\nSorry. What I meant was, if we drop the disallow_rehash-field, someone\nmight be tempted to use size < 0 (or size == -1) to answer the question\n\"is rehashing disallowed?\". (Or \"am I threaded?\" which already is a\nquestion which the hashmap as it is today doesn't know about.)\n\nSo instead of looking at \"disallow_rehash\" one should perhaps be calling\n\"hashmap_is_disallow_rehash()\" or \"hashmap_get_disallow_rehash()\", which\nwould be implemented as \"return disallow_rehash\", or possibly \"return\nsize == -1\".\n\nExcept such names are, to the best of my understanding, not the Git-way,\nso it should be, e.g., \"hashmap_disallow_rehash()\".\n\nExcept ... that name is taken.... So to free that name up, the existing\nfunction should perhaps be renamed \"hashmap_set_disallow_rehash()\",\nagain assuming I've picked up the right conventions in my recent\nbrowsing of the Git-code.\n\nThe final \"Oh well\" was a short form of \"it began with an observation\nwhich currently has no practical effect, and is slowly turning into a\nchain of ideas on how to rebuild the interface\".\n"},{"id":"326450","messageId":"88ddf942-d7e4-ef1c-3f9c-9816d0571502@kdbg.org","threadId":"46590","inReplyTo":"5815ea4f27226b604751961c8b70355a8925f0c5.1502780344.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/5] pack-objects: take lock before accessing `remaining`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-08-15T19:50:28Z","receivedAt":"2017-08-15T19:50:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 15.08.2017 um 14:53 schrieb Martin Ågren:\n> When checking the conditional of \"while (me->remaining)\", we did not\n> hold the lock. Calling find_deltas would still be safe, since it checks\n> \"remaining\" (after taking the lock) and is able to handle all values. In\n> fact, this could (currently) not trigger any bug: a bug could happen if\n> `remaining` transitioning from zero to non-zero races with the evaluation\n> of the while-condition, but these are always separated by the\n> data_ready-mechanism.\n> \n> Make sure we have the lock when we read `remaining`. This does mean we\n> release it just so that find_deltas can take it immediately again. We\n> could tweak the contract so that the lock should be taken before calling\n> find_deltas, but let's defer that until someone can actually show that\n> \"unlock+lock\" has a measurable negative impact.\n> \n> Signed-off-by: Martin Ågren \n> ---\n> I don't think this corrects any real error. The benefits of this patch\n> would be \"future-proofs things slightly\" and \"silences tsan, so that\n> other errors don't drown in noise\". Feel free to tell me those benefits\n> are negligible and that this change actually hurts.\n> \n> builtin/pack-objects.c | 6 ++++++\n> 1 file changed, 6 insertions(+)\n> \n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index c753e9237..bd391e97a 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2170,7 +2170,10 @@ static void *threaded_find_deltas(void *arg)\n> {\n> \tstruct thread_params *me = arg;\n> \n> +\tprogress_lock();\n> \twhile (me->remaining) {\n> +\t\tprogress_unlock();\n> +\n> \t\tfind_deltas(me->list, &me->remaining,\n> \t\t\t me->window, me->depth, me->processed);\n> \n> @@ -2192,7 +2195,10 @@ static void *threaded_find_deltas(void *arg)\n> \t\t\tpthread_cond_wait(&me->cond, &me->mutex);\n> \t\tme->data_ready = 0;\n> \t\tpthread_mutex_unlock(&me->mutex);\n> +\n> +\t\tprogress_lock();\n> \t}\n> +\tprogress_unlock();\n> \t/* leave ->working 1 so that this doesn't get more work assigned */\n> \treturn NULL;\n> }\n> \n\nIt is correct that this access of me->remaining requires a lock. It \ncould be solved in the way you did. I tried to do it differently, but \nall cleaner solutions that I can think of are overkill...\n\n-- Hannes\n"},{"id":"326455","messageId":"217f3160-f4cb-581a-b7f3-8b654c74d080@jeffhostetler.com","threadId":"46590","inReplyTo":"CAN0heSr-OcLJU54acTdXWx8NAo=nPD=9+DfexWZ0F7NRgRB9Dg@mail.gmail.com","subject":"Re: tsan: t3008: hashmap_add touches size from multiple threads","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-08-15T20:46:21Z","receivedAt":"2017-08-15T20:46:29Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/15/2017 3:21 PM, Martin Ågren wrote:\n> On 15 August 2017 at 20:48, Stefan Beller wrote:\n>>>>> /* total number of entries (0 means the hashmap is empty) */\n>>>>> - unsigned int size;\n>>>>> + /* -1 means size is unknown for threading reasons */\n>>>>> + int size;\n>>>>\n>>>> This double-encodes the state of disallow_rehash (i.e. if we had\n>>>> signed size, then the invariant disallow_rehash === (size < 0)\n>>>> is true, such that we could omit either the flag and just check for\n>>>> size < 0 or we do not need the negative size as any user would\n>>>> need to check disallow_rehash first. Not sure which API is harder\n>>>> to misuse. I'd think just having the size and getting rid of\n>>>> disallow_rehash might be hard to to reused.\n>>>\n>>> (Do you mean \"might be hard to be misused\"?)\n>>\n>> yes, I do.\n>>\n>>> One good thing about turning off the size-tracking with threading is\n>>> that someone who later wants to know the size in a threaded application\n>>> will not introduce any subtle bugs by misusing size, but will be forced\n>>> to provide and use some sort of InterlockedIncrement().\n>>\n>> agreed.\n>>\n>>> When/if that\n>>> change happens, it would be nice if no-one relied on the value of size\n>>> to say anything about threading. So it might make sense to have an\n>>> implementation-independent way of accessing disallow_rehash a.k.a.\n>>> (size < 0).\n>>\n>> Yes, and my point was whether we want to keep disallow_rehash around,\n>> as when a patch as this is applied, we'd have it encoded twice,\n>> both size < 0 as well as disallow_rehash set indicate the rehashing\n>> disabled.\n>>\n>> If we were to reduce it to one, we would not have \"invalid\" state possible\n>> such as size < 0 and disallow_rehash = 0.\n> \n> Agreed.\n> \n>> In the future we may have more options that make size impossible to\n>> compute efficiently, such that in that case we'd want to know which\n>> condition lead to it. In that case we'd want to have the flags around.\n> \n> Good point.\n\nI feel like we're trying to push hashmaps a little beyond\ntheir capability. I mean the core hashmap code is NOT thread\nsafe. The caller is responsible for carefully controlling how\nthe hashmap is used and whatever locking strategy it wants --\nwhether it is a single lock on the entire hashmap -- or a set\nof partition-specific locks like I created here. Whatever the\nstrategy, it is outside of hashmap.[ch].\n\nPerhaps it would be best to just define things as:\n* let (size < 0) mean we choose not to compute/track it (without\n saying why).\n* keep \"disallow_rehash = 1\" to mean we do not want automatic\n resizing (without saying why).\n\nThread-aware callers will set both.\n\nThread-aware callers (when finished with threaded operations)\ncan themselves choose whether to compute the correct size and\nre-allow rehashing. And we can add a method to hashmap.c to\nre-calculate the size if we want.\n\n\nIn my lazy_init_name_hash() I set \"disallow\", do the threaded\ncode, and then unset \"disallow\" -- mainly to keep the usage\nconsistent with the non-threaded case. I could just as easily\nset \"disallow\" and leave it that way -- the question is whether\nwe care if the hashmap automatically resizes later. (I don't.)\n\n\n>>> For example a function hashmap_disallow_rehash(), except that's\n>>> obviously taken. :-) Maybe the existing function would then be\n>>> hashmap_set_disallow_rehash(). Oh well..\n>>\n>> Not sure I understand this one.\n> \n> Sorry. What I meant was, if we drop the disallow_rehash-field, someone\n> might be tempted to use size < 0 (or size == -1) to answer the question\n> \"is rehashing disallowed?\". (Or \"am I threaded?\" which already is a\n> question which the hashmap as it is today doesn't know about.)\n> \n> So instead of looking at \"disallow_rehash\" one should perhaps be calling\n> \"hashmap_is_disallow_rehash()\" or \"hashmap_get_disallow_rehash()\", which\n> would be implemented as \"return disallow_rehash\", or possibly \"return\n> size == -1\".\n> \n> Except such names are, to the best of my understanding, not the Git-way,\n> so it should be, e.g., \"hashmap_disallow_rehash()\".\n> \n> Except ... that name is taken.... So to free that name up, the existing\n> function should perhaps be renamed \"hashmap_set_disallow_rehash()\",\n> again assuming I've picked up the right conventions in my recent\n> browsing of the Git-code.\n> \n> The final \"Oh well\" was a short form of \"it began with an observation\n> which currently has no practical effect, and is slowly turning into a\n> chain of ideas on how to rebuild the interface\".\n> \n"},{"id":"326600","messageId":"20170817105702.ma7la3xfhs7dkmy4@sigill.intra.peff.net","threadId":"46590","inReplyTo":"939b37f809dd9e1526593c02154fae14b369c73a.1502780344.git.martin.agren@gmail.com","subject":"Re: tsan: t5400: set_try_to_free_routine","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-17T10:57:02Z","receivedAt":"2017-08-17T10:57:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 15, 2017 at 02:53:07PM +0200, Martin Ågren wrote:\n\n> Using SANITIZE=thread made t5400-send-pack.sh hit the potential race\n> below.\n> \n> This is set_try_to_free_routine in wrapper.c. The race relates to the\n> reading of the \"old\" value. The caller doesn't care about the \"old\"\n> value, so this should be harmless right now. But it seems that using\n> this mechanism from multiple threads and restoring the earlier value\n> will probably not work out every time. (Not necessarily because of the\n> race in set_try_to_free_routine, but, e.g., because callers might not\n> restore the function pointer in the reverse order of how they\n> originally set it.)\n> \n> Properly \"fixing\" this for thread-safety would probably require some\n> redesigning, which at the time might not be warranted. I'm just posting\n> this for completeness.\n> \n> Martin\n> \n> WARNING: ThreadSanitizer: data race (pid=21382)\n> Read of size 8 at 0x000000979970 by thread T1:\n> #0 set_try_to_free_routine wrapper.c:35 (git+0x0000006cde1c)\n> #1 prepare_trace_line trace.c:105 (git+0x0000006a3bf0)\n> #2 trace_strbuf_fl trace.c:185 (git+0x0000006a3bf0)\n> #3 packet_trace pkt-line.c:80 (git+0x0000005f9f43)\n> #4 packet_read pkt-line.c:309 (git+0x0000005fbe10)\n> #5 recv_sideband sideband.c:37 (git+0x000000684c5e)\n> #6 sideband_demux send-pack.c:216 (git+0x00000065a38c)\n> #7 run_thread run-command.c:933 (git+0x000000655a93)\n> #8 (libtsan.so.0+0x0000000230d9)\n\nI was curious why the trace code would care about the free routine in\nthe first place. Digging in the mailing list, I didn't find a lot of\ndiscussion. But I think the problem is basically that the trace\ninfrastructure wants to be thread-safe, but the default free-pack-memory\ncallback isn't.\n\nIt's ironic that we fix the thread-unsafety of the free-pack-memory\nfunction by using the also-thread-unsafe set_try_to_free_routine.\n\nFurther irony: the trace routines aren't thread-safe in the first place,\nas they do lazy initialization of key->fd using an \"initialized\" field.\nIn practice it probably means double-writing key->fd and leaking a\ndescriptor (though there are no synchronizing operations there, so it's\nentirely possible a compiler could reorder the assignments to key->fd\nand key->initialized and a simultaneous reader could read a garbage\nkey->fd value). We also call getenv(), which isn't thread-safe with\nother calls to getenv() or setenv().\n\nI can think of a few possible directions:\n\n 1. Make set_try_to_free_routine() skip the write if it would be a\n noop. This is racy if threads are actually changing the value, but\n in practice they aren't (the first trace of any kind will set it to\n NULL, and it will remain there).\n\n 2. Make the free-packed routine thread-safe by taking a lock. It\n should hardly ever be called, so performance wouldn't matter. The\n big question is: _which_ lock. pack-objects, which uses threads\n already, has a version which does this. But it knows to take the\n big program-wide \"I'm accessing unsafe parts of Git\" lock that the\n rest of the program uses during its multi-threaded parts.\n There's no notion in the rest of Git for \"now we're going into a\n multi-threaded part, so most calls will need to take a big global\n lock before doing anything interesting\".\n\n For parts of Git that are explicitly multi-threaded (like the\n pack-objects delta search, or index-pack's delta resolution) that's\n not so bad. But the example above is just using a sideband demuxer.\n It would be unfortunate if the entire rest of send-pack had to\n start caring about taking that lock.\n\n 3. Is the free-pack-memory thing actually accomplishing much these\n days? It comes from 97bfeb34df (Release pack windows before\n reporting out of memory., 2006-12-24), and the primary issue is not\n actual allocated memory, but mmap'd packs clogging up the address\n space so that malloc can't find a suitable block.\n\n On 64-bit systems this is likely doing nothing. We have tons of\n address space. But even on 32-bit systems, the default\n core.packedGitLimit is only 256MiB (which was set around the same\n time). You can certainly come up with a corner case where freeing\n up that address space could matter. But I'd be surprised if this\n has actually helped much in practice over the years. And if you\n have a repo which is running so close to the address space limits\n of your system, the right answer is probably: upgrade to a 64-bit\n system. Even if the try-to-free thing helped in one run, it's\n likely that similar runs are not going to be so lucky, and even\n with it you're going to see sporadic out-of-memory failures.\n\n-Peff\n"},{"id":"326811","messageId":"20170820100633.ehn2sc7gwmm6lftd@sigill.intra.peff.net","threadId":"46590","inReplyTo":"cover.1502780343.git.martin.agren@gmail.com","subject":"Re: [PATCH/RFC 0/5] Some ThreadSanitizer-results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-20T10:06:34Z","receivedAt":"2017-08-20T10:06:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 15, 2017 at 02:53:00PM +0200, Martin Ågren wrote:\n\n> I tried running the test suite on Git compiled with ThreadSanitizer\n> (SANITIZE=thread). Maybe this series could be useful for someone else\n> trying to do the same. I needed the first patch to avoid warnings when\n> compiling, although it actually has nothing to do with threads.\n> \n> The last four patches are about avoiding some issues where\n> ThreadSanitizer complains for reasonable reasons, but which to the best\n> of my understanding are not real problems. These patches could be useful\n> to make \"actual\" problems stand out more. Of course, if no-one ever runs\n> ThreadSanitizer, they are of little to no (or even negative) value...\n\nI think it's a chicken-and-egg. I'd love to run the test suite with tsan\nfrom time to time, but there's no point if it turns up a bunch of false\npositives.\n\nThe general direction here looks good to me (and I agree with the\ncomments made so far, especially that we should stop writing to\nstrbuf_slopbuf entirely).\n\n> ThreadSanitizer: add suppressions\n\nThis one is the most interesting because it really is just papering over\nthe issues. I \"solved\" the transfer_debug one with actual code in:\n\n https://public-inbox.org/git/20170710133040.yom65mjol3nmf2it@sigill.intra.peff.net/\n\nbut it just feels really dirty. I'd be inclined to go with suppressions\nfor now until somebody can demonstrate or argue for an actual breakage\n(just because it makes the tool more useful for finding _real_\nproblems).\n\n-Peff\n"},{"id":"326815","messageId":"CAN0heSpdi7wNqj50ZeAStvf=pY-HRy=6Zx5t_Wo7tTnGjws-ag@mail.gmail.com","threadId":"46590","inReplyTo":"20170820100633.ehn2sc7gwmm6lftd@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 0/5] Some ThreadSanitizer-results","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-20T10:45:27Z","receivedAt":"2017-08-20T10:45:33Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 20 August 2017 at 12:06, Jeff King wrote:\n> On Tue, Aug 15, 2017 at 02:53:00PM +0200, Martin Ågren wrote:\n>\n>> I tried running the test suite on Git compiled with ThreadSanitizer\n>> (SANITIZE=thread). Maybe this series could be useful for someone else\n>> trying to do the same. I needed the first patch to avoid warnings when\n>> compiling, although it actually has nothing to do with threads.\n>>\n>> The last four patches are about avoiding some issues where\n>> ThreadSanitizer complains for reasonable reasons, but which to the best\n>> of my understanding are not real problems. These patches could be useful\n>> to make \"actual\" problems stand out more. Of course, if no-one ever runs\n>> ThreadSanitizer, they are of little to no (or even negative) value...\n>\n> I think it's a chicken-and-egg. I'd love to run the test suite with tsan\n> from time to time, but there's no point if it turns up a bunch of false\n> positives.\n>\n> The general direction here looks good to me (and I agree with the\n> comments made so far, especially that we should stop writing to\n> strbuf_slopbuf entirely).\n>\n>> ThreadSanitizer: add suppressions\n>\n> This one is the most interesting because it really is just papering over\n> the issues. I \"solved\" the transfer_debug one with actual code in:\n>\n> https://public-inbox.org/git/20170710133040.yom65mjol3nmf2it@sigill.intra.peff.net/\n\nHmm, I did search for related posts, but obviously not good enough.\nSorry that I missed this.\n\n> but it just feels really dirty. I'd be inclined to go with suppressions\n> for now until somebody can demonstrate or argue for an actual breakage\n> (just because it makes the tool more useful for finding _real_\n> problems).\n\nI actually had a more intrusive version of my patch, but which I didn't\nsend, where I extracted transport_debug_enabled() exactly like that,\nexcept I did it in order to minimize the scope of the suppression. In\nthe end, I figured the scope was already small enough.\n\nThe obvious risk of introducing any kind of \"temporary\" suppression is\nthat it remains forever... :-) I think I'll add a couple of NEEDSWORK in\nthe code to make it a bit more likely that someone stumbles over the\nproblem and addresses it.\n\nThanks.\n"},{"id":"326891","messageId":"cover.1503323390.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1502780343.git.martin.agren@gmail.com","subject":"[PATCH v2 0/4] Some ThreadSanitizer-results","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-21T17:43:44Z","receivedAt":"2017-08-21T17:44:06Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is the second version of my series to try to address some issues\nnoted by ThreadSanitizer. Thanks to all who took the time to provide\ninput on the first version.\n\nThe largest change is in the third patch, which moves the \"avoid writing\nto slopbuf\"-logic into strbuf_setlen, and compiles it unconditionally.\nPatch 2 hasn't been changed. The others have seen some minor changes.\nThe patch introducing GIT_THREAD_SANITIZER is gone.\n\nThe end result as far as ThreadSanitizer is concerned is the same:\n\n1) set_try_to_free_routine, where Peff outlined some possibilities, and\nwhere I don't feel like I have any idea which of them is better.\n\n2) hashmap_add, which I could try my hands on if Jeff doesn't beat me to\nit -- his proposed change should fix it and I doubt I could come up with\nanything \"better\", considering he knows the code.\n\nMartin\n\nMartin Ågren (4):\n convert: always initialize attr_action in convert_attrs\n pack-objects: take lock before accessing `remaining`\n strbuf_setlen: don't write to strbuf_slopbuf\n ThreadSanitizer: add suppressions\n\n strbuf.h | 5 ++++-\n builtin/pack-objects.c | 6 ++++++\n color.c | 7 +++++++\n convert.c | 5 +++--\n transport-helper.c | 7 +++++++\n .tsan-suppressions | 10 ++++++++++\n 6 files changed, 37 insertions(+), 3 deletions(-)\n create mode 100644 .tsan-suppressions\n\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326892","messageId":"f23f7f3d6cb75743cfd5b120a87e8765b281ce0a.1503323391.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1503323390.git.martin.agren@gmail.com","subject":"[PATCH v2 1/4] convert: always initialize attr_action in convert_attrs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-21T17:43:45Z","receivedAt":"2017-08-21T17:44:10Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"convert_attrs contains an \"if-else\". In the \"if\", we set attr_action\ntwice, and the first assignment has no effect. In the \"else\", we do not\nset it at all. Since git_check_attr always returns the same value, we'll\nalways end up in the \"if\", so there is no problem right now. But\nconvert_attrs is obviously trying not to rely on such an\nimplementation-detail of another component.\n\nMake the initialization of attr_action after the if-else. Remove the\nearlier assignments.\n\nSuggested-by: Torsten Bögershausen \nSigned-off-by: Martin Ågren \n---\nv2: adapted per input from Torsten\n\n convert.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 1012462e3..1f8116381 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -1040,7 +1040,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t\tca->crlf_action = git_path_check_crlf(ccheck + 4);\n \t\tif (ca->crlf_action == CRLF_UNDEFINED)\n \t\t\tca->crlf_action = git_path_check_crlf(ccheck + 0);\n-\t\tca->attr_action = ca->crlf_action;\n \t\tca->ident = git_path_check_ident(ccheck + 1);\n \t\tca->drv = git_path_check_convert(ccheck + 2);\n \t\tif (ca->crlf_action != CRLF_BINARY) {\n@@ -1054,12 +1053,14 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t\t\telse if (eol_attr == EOL_CRLF)\n \t\t\t\tca->crlf_action = CRLF_TEXT_CRLF;\n \t\t}\n-\t\tca->attr_action = ca->crlf_action;\n \t} else {\n \t\tca->drv = NULL;\n \t\tca->crlf_action = CRLF_UNDEFINED;\n \t\tca->ident = 0;\n \t}\n+\n+\t/* Save attr and make a decision for action */\n+\tca->attr_action = ca->crlf_action;\n \tif (ca->crlf_action == CRLF_TEXT)\n \t\tca->crlf_action = text_eol_is_crlf() ? CRLF_TEXT_CRLF : CRLF_TEXT_INPUT;\n \tif (ca->crlf_action == CRLF_UNDEFINED && auto_crlf == AUTO_CRLF_FALSE)\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326893","messageId":"6b50c193fdf441349809c01b2eb63b484e8d89ab.1503323391.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1503323390.git.martin.agren@gmail.com","subject":"[PATCH v2 2/4] pack-objects: take lock before accessing `remaining`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-21T17:43:46Z","receivedAt":"2017-08-21T17:44:11Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When checking the conditional of \"while (me->remaining)\", we did not\nhold the lock. Calling find_deltas would still be safe, since it checks\n\"remaining\" (after taking the lock) and is able to handle all values. In\nfact, this could (currently) not trigger any bug: a bug could happen if\n`remaining` transitioning from zero to non-zero races with the evaluation\nof the while-condition, but these are always separated by the\ndata_ready-mechanism.\n\nMake sure we have the lock when we read `remaining`. This does mean we\nrelease it just so that find_deltas can take it immediately again. We\ncould tweak the contract so that the lock should be taken before calling\nfind_deltas, but let's defer that until someone can actually show that\n\"unlock+lock\" has a measurable negative impact.\n\nSigned-off-by: Martin Ågren \n---\nv2: unchanged from v1\n\n builtin/pack-objects.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c753e9237..bd391e97a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2170,7 +2170,10 @@ static void *threaded_find_deltas(void *arg)\n {\n \tstruct thread_params *me = arg;\n \n+\tprogress_lock();\n \twhile (me->remaining) {\n+\t\tprogress_unlock();\n+\n \t\tfind_deltas(me->list, &me->remaining,\n \t\t\t me->window, me->depth, me->processed);\n \n@@ -2192,7 +2195,10 @@ static void *threaded_find_deltas(void *arg)\n \t\t\tpthread_cond_wait(&me->cond, &me->mutex);\n \t\tme->data_ready = 0;\n \t\tpthread_mutex_unlock(&me->mutex);\n+\n+\t\tprogress_lock();\n \t}\n+\tprogress_unlock();\n \t/* leave ->working 1 so that this doesn't get more work assigned */\n \treturn NULL;\n }\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326894","messageId":"dccd3e75fcd1b2de93263e8373a3b4cd5da0dd32.1503323391.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1503323390.git.martin.agren@gmail.com","subject":"[PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-21T17:43:47Z","receivedAt":"2017-08-21T17:44:13Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"strbuf_setlen(., 0) writes '\\0' to sb.buf[0], where buf is either\nallocated and unique to sb, or the global slopbuf. The slopbuf is meant\nto provide a guarantee that buf is not NULL and that a freshly\ninitialized buffer contains the empty string, but it is not supposed to\nbe written to. That strbuf_setlen writes to slopbuf has at least two\nimplications:\n\nFirst, it's wrong in principle. Second, it might be hiding misuses which\nare just waiting to wreak havoc. Third, ThreadSanitizer detects a race\nwhen multiple threads write to slopbuf at roughly the same time, thus\npotentially making any more critical races harder to spot.\n\nAvoid writing to strbuf_slopbuf in strbuf_setlen. Let's instead assert\non the first byte of slopbuf being '\\0', since it helps ensure the\npromised invariant of buf[len] == '\\0'. (We know that \"len\" was already\n0, or someone has messed with \"alloc\". If someone has fiddled with the\nfields that much beyond the correct interface, they're on their own.)\n\nThis is a function which is used in many places, possibly also in hot\ncode paths. There are two branches in strbuf_setlen already, and we are\nadding a third and possibly a fourth (in the assert). In hot code paths,\nwe hopefully reuse the buffer in order to avoid continous reallocations.\nThus, after a start-up phase, we should always take the same path,\nwhich might help branch prediction, and we would never make the assert.\nIf a hot code path continuously reallocates, we probably have bigger\nperformance problems than this new safety-check.\n\nSimple measurements do not contradict this reasoning. 100000000 times\nresetting a buffer and adding the empty string takes 5.29/5.26 seconds\nwith/without this patch (best of three). Releasing at every iteration\nyields 18.01/17.87. Adding a 30-character string instead of the empty\nstring yields 5.61/5.58 and 17.28/17.28(!).\n\nThis patch causes the git binary emitted by gcc 5.4.0 -O2 on my machine\nto grow from 11389848 bytes to 11497184 bytes, an increase of 0.9%.\n\nI also tried to piggy-back on the fact that we already check alloc,\nwhich should already tell us whether we are using the slopbuf:\n\n if (sb->alloc) {\n if (len > sb->alloc - 1)\n die(\"BUG: strbuf_setlen() beyond buffer\");\n sb->buf[len] = '\\0';\n } else {\n if (len)\n die(\"BUG: strbuf_setlen() beyond buffer\");\n assert(!strbuf_slopbuf[0]);\n }\n sb->len = len;\n\nThat didn't seem to be much slower (5.38, 18.02, 5.70, 17.32 seconds),\nbut it does introduce some minor code duplication. The resulting git\nbinary was 11510528 bytes large (another 0.1% increase).\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Martin Ågren \n---\nv2: no \"ifdef TSAN\"; moved check from strbuf_reset into strbuf_setlen\n\n strbuf.h | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex e705b94db..7496cb8ec 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n \tif (len > (sb->alloc ? sb->alloc - 1 : 0))\n \t\tdie(\"BUG: strbuf_setlen() beyond buffer\");\n \tsb->len = len;\n-\tsb->buf[len] = '\\0';\n+\tif (sb->buf != strbuf_slopbuf)\n+\t\tsb->buf[len] = '\\0';\n+\telse\n+\t\tassert(!strbuf_slopbuf[0]);\n }\n \n /**\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"326895","messageId":"09bbbcd1429a28774ea2d8c67ef6106ab558c296.1503323391.git.martin.agren@gmail.com","threadId":"46590","inReplyTo":"cover.1503323390.git.martin.agren@gmail.com","subject":"[PATCH v2 4/4] ThreadSanitizer: add suppressions","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-21T17:43:48Z","receivedAt":"2017-08-21T17:44:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Add a file .tsan-suppressions and list two functions in it: want_color()\nand transfer_debug(). Both of these use the pattern\n\n\tstatic int foo = -1;\n\tif (foo < 0)\n\t\tfoo = bar();\n\nwhere bar always returns the same non-negative value. This can cause\nThreadSanitizer to diagnose a race when foo is written from two threads.\nThat is indeed a race, although it arguably doesn't matter in practice\nsince it's always the same value that is written.\n\nAdd NEEDSWORK-comments to the functions so that this problem is not\nforever swept way under the carpet.\n\nThe suppressions-file is used by setting the environment variable\nTSAN_OPTIONS to, e.g., \"suppressions=$(pwd)/.tsan-suppressions\". Observe\nthat relative paths such as \".tsan-suppressions\" might not work.\n\nSigned-off-by: Martin Ågren \n---\nv2: added NEEDSWORK; reworded the comments in the new file\n color.c | 7 +++++++\n transport-helper.c | 7 +++++++\n .tsan-suppressions | 10 ++++++++++\n 3 files changed, 24 insertions(+)\n create mode 100644 .tsan-suppressions\n\ndiff --git a/color.c b/color.c\nindex 7aa8b076f..9ccd954d6 100644\n--- a/color.c\n+++ b/color.c\n@@ -338,6 +338,13 @@ static int check_auto_color(void)\n \n int want_color(int var)\n {\n+\t/*\n+\t * NEEDSWORK: This function is sometimes used from multiple threads, and\n+\t * we end up using want_auto racily. That \"should not matter\" since\n+\t * we always write the same value, but it's still wrong. This function\n+\t * is listed in .tsan-suppressions for the time being.\n+\t */\n+\n \tstatic int want_auto = -1;\n \n \tif (var < 0)\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 8f68d69a8..f50b34df2 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1117,6 +1117,13 @@ int transport_helper_init(struct transport *transport, const char *name)\n __attribute__((format (printf, 1, 2)))\n static void transfer_debug(const char *fmt, ...)\n {\n+\t/*\n+\t * NEEDSWORK: This function is sometimes used from multiple threads, and\n+\t * we end up using debug_enabled racily. That \"should not matter\" since\n+\t * we always write the same value, but it's still wrong. This function\n+\t * is listed in .tsan-suppressions for the time being.\n+\t */\n+\n \tva_list args;\n \tchar msgbuf[PBUFFERSIZE];\n \tstatic int debug_enabled = -1;\ndiff --git a/.tsan-suppressions b/.tsan-suppressions\nnew file mode 100644\nindex 000000000..8c85014a0\n--- /dev/null\n+++ b/.tsan-suppressions\n@@ -0,0 +1,10 @@\n+# Suppressions for ThreadSanitizer (tsan).\n+#\n+# This file is used by setting the environment variable TSAN_OPTIONS to, e.g.,\n+# \"suppressions=$(pwd)/.tsan-suppressions\". Observe that relative paths such as\n+# \".tsan-suppressions\" might not work.\n+\n+# A static variable is written to racily, but we always write the same value, so\n+# in practice it (hopefully!) doesn't matter.\n+race:^want_color$\n+race:^transfer_debug$\n-- \n2.14.1.151.gdfeca7a7e\n\n"},{"id":"327051","messageId":"xmqq378i19ku.fsf@gitster.mtv.corp.google.com","threadId":"46590","inReplyTo":"dccd3e75fcd1b2de93263e8373a3b4cd5da0dd32.1503323391.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T17:24:17Z","receivedAt":"2017-08-23T17:24:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> strbuf_setlen(., 0) writes '\\0' to sb.buf[0], where buf is either\n> allocated and unique to sb, or the global slopbuf. The slopbuf is meant\n> to provide a guarantee that buf is not NULL and that a freshly\n> initialized buffer contains the empty string, but it is not supposed to\n> be written to. That strbuf_setlen writes to slopbuf has at least two\n> implications:\n>\n> First, it's wrong in principle. Second, it might be hiding misuses which\n> are just waiting to wreak havoc. Third, ThreadSanitizer detects a race\n> when multiple threads write to slopbuf at roughly the same time, thus\n> potentially making any more critical races harder to spot.\n\nThere are two hard things in computer science ;-).\n\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Martin Ågren \n> ---\n> v2: no \"ifdef TSAN\"; moved check from strbuf_reset into strbuf_setlen\n\nLooks much better. I have a mild objection to \"suggested-by\",\nthough. It makes it sound as if this were my itch, but it is not.\n\nAll the credit for being motivate to fix the issue should go to you.\nFor what I did during the review of the previous one to lead to this\nsimpler version, if you want to document it, \"helped-by\" would be\nmore appropriate.\n\n> strbuf.h | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/strbuf.h b/strbuf.h\n> index e705b94db..7496cb8ec 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n> \tif (len > (sb->alloc ? sb->alloc - 1 : 0))\n> \t\tdie(\"BUG: strbuf_setlen() beyond buffer\");\n> \tsb->len = len;\n> -\tsb->buf[len] = '\\0';\n> +\tif (sb->buf != strbuf_slopbuf)\n> +\t\tsb->buf[len] = '\\0';\n> +\telse\n> +\t\tassert(!strbuf_slopbuf[0]);\n> }\n> \n> /**\n"},{"id":"327054","messageId":"CAN0heSoqnEx=vPVZ5-OfqMkzL_JKKoa+iyP=G5h-cnqOwjPPYg@mail.gmail.com","threadId":"46590","inReplyTo":"xmqq378i19ku.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-23T17:43:01Z","receivedAt":"2017-08-23T17:43:07Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 23 August 2017 at 19:24, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> strbuf_setlen(., 0) writes '\\0' to sb.buf[0], where buf is either\n>> allocated and unique to sb, or the global slopbuf. The slopbuf is meant\n>> to provide a guarantee that buf is not NULL and that a freshly\n>> initialized buffer contains the empty string, but it is not supposed to\n>> be written to. That strbuf_setlen writes to slopbuf has at least two\n>> implications:\n>>\n>> First, it's wrong in principle. Second, it might be hiding misuses which\n>> are just waiting to wreak havoc. Third, ThreadSanitizer detects a race\n>> when multiple threads write to slopbuf at roughly the same time, thus\n>> potentially making any more critical races harder to spot.\n>\n> There are two hard things in computer science ;-).\n\nIndeed. :-)\n\n>> Suggested-by: Junio C Hamano \n>> Signed-off-by: Martin Ågren \n>> ---\n>> v2: no \"ifdef TSAN\"; moved check from strbuf_reset into strbuf_setlen\n>\n> Looks much better. I have a mild objection to \"suggested-by\",\n> though. It makes it sound as if this were my itch, but it is not.\n>\n> All the credit for being motivate to fix the issue should go to you.\n> For what I did during the review of the previous one to lead to this\n> simpler version, if you want to document it, \"helped-by\" would be\n> more appropriate.\n\nOk, so that's two things to tweak in the commit message. I'll hold off\non v3 in case I get some more feedback the coming days. Thanks.\n\nMartin\n"},{"id":"327065","messageId":"xmqqk21uyw5m.fsf@gitster.mtv.corp.google.com","threadId":"46590","inReplyTo":"CAN0heSoqnEx=vPVZ5-OfqMkzL_JKKoa+iyP=G5h-cnqOwjPPYg@mail.gmail.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T18:30:13Z","receivedAt":"2017-08-23T18:30:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> On 23 August 2017 at 19:24, Junio C Hamano wrote:\n>> Martin Ågren writes:\n>>\n>>> strbuf_setlen(., 0) writes '\\0' to sb.buf[0], where buf is either\n>>> allocated and unique to sb, or the global slopbuf. The slopbuf is meant\n>>> to provide a guarantee that buf is not NULL and that a freshly\n>>> initialized buffer contains the empty string, but it is not supposed to\n>>> be written to. That strbuf_setlen writes to slopbuf has at least two\n>>> implications:\n>>>\n>>> First, it's wrong in principle. Second, it might be hiding misuses which\n>>> are just waiting to wreak havoc. Third, ThreadSanitizer detects a race\n>>> when multiple threads write to slopbuf at roughly the same time, thus\n>>> potentially making any more critical races harder to spot.\n>>\n>> There are two hard things in computer science ;-).\n>\n> Indeed. :-)\n>\n>>> Suggested-by: Junio C Hamano \n>>> Signed-off-by: Martin Ågren \n>>> ---\n>>> v2: no \"ifdef TSAN\"; moved check from strbuf_reset into strbuf_setlen\n>>\n>> Looks much better. I have a mild objection to \"suggested-by\",\n>> though. It makes it sound as if this were my itch, but it is not.\n>>\n>> All the credit for being motivate to fix the issue should go to you.\n>> For what I did during the review of the previous one to lead to this\n>> simpler version, if you want to document it, \"helped-by\" would be\n>> more appropriate.\n>\n> Ok, so that's two things to tweak in the commit message. I'll hold off\n> on v3 in case I get some more feedback the coming days. Thanks.\n\nWell, this one is good enough and your \"at least two\" is technically\nfine ;-) Let's not reroll this any further.\n"},{"id":"327083","messageId":"CA+sFfMdXv+nqpXmwfLTHtkRLuGkAEAwWXZCvOryVZ=aLb_UmbA@mail.gmail.com","threadId":"46590","inReplyTo":"dccd3e75fcd1b2de93263e8373a3b4cd5da0dd32.1503323391.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-08-23T20:37:12Z","receivedAt":"2017-08-23T20:37:20Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Mon, Aug 21, 2017 at 10:43 AM, Martin Ågren wrote:\n> strbuf_setlen(., 0) writes '\\0' to sb.buf[0], where buf is either\n> allocated and unique to sb, or the global slopbuf. The slopbuf is meant\n> to provide a guarantee that buf is not NULL and that a freshly\n> initialized buffer contains the empty string, but it is not supposed to\n> be written to. That strbuf_setlen writes to slopbuf has at least two\n> implications:\n\n\n> diff --git a/strbuf.h b/strbuf.h\n> index e705b94db..7496cb8ec 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n> if (len > (sb->alloc ? sb->alloc - 1 : 0))\n> die(\"BUG: strbuf_setlen() beyond buffer\");\n> sb->len = len;\n> - sb->buf[len] = '\\0';\n> + if (sb->buf != strbuf_slopbuf)\n> + sb->buf[len] = '\\0';\n> + else\n> + assert(!strbuf_slopbuf[0]);\n> }\n>\n> /**\n> --\n> 2.14.1.151.gdfeca7a7e\n>\n\nI know this must have been discussed before and/or I'm having a neuron\nmisfire, but is there any reason why we didn't just make slopbuf a\nmacro for \"\"?\n\ni.e.\n\n #define strbuf_slopbuf \"\"\n\nThat way it should point to readonly memory and any attempt to assign\nto it should produce a crash.\n\nOne benefit that I can think of for making strbuf_slopbuf be a real\nvariable is that we can then compare the pointer stored in the strbuf\nto the strbuf_slopbuf address to detect whether the strbuf held the\nslopbuf. With the static string macro, each execution unit may get\nit's own instance of the empty string. But, before this patch, we\ndon't actually seem to be doing that anywhere and instead rely on the\nvalue of alloc being accurate to determine whether the strbuf contains\nthe slopbuf or not.\n\nSo is there any reason why didn't do something like the following in\nthe first place?\n\ndiff --git a/strbuf.h b/strbuf.h\nindex e705b94..fcca618 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -67,7 +67,7 @@ struct strbuf {\n char *buf;\n };\n\n-extern char strbuf_slopbuf[];\n+#define strbuf_slopbuf \"\"\n #define STRBUF_INIT { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }\n\n /**\n@@ -147,7 +147,9 @@ static inline void strbuf_setlen(struct strbuf\n*sb, size_t len)\n if (len > (sb->alloc ? sb->alloc - 1 : 0))\n die(\"BUG: strbuf_setlen() beyond buffer\");\n sb->len = len;\n- sb->buf[len] = '\\0';\n+ if (sb->alloc) {\n+ sb->buf[len] = '\\0';\n+ }\n }\n\n-Brandon\n"},{"id":"327085","messageId":"xmqqh8wyxag1.fsf@gitster.mtv.corp.google.com","threadId":"46590","inReplyTo":"CA+sFfMdXv+nqpXmwfLTHtkRLuGkAEAwWXZCvOryVZ=aLb_UmbA@mail.gmail.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T21:04:30Z","receivedAt":"2017-08-23T21:04:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> So is there any reason why didn't do something like the following in\n> the first place?\n\nMy guess is that we didn't bother; if we cared, we would have used a\nsingle instance of const char in a read-only segment, instead of\nsuch a macro.\n\n> diff --git a/strbuf.h b/strbuf.h\n> index e705b94..fcca618 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -67,7 +67,7 @@ struct strbuf {\n> char *buf;\n> };\n>\n> -extern char strbuf_slopbuf[];\n> +#define strbuf_slopbuf \"\"\n> #define STRBUF_INIT { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }\n>\n> /**\n> @@ -147,7 +147,9 @@ static inline void strbuf_setlen(struct strbuf\n> *sb, size_t len)\n> if (len > (sb->alloc ? sb->alloc - 1 : 0))\n> die(\"BUG: strbuf_setlen() beyond buffer\");\n> sb->len = len;\n> - sb->buf[len] = '\\0';\n> + if (sb->alloc) {\n> + sb->buf[len] = '\\0';\n> + }\n> }\n>\n> -Brandon\n"},{"id":"327089","messageId":"CA+sFfMe56itAMDXOJybf0yHj+BqU1Ai1aU7inoTG3FJtdtZxyw@mail.gmail.com","threadId":"46590","inReplyTo":"xmqqh8wyxag1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-08-23T21:20:23Z","receivedAt":"2017-08-23T21:20:33Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote:\n> Brandon Casey writes:\n>\n>> So is there any reason why didn't do something like the following in\n>> the first place?\n>\n> My guess is that we didn't bother; if we cared, we would have used a\n> single instance of const char in a read-only segment, instead of\n> such a macro.\n\nI think you mean something like this:\n\n const char * const strbuf_slopbuf = \"\";\n\n..with or without \"const\" at the beginning. We can't use an actual\nvariable like that since we also want to be able to do initialization\nlike:\n\n struct strbuf b = STRBUF_INIT;\n\ni.e.\n\n struct strbuf b = { 0, 0, strbuf_slopbuf };\n\nSo the compiler needs to be able to determine that everything within\nthe curly braces is constant and apparently gcc cannot.\n\n\nOn a related note... I was just looking at object.c which also uses a\nslopbuf. We could similarly protect it from inadvertent modification\nby doing something like this:\n\ndiff --git a/object.c b/object.c\nindex 321d7e9..4c7a041 100644\n--- a/object.c\n+++ b/object.c\n@@ -303,7 +303,7 @@ int object_list_contains(struct object_list *list, struct ob\nject *obj)\n * A zero-length string to which object_array_entry::name can be\n * initialized without requiring a malloc/free.\n */\n-static char object_array_slopbuf[1];\n+static const char * const object_array_slopbuf = \"\";\n\n void add_object_array_with_path(struct object *obj, const char *name,\n struct object_array *array,\n@@ -326,7 +326,7 @@ void add_object_array_with_path(struct object\n*obj, const char *name,\n entry->name = NULL;\n else if (!*name)\n /* Use our own empty string instead of allocating one: */\n- entry->name = object_array_slopbuf;\n+ entry->name = (char*) object_array_slopbuf;\n else\n entry->name = xstrdup(name);\n entry->mode = mode;\n"},{"id":"327094","messageId":"CA+sFfMdMgrGBhECegBe09c38nRM+Zt5JK4gJaZ96DO-9zC-8qA@mail.gmail.com","threadId":"46590","inReplyTo":"CA+sFfMe56itAMDXOJybf0yHj+BqU1Ai1aU7inoTG3FJtdtZxyw@mail.gmail.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-08-23T21:54:56Z","receivedAt":"2017-08-23T21:55:06Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Aug 23, 2017 at 2:20 PM, Brandon Casey wrote:\n> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote:\n>> Brandon Casey writes:\n>>\n>>> So is there any reason why didn't do something like the following in\n>>> the first place?\n>>\n>> My guess is that we didn't bother; if we cared, we would have used a\n>> single instance of const char in a read-only segment, instead of\n>> such a macro.\n>\n> I think you mean something like this:\n>\n> const char * const strbuf_slopbuf = \"\";\n\nAh, you probably meant something like this:\n\n const char strbuf_slopbuf = '\\0';\n\nwhich gcc will apparently place in the read-only segment. I did not know that.\n\nAnd assignment and initialization would look like:\n\n sb->buf = (char*) &strbuf_slopbuf;\n\nand\n\n #define STRBUF_INIT { .alloc = 0, .len = 0, .buf = (char*) &strbuf_slopbuf }\n\nrespectively. Yeah, that's definitely preferable to a macro.\nSomething similar could be done in object.c.\n\n-Brandon\n"},{"id":"327097","messageId":"CA+sFfMf7nerBhm5jZ+sDJdGbTbLz_JXyqkSPGOA8o9mOE-7Khg@mail.gmail.com","threadId":"46590","inReplyTo":"CA+sFfMdMgrGBhECegBe09c38nRM+Zt5JK4gJaZ96DO-9zC-8qA@mail.gmail.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-08-23T22:11:26Z","receivedAt":"2017-08-23T22:11:35Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Aug 23, 2017 at 2:54 PM, Brandon Casey wrote:\n> On Wed, Aug 23, 2017 at 2:20 PM, Brandon Casey wrote:\n>> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote:\n>>> Brandon Casey writes:\n>>>\n>>>> So is there any reason why didn't do something like the following in\n>>>> the first place?\n>>>\n>>> My guess is that we didn't bother; if we cared, we would have used a\n>>> single instance of const char in a read-only segment, instead of\n>>> such a macro.\n>>\n>> I think you mean something like this:\n>>\n>> const char * const strbuf_slopbuf = \"\";\n\nHmm, apparently it is sufficient to mark our current strbuf_slopbuf\narray as const and initialize it with a static string to trigger its\nplacement into the read-only section by gcc (and clang).\n\n const char strbuf_slopbuf[1] = \"\";\n\n-Brandon\n"},{"id":"327098","messageId":"xmqq378hylbd.fsf@gitster.mtv.corp.google.com","threadId":"46590","inReplyTo":"CA+sFfMe56itAMDXOJybf0yHj+BqU1Ai1aU7inoTG3FJtdtZxyw@mail.gmail.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T22:24:22Z","receivedAt":"2017-08-23T22:24:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote:\n>> Brandon Casey writes:\n>>\n>>> So is there any reason why didn't do something like the following in\n>>> the first place?\n>>\n>> My guess is that we didn't bother; if we cared, we would have used a\n>> single instance of const char in a read-only segment, instead of\n>> such a macro.\n>\n> I think you mean something like this:\n>\n> const char * const strbuf_slopbuf = \"\";\n\nRather, more like \"const char slopbuf[1];\"\n"},{"id":"327100","messageId":"CA+sFfMdvOcXd_ePyqyiCXL9O6xpYxxn+iKw6NRJCYsiCFK_ADA@mail.gmail.com","threadId":"46590","inReplyTo":"xmqq378hylbd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-08-23T22:39:25Z","receivedAt":"2017-08-23T22:39:31Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Aug 23, 2017 at 3:24 PM, Junio C Hamano wrote:\n> Brandon Casey writes:\n>\n>> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote:\n>>> Brandon Casey writes:\n>>>\n>>>> So is there any reason why didn't do something like the following in\n>>>> the first place?\n>>>\n>>> My guess is that we didn't bother; if we cared, we would have used a\n>>> single instance of const char in a read-only segment, instead of\n>>> such a macro.\n>>\n>> I think you mean something like this:\n>>\n>> const char * const strbuf_slopbuf = \"\";\n>\n> Rather, more like \"const char slopbuf[1];\"\n\nYou'd think that would be sufficient right? Apparently gcc and clang\nwill only place a variable marked as const in the read-only section if\nyou initialize it with a constant too. (gcc 4.9.2, clang 3.8.1 on\nlinux, clang 8.1.0 on osx)\n\nI might have to adjust my stance on initializing global variables\nmoving forward :-)\n\n-Brandon\n"},{"id":"327139","messageId":"xmqqpobkx610.fsf@gitster.mtv.corp.google.com","threadId":"46590","inReplyTo":"CA+sFfMdMgrGBhECegBe09c38nRM+Zt5JK4gJaZ96DO-9zC-8qA@mail.gmail.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-24T16:52:11Z","receivedAt":"2017-08-24T16:52:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Ah, you probably meant something like this:\n>\n> const char strbuf_slopbuf = '\\0';\n>\n> which gcc will apparently place in the read-only segment. I did not know that.\n\nYes but I highly suspect that it would be very compiler dependent\nand not something the language lawyers would recommend us to rely\non.\n\nMy response was primarily to answer \"why?\" with \"because we did not\nbother\". The above is a mere tangent, i.e. \"multiple copies of\nempty strings is a horrible implementation (and there would be a way\nto do it with a single instance)\".\n\n> #define STRBUF_INIT { .alloc = 0, .len = 0, .buf = (char*) &strbuf_slopbuf }\n>\n> respectively. Yeah, that's definitely preferable to a macro.\n> Something similar could be done in object.c.\n\nWhat is the main objective for doing this change? The \"make sure we\ndo not write into that slopbuf\" assert() bothers you and you want to\nreplace it with an address in the read-only segment?\n"},{"id":"327140","messageId":"CA+sFfMdYXDt2mgnWq-HQQyBsCqYZ+689BCKEOw7siGjQoUysjg@mail.gmail.com","threadId":"46590","inReplyTo":"xmqqpobkx610.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-08-24T18:29:13Z","receivedAt":"2017-08-24T18:29:20Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Aug 24, 2017 at 9:52 AM, Junio C Hamano wrote:\n> Brandon Casey writes:\n>\n>> Ah, you probably meant something like this:\n>>\n>> const char strbuf_slopbuf = '\\0';\n>>\n>> which gcc will apparently place in the read-only segment. I did not know that.\n>\n> Yes but I highly suspect that it would be very compiler dependent\n> and not something the language lawyers would recommend us to rely\n> on.\n\nI think compilers may have the option of placing variables that are\nexplicitly initialized to zero in the bss segment too, in addition to\nthose that are not explicitly initialized. So I agree that no one\nshould write code that depends on their variables being placed in one\nsegment or the other, but I could see someone using this behavior as\nan additional safety check; kind of a free assert, aside from the\nadditional space in the .rodata segment.\n\n> My response was primarily to answer \"why?\" with \"because we did not\n> bother\". The above is a mere tangent, i.e. \"multiple copies of\n> empty strings is a horrible implementation (and there would be a way\n> to do it with a single instance)\".\n\nMerely adding const to our current strbuf_slopbuf declaration does not\nbuy us anything since it will be allocated in r/w memory. i.e. it\nwould still be possible to modify it via the buf member of strbuf. So\nyou can't just do this:\n\n const char strbuf_slopbuf[1];\n\nThat's pretty much equivalent to what we currently have since it only\nrestricts modifying the contents of strbuf_slopbuf directly through\nthe strbuf_slopbuf variable, but it does not restrict modifying it\nthrough a pointer to that object.\n\nUntil yesterday, I was under the impression that the only way to\naccess data in the rodata segment was through a constant literal. So\nmy initial thought was that we could do something like:\n\n const char * const strbuf_slopbuf = \"\";\n\n..but that variable cannot be used in a static assignment like:\n\n struct strbuf foo = {0, 0, (char*) strbuf_slopbuf};\n\nSo it seemed like our only option was to use a literal \"\" everywhere\ninstead of a slopbuf variable _if_ we wanted to have the guarantee\nthat our \"slopbuf\" could not be modified.\n\nBut what I learned yesterday, is that at least gcc/clang will place\nthe entire variable in the rodata segment if the variable is both\nmarked const _and_ initialized.\n\ni.e. this will be allocated in the .rodata segment:\n\n const char strbuf_slopbuf[1] = \"\";\n\n>> #define STRBUF_INIT { .alloc = 0, .len = 0, .buf = (char*) &strbuf_slopbuf }\n>>\n>> respectively. Yeah, that's definitely preferable to a macro.\n>> Something similar could be done in object.c.\n>\n> What is the main objective for doing this change? The \"make sure we\n> do not write into that slopbuf\" assert() bothers you and you want to\n> replace it with an address in the read-only segment?\n\nActually nothing about the patch bothers me. The point of that patch\nis to make sure we don't accidentally modify the slopbuf. I was just\nlooking for a way for the compiler to help out and wondering if there\nwas a reason we didn't attempt to do so in the first place.\n\nI think the main takeaway here is that I learned something yesterday\n:-) I didn't actually intend to submit a patch for any of this, but\nif anything useful came out of the discussion I thought Martin may\nincorporate it into his patch if he wanted to.\n\n-Brandon\n"},{"id":"327147","messageId":"CAN0heSqaRvS2N=iJDCTGe=LT+y5eUQJskOCOZ8MPJ6znWKJifA@mail.gmail.com","threadId":"46590","inReplyTo":"CA+sFfMdYXDt2mgnWq-HQQyBsCqYZ+689BCKEOw7siGjQoUysjg@mail.gmail.com","subject":"Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-24T19:16:00Z","receivedAt":"2017-08-24T19:16:10Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 24 August 2017 at 20:29, Brandon Casey wrote:\n> On Thu, Aug 24, 2017 at 9:52 AM, Junio C Hamano wrote:\n>> Brandon Casey writes:\n>>\n>>> Ah, you probably meant something like this:\n>>>\n>>> const char strbuf_slopbuf = '\\0';\n>>>\n>>> which gcc will apparently place in the read-only segment. I did not know that.\n>>\n>> Yes but I highly suspect that it would be very compiler dependent\n>> and not something the language lawyers would recommend us to rely\n>> on.\n>\n> I think compilers may have the option of placing variables that are\n> explicitly initialized to zero in the bss segment too, in addition to\n> those that are not explicitly initialized. So I agree that no one\n> should write code that depends on their variables being placed in one\n> segment or the other, but I could see someone using this behavior as\n> an additional safety check; kind of a free assert, aside from the\n> additional space in the .rodata segment.\n>\n>> My response was primarily to answer \"why?\" with \"because we did not\n>> bother\". The above is a mere tangent, i.e. \"multiple copies of\n>> empty strings is a horrible implementation (and there would be a way\n>> to do it with a single instance)\".\n>\n> Merely adding const to our current strbuf_slopbuf declaration does not\n> buy us anything since it will be allocated in r/w memory. i.e. it\n> would still be possible to modify it via the buf member of strbuf. So\n> you can't just do this:\n>\n> const char strbuf_slopbuf[1];\n>\n> That's pretty much equivalent to what we currently have since it only\n> restricts modifying the contents of strbuf_slopbuf directly through\n> the strbuf_slopbuf variable, but it does not restrict modifying it\n> through a pointer to that object.\n>\n> Until yesterday, I was under the impression that the only way to\n> access data in the rodata segment was through a constant literal. So\n> my initial thought was that we could do something like:\n>\n> const char * const strbuf_slopbuf = \"\";\n>\n> ..but that variable cannot be used in a static assignment like:\n>\n> struct strbuf foo = {0, 0, (char*) strbuf_slopbuf};\n>\n> So it seemed like our only option was to use a literal \"\" everywhere\n> instead of a slopbuf variable _if_ we wanted to have the guarantee\n> that our \"slopbuf\" could not be modified.\n>\n> But what I learned yesterday, is that at least gcc/clang will place\n> the entire variable in the rodata segment if the variable is both\n> marked const _and_ initialized.\n>\n> i.e. this will be allocated in the .rodata segment:\n>\n> const char strbuf_slopbuf[1] = \"\";\n>\n>>> #define STRBUF_INIT { .alloc = 0, .len = 0, .buf = (char*) &strbuf_slopbuf }\n>>>\n>>> respectively. Yeah, that's definitely preferable to a macro.\n>>> Something similar could be done in object.c.\n>>\n>> What is the main objective for doing this change? The \"make sure we\n>> do not write into that slopbuf\" assert() bothers you and you want to\n>> replace it with an address in the read-only segment?\n>\n> Actually nothing about the patch bothers me. The point of that patch\n> is to make sure we don't accidentally modify the slopbuf. I was just\n> looking for a way for the compiler to help out and wondering if there\n> was a reason we didn't attempt to do so in the first place.\n>\n> I think the main takeaway here is that I learned something yesterday\n> :-) I didn't actually intend to submit a patch for any of this, but\n> if anything useful came out of the discussion I thought Martin may\n> incorporate it into his patch if he wanted to.\n\nThanks for interesting information. I also learned something new. :-)\n\nMy first thought was, well, maybe someone writes '\\0' to sb.buf[len].\nThat should intuitively be a no-op and \"ok\", but the documentation\nactually only says that it's safe to write to positions 0 .. len-1, so\nsb.buf[len] is supposedly not safe (no-op or not). Maybe a degenerate\nand rarely tested case of otherwise sane code could end up writing '\\0'\nto slopbuf[0]. (Arguably strbuf_setlen should have been used instead.)\n\nI can see the value of placing the slopbuf in read-only memory, but I\nthink that would be a follow-up patch with its own pros and cons.\n\nMartin\n"},{"id":"327193","messageId":"20170825170411.qo6miyd46hfg6jzw@sigill.intra.peff.net","threadId":"46590","inReplyTo":"09bbbcd1429a28774ea2d8c67ef6106ab558c296.1503323391.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 4/4] ThreadSanitizer: add suppressions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-25T17:04:12Z","receivedAt":"2017-08-25T17:04:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 21, 2017 at 07:43:48PM +0200, Martin Ågren wrote:\n\n> Add a file .tsan-suppressions and list two functions in it: want_color()\n> and transfer_debug(). Both of these use the pattern\n> \n> \tstatic int foo = -1;\n> \tif (foo < 0)\n> \t\tfoo = bar();\n> \n> where bar always returns the same non-negative value. This can cause\n> ThreadSanitizer to diagnose a race when foo is written from two threads.\n> That is indeed a race, although it arguably doesn't matter in practice\n> since it's always the same value that is written.\n> \n> Add NEEDSWORK-comments to the functions so that this problem is not\n> forever swept way under the carpet.\n> \n> The suppressions-file is used by setting the environment variable\n> TSAN_OPTIONS to, e.g., \"suppressions=$(pwd)/.tsan-suppressions\". Observe\n> that relative paths such as \".tsan-suppressions\" might not work.\n\nThis looks good to me.\n\n-Peff\n"},{"id":"327295","messageId":"35ca4125-baed-c54b-7478-7531066daa0a@jeffhostetler.com","threadId":"46590","inReplyTo":"cover.1503323390.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/4] Some ThreadSanitizer-results","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-08-28T20:56:50Z","receivedAt":"2017-08-28T20:56:56Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 8/21/2017 1:43 PM, Martin Ågren wrote:\n> This is the second version of my series to try to address some issues\n> ... \n> 2) hashmap_add, which I could try my hands on if Jeff doesn't beat me to\n> it -- his proposed change should fix it and I doubt I could come up with\n> anything \"better\", considering he knows the code.\n\nNow that I'm back from vacation, let me take another stab at this.\n\nJeff\n\n"},{"id":"327459","messageId":"20170830185922.10107-1-git@jeffhostetler.com","threadId":"46590","inReplyTo":"adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com","subject":"[PATCH] hashmap: address ThreadSanitizer concerns","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-08-30T18:59:21Z","receivedAt":"2017-08-30T18:59:51Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nThis is to address concerns raised by ThreadSanitizer on the\nmailing list about threaded unprotected R/W access to map.size with my previous\n\"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).\n\nSee:\nhttps://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n\nTODO This version contains methods to disable item counting and automatic\nrehashing independently. Since the latter is implicit with the former, we\ncould get by with just the one, but I thought we could discuss it since it\ndoes provide a little extra clarity of intent.\n\n\nJeff Hostetler (1):\n hashmap: add API to disable item counting when threaded\n\n attr.c | 14 ++++---\n builtin/describe.c | 2 +-\n hashmap.c | 31 +++++++++++-----\n hashmap.h | 98 ++++++++++++++++++++++++++++++++++++++-----------\n name-hash.c | 6 ++-\n t/helper/test-hashmap.c | 2 +-\n 6 files changed, 113 insertions(+), 40 deletions(-)\n\n-- \n2.9.3\n\n"},{"id":"327460","messageId":"20170830185922.10107-2-git@jeffhostetler.com","threadId":"46590","inReplyTo":"20170830185922.10107-1-git@jeffhostetler.com","subject":"[PATCH] hashmap: add API to disable item counting when threaded","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-08-30T18:59:22Z","receivedAt":"2017-08-30T19:00:08Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nThis is to address concerns raised by ThreadSanitizer on the\nmailing list about threaded unprotected R/W access to map.size with my previous\n\"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4). See:\nhttps://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n\nAdd API to hashmap to disable item counting and to disable automatic rehashing. \nAlso include APIs to re-enable item counting and automatica rehashing.\n\nWhen item counting is disabled, the map.size field is invalid. So to\nprevent accidents, the field has been renamed and an accessor function\nhashmap_get_size() has been added. All direct references to this\nfield have been been updated. And the name of the field changed\nto map.private_size to communicate thie.\n\nSigned-off-by: Jeff Hostetler \n---\n attr.c | 14 ++++---\n builtin/describe.c | 2 +-\n hashmap.c | 31 +++++++++++-----\n hashmap.h | 98 ++++++++++++++++++++++++++++++++++++++-----------\n name-hash.c | 6 ++-\n t/helper/test-hashmap.c | 2 +-\n 6 files changed, 113 insertions(+), 40 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 56961f0..a987f37 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -149,10 +149,12 @@ struct all_attrs_item {\n static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)\n {\n \tint i;\n+\tunsigned int size;\n \n \thashmap_lock(map);\n \n-\tif (map->map.size < check->all_attrs_nr)\n+\tsize = hashmap_get_size(&map->map);\n+\tif (size < check->all_attrs_nr)\n \t\tdie(\"BUG: interned attributes shouldn't be deleted\");\n \n \t/*\n@@ -161,13 +163,13 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)\n \t * field), reallocate the provided attr_check instance's all_attrs\n \t * field and fill each entry with its corresponding git_attr.\n \t */\n-\tif (map->map.size != check->all_attrs_nr) {\n+\tif (size != check->all_attrs_nr) {\n \t\tstruct attr_hash_entry *e;\n \t\tstruct hashmap_iter iter;\n \t\thashmap_iter_init(&map->map, &iter);\n \n-\t\tREALLOC_ARRAY(check->all_attrs, map->map.size);\n-\t\tcheck->all_attrs_nr = map->map.size;\n+\t\tREALLOC_ARRAY(check->all_attrs, size);\n+\t\tcheck->all_attrs_nr = size;\n \n \t\twhile ((e = hashmap_iter_next(&iter))) {\n \t\t\tconst struct git_attr *a = e->value;\n@@ -235,10 +237,10 @@ static const struct git_attr *git_attr_internal(const char *name, int namelen)\n \n \tif (!a) {\n \t\tFLEX_ALLOC_MEM(a, name, name, namelen);\n-\t\ta->attr_nr = g_attr_hashmap.map.size;\n+\t\ta->attr_nr = hashmap_get_size(&g_attr_hashmap.map);\n \n \t\tattr_hashmap_add(&g_attr_hashmap, a->name, namelen, a);\n-\t\tassert(a->attr_nr == (g_attr_hashmap.map.size - 1));\n+\t\tassert(a->attr_nr == (hashmap_get_size(&g_attr_hashmap.map) - 1));\n \t}\n \n \thashmap_unlock(&g_attr_hashmap);\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 89ea1cd..1e3cbc7 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -505,7 +505,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \n \thashmap_init(&names, (hashmap_cmp_fn) commit_name_cmp, NULL, 0);\n \tfor_each_rawref(get_name, NULL);\n-\tif (!names.size && !always)\n+\tif (!hashmap_get_size(&names) && !always)\n \t\tdie(_(\"No names found, cannot describe anything.\"));\n \n \tif (argc == 0) {\ndiff --git a/hashmap.c b/hashmap.c\nindex 9b6a128..054f334 100644\n--- a/hashmap.c\n+++ b/hashmap.c\n@@ -116,9 +116,6 @@ static void rehash(struct hashmap *map, unsigned int newsize)\n \tunsigned int i, oldsize = map->tablesize;\n \tstruct hashmap_entry **oldtable = map->table;\n \n-\tif (map->disallow_rehash)\n-\t\treturn;\n-\n \talloc_table(map, newsize);\n \tfor (i = 0; i < oldsize; i++) {\n \t\tstruct hashmap_entry *e = oldtable[i];\n@@ -166,6 +163,17 @@ void hashmap_init(struct hashmap *map, hashmap_cmp_fn equals_function,\n \twhile (initial_size > size)\n \t\tsize <<= HASHMAP_RESIZE_BITS;\n \talloc_table(map, size);\n+\n+\t/*\n+\t * By default, we want to:\n+\t * [] keep track of the number of items in the map and\n+\t * [] allow the map to automatically grow as necessary.\n+\t *\n+\t * We probably only need to disable these if we are being\n+\t * used by a threaded caller.\n+\t */\n+\tmap->do_count_items = 1;\n+\tmap->do_auto_rehash = 1;\n }\n \n void hashmap_free(struct hashmap *map, int free_entries)\n@@ -206,9 +214,11 @@ void hashmap_add(struct hashmap *map, void *entry)\n \tmap->table[b] = entry;\n \n \t/* fix size and rehash if appropriate */\n-\tmap->size++;\n-\tif (map->size > map->grow_at)\n-\t\trehash(map, map->tablesize << HASHMAP_RESIZE_BITS);\n+\tif (map->do_count_items) {\n+\t\tmap->private_size++;\n+\t\tif (map->do_auto_rehash && map->private_size > map->grow_at)\n+\t\t\trehash(map, map->tablesize << HASHMAP_RESIZE_BITS);\n+\t}\n }\n \n void *hashmap_remove(struct hashmap *map, const void *key, const void *keydata)\n@@ -224,9 +234,12 @@ void *hashmap_remove(struct hashmap *map, const void *key, const void *keydata)\n \told->next = NULL;\n \n \t/* fix size and rehash if appropriate */\n-\tmap->size--;\n-\tif (map->size < map->shrink_at)\n-\t\trehash(map, map->tablesize >> HASHMAP_RESIZE_BITS);\n+\tif (map->do_count_items) {\n+\t\tmap->private_size--;\n+\t\tif (map->do_auto_rehash && map->private_size < map->shrink_at)\n+\t\t\trehash(map, map->tablesize >> HASHMAP_RESIZE_BITS);\n+\t}\n+\n \treturn old;\n }\n \ndiff --git a/hashmap.h b/hashmap.h\nindex 7a8fa7f..7b8e6f4 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -183,7 +183,7 @@ struct hashmap {\n \tconst void *cmpfn_data;\n \n \t/* total number of entries (0 means the hashmap is empty) */\n-\tunsigned int size;\n+\tunsigned int private_size; /* use hashmap_get_size() */\n \n \t/*\n \t * tablesize is the allocated size of the hash table. A non-0 value\n@@ -196,8 +196,8 @@ struct hashmap {\n \tunsigned int grow_at;\n \tunsigned int shrink_at;\n \n-\t/* See `hashmap_disallow_rehash`. */\n-\tunsigned disallow_rehash : 1;\n+\tunsigned int do_count_items : 1;\n+\tunsigned int do_auto_rehash : 1;\n };\n \n /* hashmap functions */\n@@ -253,6 +253,19 @@ static inline void hashmap_entry_init(void *entry, unsigned int hash)\n }\n \n /*\n+ * Return the number of items in the map.\n+ */\n+inline unsigned int hashmap_get_size(struct hashmap *map)\n+{\n+\tif (map->do_count_items)\n+\t\treturn map->private_size;\n+\n+\t/* TODO Consider counting them and returning that. */\n+\tdie(\"hashmap_get_size: size not set\");\n+\treturn 0;\n+}\n+\n+/*\n * Returns the hashmap entry for the specified key, or NULL if not found.\n *\n * `map` is the hashmap structure.\n@@ -345,24 +358,6 @@ extern void *hashmap_remove(struct hashmap *map, const void *key,\n int hashmap_bucket(const struct hashmap *map, unsigned int hash);\n \n /*\n- * Disallow/allow rehashing of the hashmap.\n- * This is useful if the caller knows that the hashmap needs multi-threaded\n- * access. The caller is still required to guard/lock searches and inserts\n- * in a manner appropriate to their usage. This simply prevents the table\n- * from being unexpectedly re-mapped.\n- *\n- * It is up to the caller to ensure that the hashmap is initialized to a\n- * reasonable size to prevent poor performance.\n- *\n- * A call to allow rehashing does not force a rehash; that might happen\n- * with the next insert or delete.\n- */\n-static inline void hashmap_disallow_rehash(struct hashmap *map, unsigned value)\n-{\n-\tmap->disallow_rehash = value;\n-}\n-\n-/*\n * Used to iterate over all entries of a hashmap. Note that it is\n * not safe to add or remove entries to the hashmap while\n * iterating.\n@@ -387,6 +382,67 @@ static inline void *hashmap_iter_first(struct hashmap *map,\n \treturn hashmap_iter_next(iter);\n }\n \n+/*\n+ * Disable item counting when adding/removing items.\n+ *\n+ * Normally, the hashmap keeps track of the number of items in the map\n+ * and uses it to dynamically resize it. This (both the counting and\n+ * the resizing) can cause problems when the map is being used by\n+ * threaded callers (because the hashmap code does not know about the\n+ * locking strategy used by the threaded callers and therefore, does\n+ * not know how to protect the \"private_size\" counter).\n+ *\n+ * (This will implicitly disable auto resizing.)\n+ */\n+static inline void hashmap_disable_item_counting(struct hashmap *map)\n+{\n+\tmap->do_count_items = 0;\n+}\n+\n+/*\n+ * Re-enable item couting when adding/removing items.\n+ * If counting is currently disabled, it will force count them.\n+ *\n+ * This might be used after leaving a threaded section.\n+ */\n+static inline void hashmap_enable_item_counting(struct hashmap *map)\n+{\n+\tvoid *item;\n+\tunsigned int n = 0;\n+\tstruct hashmap_iter iter;\n+\n+\thashmap_iter_init(map, &iter);\n+\twhile ((item = hashmap_iter_next(&iter)))\n+\t\tn++;\n+\n+\tmap->do_count_items = 1;\n+\tmap->private_size = n;\n+}\n+\n+/*\n+ * Disable automatic resizing when adding/removing items.\n+ *\n+ * This prevents the hashmap from resizing the table size and\n+ * redistributing the items into new bucket chains. This causes\n+ * problems when the map is being used by threaded callers (because\n+ * the hashmap code does not know about the locking strategy used by\n+ * the threaded callers and therefore, does not know how to safely\n+ * build new chains while other threads may be walking them).\n+ */\n+static inline void hashmap_disable_auto_rehash(struct hashmap *map)\n+{\n+\tmap->do_auto_rehash = 0;\n+}\n+\n+/*\n+ * Re-enable automatic resizing when adding/removing items.\n+ * This DOES NOT force an immediate rehash.\n+ */\n+static inline void hashmap_enable_auto_rehash(struct hashmap *map)\n+{\n+\tmap->do_auto_rehash = 1;\n+}\n+\n /* String interning */\n \n /*\ndiff --git a/name-hash.c b/name-hash.c\nindex 0e10f3e..829ff59 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -580,9 +580,11 @@ static void lazy_init_name_hash(struct index_state *istate)\n \t\t\tNULL, istate->cache_nr);\n \n \tif (lookup_lazy_params(istate)) {\n-\t\thashmap_disallow_rehash(&istate->dir_hash, 1);\n+\t\thashmap_disable_item_counting(&istate->dir_hash);\n+\t\thashmap_disable_auto_rehash(&istate->dir_hash);\n \t\tthreaded_lazy_init_name_hash(istate);\n-\t\thashmap_disallow_rehash(&istate->dir_hash, 0);\n+\t\thashmap_enable_auto_rehash(&istate->dir_hash);\n+\t\thashmap_enable_item_counting(&istate->dir_hash);\n \t} else {\n \t\tint nr;\n \t\tfor (nr = 0; nr < istate->cache_nr; nr++)\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 095d739..84c57b9 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -237,7 +237,7 @@ int cmd_main(int argc, const char **argv)\n \t\t} else if (!strcmp(\"size\", cmd)) {\n \n \t\t\t/* print table sizes */\n-\t\t\tprintf(\"%u %u\\n\", map.tablesize, map.size);\n+\t\t\tprintf(\"%u %u\\n\", map.tablesize, hashmap_get_size(&map));\n \n \t\t} else if (!strcmp(\"intern\", cmd) && l1) {\n \n-- \n2.9.3\n\n"},{"id":"327520","messageId":"alpine.DEB.2.21.1.1709020109520.4132@virtualbox","threadId":"46590","inReplyTo":"20170830185922.10107-2-git@jeffhostetler.com","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-01T23:31:19Z","receivedAt":"2017-09-01T23:31:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jeff,\n\nOn Wed, 30 Aug 2017, Jeff Hostetler wrote:\n\n> From: Jeff Hostetler \n> \n> This is to address concerns raised by ThreadSanitizer on the mailing\n> list about threaded unprotected R/W access to map.size with my previous\n> \"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).\n> See:\n> https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n> \n> Add API to hashmap to disable item counting and to disable automatic\n> rehashing. Also include APIs to re-enable item counting and automatica\n> rehashing.\n> \n> When item counting is disabled, the map.size field is invalid. So to\n> prevent accidents, the field has been renamed and an accessor function\n> hashmap_get_size() has been added. All direct references to this field\n> have been been updated. And the name of the field changed to\n> map.private_size to communicate thie.\n> \n> Signed-off-by: Jeff Hostetler \n> ---\n\nThe Git contribution process forces me to point out lines longer than 80\ncolumns. I wish there was already an automated tool to fix that, but we\n(as in \"the core Git developers\") have not yet managed to agree on one. So\nI'll have to ask you to identify and fix them manually.\n\n> @@ -253,6 +253,19 @@ static inline void hashmap_entry_init(void *entry, unsigned int hash)\n> }\n> \n> /*\n> + * Return the number of items in the map.\n> + */\n> +inline unsigned int hashmap_get_size(struct hashmap *map)\n> +{\n> +\tif (map->do_count_items)\n> +\t\treturn map->private_size;\n> +\n> +\t/* TODO Consider counting them and returning that. */\n\nI'd rather not. If counting is disabled, it is disabled.\n\n> +\tdie(\"hashmap_get_size: size not set\");\n\nBefore anybody can ask for this message to be wrapped in _(...) to be\ntranslateable, let me suggest instead to add the prefix \"BUG: \".\n\n> +static inline void hashmap_enable_item_counting(struct hashmap *map)\n> +{\n> +\tvoid *item;\n> +\tunsigned int n = 0;\n> +\tstruct hashmap_iter iter;\n> +\n> +\thashmap_iter_init(map, &iter);\n> +\twhile ((item = hashmap_iter_next(&iter)))\n> +\t\tn++;\n> +\n> +\tmap->do_count_items = 1;\n> +\tmap->private_size = n;\n> +}\n\nBTW this made me think that we may have a problem in our code since\nswitching from my original hashmap implementation to the bucket one added\nin 6a364ced497 (add a hashtable implementation that supports O(1) removal,\n2013-11-14): while it is not expected that there are many collisions, the\n\"grow_at\" logic still essentially assumes the number of buckets to be\nequal to the number of hashmap entries.\n\nYour code simply reiterates that assumption, so I do not blame you for\nanything here, nor ask you to change your patch.\n\nBut it does look a bit weird to assume so much about the nature of our\ndata, without having any real-life numbers. I wish I had more time so that\nI could afford to run a couple of tests on this hashmap, such as: what is\nthe typical difference between bucket count and entry count, or the median\nof the bucket sizes when the map is 80% full (i.e. *just* below the grow\nthreshold).\n\n> diff --git a/name-hash.c b/name-hash.c\n> index 0e10f3e..829ff59 100644\n> --- a/name-hash.c\n> +++ b/name-hash.c\n> @@ -580,9 +580,11 @@ static void lazy_init_name_hash(struct index_state *istate)\n> \t\t\tNULL, istate->cache_nr);\n> \n> \tif (lookup_lazy_params(istate)) {\n> -\t\thashmap_disallow_rehash(&istate->dir_hash, 1);\n> +\t\thashmap_disable_item_counting(&istate->dir_hash);\n> +\t\thashmap_disable_auto_rehash(&istate->dir_hash);\n> \t\tthreaded_lazy_init_name_hash(istate);\n> -\t\thashmap_disallow_rehash(&istate->dir_hash, 0);\n> +\t\thashmap_enable_auto_rehash(&istate->dir_hash);\n> +\t\thashmap_enable_item_counting(&istate->dir_hash);\n\nBy your rationale, it would be enough to simply disable and re-enable\ncounting...\n\nThe rest of the patch looks just dandy to me.\n\nThanks,\nDscho\n"},{"id":"327521","messageId":"20170901235030.GD143138@aiede.mtv.corp.google.com","threadId":"46590","inReplyTo":"alpine.DEB.2.21.1.1709020109520.4132@virtualbox","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-01T23:50:30Z","receivedAt":"2017-09-01T23:50:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJohannes Schindelin wrote:\n> On Wed, 30 Aug 2017, Jeff Hostetler wrote:\n\n>> This is to address concerns raised by ThreadSanitizer on the mailing\n>> list about threaded unprotected R/W access to map.size with my previous\n>> \"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).\n\nNice!\n\nWhat does the message from TSan look like? (The full message doesn't\nneed to go in the commit message, but a snippet can help.) How can I\nreproduce it?\n\n\n>> See:\n>> https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n>>\n>> Add API to hashmap to disable item counting and to disable automatic\n>> rehashing. Also include APIs to re-enable item counting and automatica\n>> rehashing.\n>>\n>> When item counting is disabled, the map.size field is invalid. So to\n>> prevent accidents, the field has been renamed and an accessor function\n>> hashmap_get_size() has been added. All direct references to this field\n>> have been been updated. And the name of the field changed to\n>> map.private_size to communicate thie.\n>>\n>> Signed-off-by: Jeff Hostetler \n>> ---\n>\n> The Git contribution process forces me to point out lines longer than 80\n> columns. I wish there was already an automated tool to fix that, but we\n> (as in \"the core Git developers\") have not yet managed to agree on one. So\n> I'll have to ask you to identify and fix them manually.\n\nI *think* (but am not sure) that there is an implied bug report in this\ncomment, but I am not sure what that bug report is and how I can help\nwith it! If you have a link to an issue tracker, thread, wiki page, or\nother pointer where I can learn more, then that might help me.\n\nHave you experimented with the patches in Junio's branch\nbw/git-clang-format?\n\n[...]\n>> +\t/* TODO Consider counting them and returning that. */\n>\n> I'd rather not. If counting is disabled, it is disabled.\n>\n>> +\tdie(\"hashmap_get_size: size not set\");\n>\n> Before anybody can ask for this message to be wrapped in _(...) to be\n> translateable, let me suggest instead to add the prefix \"BUG: \".\n\nNowadays (since v2.13.2~26^2~3, 2017-05-12), there is a BUG() function\nthat you can use in place of die():\n\n\tBUG(\"hashmap_get_size: size not set\");\n\nThanks and hope that helps,\nJonathan\n"},{"id":"327524","messageId":"20170902080535.j7dommmxowpbk2no@sigill.intra.peff.net","threadId":"46590","inReplyTo":"20170830185922.10107-2-git@jeffhostetler.com","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-02T08:05:35Z","receivedAt":"2017-09-02T08:05:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2017 at 06:59:22PM +0000, Jeff Hostetler wrote:\n\n> From: Jeff Hostetler \n> \n> This is to address concerns raised by ThreadSanitizer on the\n> mailing list about threaded unprotected R/W access to map.size with my previous\n> \"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4). See:\n> https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n> \n> Add API to hashmap to disable item counting and to disable automatic rehashing. \n> Also include APIs to re-enable item counting and automatica rehashing.\n\nIt may be worth summarizing the discussion at that thread here.\n\nAt first glance, it looks like this is woefully inadequate for allowing\nmulti-threaded access. But after digging, the issue is that we're really\ntrying to accommodate one specific callers which is doing its own\nper-bucket locking, and which needs the internals of the hashmap to be\ntruly read-only.\n\nI suspect the code might be easier to follow if that pattern were pushed\ninto its own threaded_hashmap that disabled the size and handled the\nmod-n locking, but I don't insist on that as a blocker to this fix.\n\n-Peff\n"},{"id":"327525","messageId":"20170902081747.lca2kkzpniykdxy2@sigill.intra.peff.net","threadId":"46590","inReplyTo":"alpine.DEB.2.21.1.1709020109520.4132@virtualbox","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-02T08:17:47Z","receivedAt":"2017-09-02T08:17:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 02, 2017 at 01:31:19AM +0200, Johannes Schindelin wrote:\n\n> > https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n> [...]\n> > Add API to hashmap to disable item counting and to disable automatic\n> > rehashing. Also include APIs to re-enable item counting and automatica\n> > rehashing.\n> [...]\n> \n> The Git contribution process forces me to point out lines longer than 80\n> columns. I wish there was already an automated tool to fix that, but we\n> (as in \"the core Git developers\") have not yet managed to agree on one. So\n> I'll have to ask you to identify and fix them manually.\n\nPerhaps it would be helpful if you pointed out the lines that are too\nlong. Because I don't see any being added by the patch. There are two in\nthe commit message. One is a URL. For the other, which is 82 characters,\nI'm not sure there is a better tool than \"turn on text wrapping in your\neditor\".\n\n> > +\t/* TODO Consider counting them and returning that. */\n> \n> I'd rather not. If counting is disabled, it is disabled.\n> \n> > +\tdie(\"hashmap_get_size: size not set\");\n> \n> Before anybody can ask for this message to be wrapped in _(...) to be\n> translateable, let me suggest instead to add the prefix \"BUG: \".\n\nAgreed on both (and Jonathan's suggestion to just use BUG()).\n\n> > +static inline void hashmap_enable_item_counting(struct hashmap *map)\n> > +{\n> > +\tvoid *item;\n> > +\tunsigned int n = 0;\n> > +\tstruct hashmap_iter iter;\n> > +\n> > +\thashmap_iter_init(map, &iter);\n> > +\twhile ((item = hashmap_iter_next(&iter)))\n> > +\t\tn++;\n> > +\n> > +\tmap->do_count_items = 1;\n> > +\tmap->private_size = n;\n> > +}\n> \n> BTW this made me think that we may have a problem in our code since\n> switching from my original hashmap implementation to the bucket one added\n> in 6a364ced497 (add a hashtable implementation that supports O(1) removal,\n> 2013-11-14): while it is not expected that there are many collisions, the\n> \"grow_at\" logic still essentially assumes the number of buckets to be\n> equal to the number of hashmap entries.\n\nI'm confused about what the problem is. If I am reading the code\ncorrectly, \"size\" is always the number of elements and \"grow_at\" is the\ntable size times a load factor. Those are the same numbers you'd use to\ndecide to grow in an open-address table.\n\nIt's true that this does not take into account the actual number of\ncollisions we see (or the average per bucket, or however you want to\ncount it). But generally nor do open-address schemes (and certainly our\nother hash tables just use load factor to decide when to grow).\n\nAm I missing something?\n\n-Peff\n"},{"id":"327527","messageId":"20170902083920.simgdrdteorxt6yo@ruderich.org","threadId":"46590","inReplyTo":"20170830185922.10107-2-git@jeffhostetler.com","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-09-02T08:39:20Z","receivedAt":"2017-09-02T08:39:30Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Wed, Aug 30, 2017 at 06:59:22PM +0000, Jeff Hostetler wrote:\n> [snip]\n> +/*\n> + * Re-enable item couting when adding/removing items.\n> + * If counting is currently disabled, it will force count them.\n\nThe code always recounts them. Either the comment or the\ncode should be adjusted.\n\n> + * This might be used after leaving a threaded section.\n> + */\n> +static inline void hashmap_enable_item_counting(struct hashmap *map)\n> +{\n> +\tvoid *item;\n> +\tunsigned int n = 0;\n> +\tstruct hashmap_iter iter;\n> +\n> +\thashmap_iter_init(map, &iter);\n> +\twhile ((item = hashmap_iter_next(&iter)))\n> +\t\tn++;\n> +\n> +\tmap->do_count_items = 1;\n> +\tmap->private_size = n;\n> +}\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"327535","messageId":"alpine.DEB.2.21.1.1709041758350.4132@virtualbox","threadId":"46590","inReplyTo":"20170902081747.lca2kkzpniykdxy2@sigill.intra.peff.net","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-04T15:59:43Z","receivedAt":"2017-09-04T16:00:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 2 Sep 2017, Jeff King wrote:\n\n> On Sat, Sep 02, 2017 at 01:31:19AM +0200, Johannes Schindelin wrote:\n> \n> > > https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n> > [...]\n> > > +static inline void hashmap_enable_item_counting(struct hashmap *map)\n> > > +{\n> > > +\tvoid *item;\n> > > +\tunsigned int n = 0;\n> > > +\tstruct hashmap_iter iter;\n> > > +\n> > > +\thashmap_iter_init(map, &iter);\n> > > +\twhile ((item = hashmap_iter_next(&iter)))\n> > > +\t\tn++;\n> > > +\n> > > +\tmap->do_count_items = 1;\n> > > +\tmap->private_size = n;\n> > > +}\n> > \n> > BTW this made me think that we may have a problem in our code since\n> > switching from my original hashmap implementation to the bucket one\n> > added in 6a364ced497 (add a hashtable implementation that supports\n> > O(1) removal, 2013-11-14): while it is not expected that there are\n> > many collisions, the \"grow_at\" logic still essentially assumes the\n> > number of buckets to be equal to the number of hashmap entries.\n> \n> I'm confused about what the problem is. If I am reading the code\n> correctly, \"size\" is always the number of elements and \"grow_at\" is the\n> table size times a load factor. Those are the same numbers you'd use to\n> decide to grow in an open-address table.\n> \n> It's true that this does not take into account the actual number of\n> collisions we see (or the average per bucket, or however you want to\n> count it). But generally nor do open-address schemes (and certainly our\n> other hash tables just use load factor to decide when to grow).\n\nIn the worst case, there is only one bucket when the table is grown\nalready, is all I tried to point out.\n\nCiao,\nDscho\n"},{"id":"327584","messageId":"9ec32edc-5aeb-53c0-7888-541f7a9db8bf@jeffhostetler.com","threadId":"46590","inReplyTo":"alpine.DEB.2.21.1.1709020109520.4132@virtualbox","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-09-05T16:33:21Z","receivedAt":"2017-09-05T16:33:32Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/1/2017 7:31 PM, Johannes Schindelin wrote:\n> Hi Jeff,\n> \n> On Wed, 30 Aug 2017, Jeff Hostetler wrote:\n> \n>> From: Jeff Hostetler \n>>\n>> This is to address concerns raised by ThreadSanitizer on the mailing\n>> list about threaded unprotected R/W access to map.size with my previous\n>> \"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).\n>> See:\n>> https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n>>\n>> Add API to hashmap to disable item counting and to disable automatic\n>> rehashing. Also include APIs to re-enable item counting and automatica\n>> rehashing.\n>>\n>> When item counting is disabled, the map.size field is invalid. So to\n>> prevent accidents, the field has been renamed and an accessor function\n>> hashmap_get_size() has been added. All direct references to this field\n>> have been been updated. And the name of the field changed to\n>> map.private_size to communicate thie.\n>>\n>> Signed-off-by: Jeff Hostetler \n>> ---\n> \n> The Git contribution process forces me to point out lines longer than 80\n> columns. I wish there was already an automated tool to fix that, but we\n> (as in \"the core Git developers\") have not yet managed to agree on one. So\n> I'll have to ask you to identify and fix them manually.\n\nI'm not sure which lines you're talking about, but I'll\ngive it another scan and double check.\n\nThere's not much I can do about the public-inbox.org URL.\n\n> \n>> @@ -253,6 +253,19 @@ static inline void hashmap_entry_init(void *entry, unsigned int hash)\n>> }\n>> \n>> /*\n>> + * Return the number of items in the map.\n>> + */\n>> +inline unsigned int hashmap_get_size(struct hashmap *map)\n>> +{\n>> +\tif (map->do_count_items)\n>> +\t\treturn map->private_size;\n>> +\n>> +\t/* TODO Consider counting them and returning that. */\n> \n> I'd rather not. If counting is disabled, it is disabled.\n> \n>> +\tdie(\"hashmap_get_size: size not set\");\n> \n> Before anybody can ask for this message to be wrapped in _(...) to be\n> translateable, let me suggest instead to add the prefix \"BUG: \".\n\nGood point. Thanks.\n\n> \n>> +static inline void hashmap_enable_item_counting(struct hashmap *map)\n>> +{\n>> +\tvoid *item;\n>> +\tunsigned int n = 0;\n>> +\tstruct hashmap_iter iter;\n>> +\n>> +\thashmap_iter_init(map, &iter);\n>> +\twhile ((item = hashmap_iter_next(&iter)))\n>> +\t\tn++;\n>> +\n>> +\tmap->do_count_items = 1;\n>> +\tmap->private_size = n;\n>> +}\n> \n> BTW this made me think that we may have a problem in our code since\n> switching from my original hashmap implementation to the bucket one added\n> in 6a364ced497 (add a hashtable implementation that supports O(1) removal,\n> 2013-11-14): while it is not expected that there are many collisions, the\n> \"grow_at\" logic still essentially assumes the number of buckets to be\n> equal to the number of hashmap entries.\n> \n> Your code simply reiterates that assumption, so I do not blame you for\n> anything here, nor ask you to change your patch.\n\nI'm not sure what you're saying here. The iterator iterates over\nall entries (and handles walking collision chains), so my newly\ncomputed count should be correct and all of this is independent of\nthe \"grow-at\" and table-size logic.\n\nI'm not forcing a rehash when counting is enabled. I'm just\nreestablishing the expected state. The next insert may cause\na rehash, but I'm not forcing it.\n\nHowever, there is an assumption that the caller pre-allocated sufficient\ntable-size space to avoid poor performance for the duration of the\nnon-counting period.\n\n> \n> But it does look a bit weird to assume so much about the nature of our\n> data, without having any real-life numbers. I wish I had more time so that\n> I could afford to run a couple of tests on this hashmap, such as: what is\n> the typical difference between bucket count and entry count, or the median\n> of the bucket sizes when the map is 80% full (i.e. *just* below the grow\n> threshold).\n\nPersonally, I think the 80% threshold is too aggressive (and the\ndefault size is too small), but that's a different question.\n\nThe hashmap in question contains directory pathnames, so the\ndistribution will be completely dependent on the shape of the\ndata.\n\nFWIW, I created a tool to dump some of this data. See:\n t/helper/test-lazy-init-name-hash.c\n\n> \n>> diff --git a/name-hash.c b/name-hash.c\n>> index 0e10f3e..829ff59 100644\n>> --- a/name-hash.c\n>> +++ b/name-hash.c\n>> @@ -580,9 +580,11 @@ static void lazy_init_name_hash(struct index_state *istate)\n>> \t\t\tNULL, istate->cache_nr);\n>> \n>> \tif (lookup_lazy_params(istate)) {\n>> -\t\thashmap_disallow_rehash(&istate->dir_hash, 1);\n>> +\t\thashmap_disable_item_counting(&istate->dir_hash);\n>> +\t\thashmap_disable_auto_rehash(&istate->dir_hash);\n>> \t\tthreaded_lazy_init_name_hash(istate);\n>> -\t\thashmap_disallow_rehash(&istate->dir_hash, 0);\n>> +\t\thashmap_enable_auto_rehash(&istate->dir_hash);\n>> +\t\thashmap_enable_item_counting(&istate->dir_hash);\n> \n> By your rationale, it would be enough to simply disable and re-enable\n> counting...\n> \n> The rest of the patch looks just dandy to me.\n> \n> Thanks,\n> Dscho\n> \n\nthanks\nJeff\n\n"},{"id":"327585","messageId":"06c552de-80a0-9cd0-d016-07a4ab0afaf0@jeffhostetler.com","threadId":"46590","inReplyTo":"20170901235030.GD143138@aiede.mtv.corp.google.com","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-09-05T16:39:20Z","receivedAt":"2017-09-05T16:39:33Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/1/2017 7:50 PM, Jonathan Nieder wrote:\n> Hi,\n> \n> Johannes Schindelin wrote:\n>> On Wed, 30 Aug 2017, Jeff Hostetler wrote:\n> \n>>> This is to address concerns raised by ThreadSanitizer on the mailing\n>>> list about threaded unprotected R/W access to map.size with my previous\n>>> \"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).\n> \n> Nice!\n> \n> What does the message from TSan look like? (The full message doesn't\n> need to go in the commit message, but a snippet can help.) How can I\n> reproduce it?\n\nI'll let Martin common on how to run TSan; I'm just going on\nwhat he reported in the \"tsan: t3008...\" message from the URL\nI quoted. I didn't think to copy that text into the commit\nmessage because it is just stack traces and too long, but I\ncould include a snippet.\n\nThanks,\nJeff\n"},{"id":"327586","messageId":"52aa964f-0511-9105-11d9-6e41221eb41e@jeffhostetler.com","threadId":"46590","inReplyTo":"20170902081747.lca2kkzpniykdxy2@sigill.intra.peff.net","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-09-05T16:54:45Z","receivedAt":"2017-09-05T16:54:55Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/2/2017 4:17 AM, Jeff King wrote:\n> On Sat, Sep 02, 2017 at 01:31:19AM +0200, Johannes Schindelin wrote:\n>> Before anybody can ask for this message to be wrapped in _(...) to be\n>> translateable, let me suggest instead to add the prefix \"BUG: \".\n> \n> Agreed on both (and Jonathan's suggestion to just use BUG()).\n\nwill do. thanks.\n\n> \n>>> +static inline void hashmap_enable_item_counting(struct hashmap *map)\n>>> +{\n>>> +\tvoid *item;\n>>> +\tunsigned int n = 0;\n>>> +\tstruct hashmap_iter iter;\n>>> +\n>>> +\thashmap_iter_init(map, &iter);\n>>> +\twhile ((item = hashmap_iter_next(&iter)))\n>>> +\t\tn++;\n>>> +\n>>> +\tmap->do_count_items = 1;\n>>> +\tmap->private_size = n;\n>>> +}\n>>\n>> BTW this made me think that we may have a problem in our code since\n>> switching from my original hashmap implementation to the bucket one added\n>> in 6a364ced497 (add a hashtable implementation that supports O(1) removal,\n>> 2013-11-14): while it is not expected that there are many collisions, the\n>> \"grow_at\" logic still essentially assumes the number of buckets to be\n>> equal to the number of hashmap entries.\n> \n> I'm confused about what the problem is. If I am reading the code\n> correctly, \"size\" is always the number of elements and \"grow_at\" is the\n> table size times a load factor. Those are the same numbers you'd use to\n> decide to grow in an open-address table.\n> \n> It's true that this does not take into account the actual number of\n> collisions we see (or the average per bucket, or however you want to\n> count it). But generally nor do open-address schemes (and certainly our\n> other hash tables just use load factor to decide when to grow).\n> \n> Am I missing something?\n> \n> -Peff\n> \n\nHashmap is not thread-safe by itself. There are several uses of\nit in a threaded context and they all handle their own locking\nbefore accessing the hashmap. Those usually work by locking the\nwhole hashmap.\n\nMy changes in \"lazy-init-name-hash\" deviated from that pattern\nby locking on individual hash chains. That is, n locks each\ncontrolling 1/nth of the chains.\n\nhttps://public-inbox.org/git/1490202865-31325-1-git-send-email-git@jeffhostetler.com/\n\nTo do that I had to disable automatic rehashing for the duration\nof my threaded computation. The problem that TSan identified is\nthat \"size\" is always incremented during inserts and it doesn't\nhave any locks protecting it. So even though auto-rehash was\ndisabled, we are still counting the number of items in the map.\nNot a terrible problem, but still a race.\n\nJeff\n\n"},{"id":"327589","messageId":"77c206f8-43f6-4468-3289-096e37b9fead@jeffhostetler.com","threadId":"46590","inReplyTo":"20170902080535.j7dommmxowpbk2no@sigill.intra.peff.net","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-09-05T17:07:33Z","receivedAt":"2017-09-05T17:07:42Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/2/2017 4:05 AM, Jeff King wrote:\n> On Wed, Aug 30, 2017 at 06:59:22PM +0000, Jeff Hostetler wrote:\n> \n>> From: Jeff Hostetler \n>>\n>> This is to address concerns raised by ThreadSanitizer on the\n>> mailing list about threaded unprotected R/W access to map.size with my previous\n>> \"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4). See:\n>> https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n>>\n>> Add API to hashmap to disable item counting and to disable automatic rehashing.\n>> Also include APIs to re-enable item counting and automatica rehashing.\n> \n> It may be worth summarizing the discussion at that thread here.\n> \n> At first glance, it looks like this is woefully inadequate for allowing\n> multi-threaded access. But after digging, the issue is that we're really\n> trying to accommodate one specific callers which is doing its own\n> per-bucket locking, and which needs the internals of the hashmap to be\n> truly read-only.\n> \n> I suspect the code might be easier to follow if that pattern were pushed\n> into its own threaded_hashmap that disabled the size and handled the\n> mod-n locking, but I don't insist on that as a blocker to this fix.\n\nAgreed. It would be better and easier to understand to produce\na thread-safe version of the hashmap code -- there are other uses\nof the code that are currently doing their own locking that might\nbenefit, but I'm not looking to gratuitously refactor things.\n\nThe other issue was that we are multi-threaded during the\nlazy-init phase, but the main status-or-whatever code that then\nuses the hashmap is not. So we only pay for the locking during\nthe multi-threaded usage.\n\nThanks,\nJeff\n"},{"id":"327591","messageId":"CAN0heSoJDL9pWELD6ciLTmWf-a=oyxe4EXXOmCKvsG5MSuzxsA@mail.gmail.com","threadId":"46590","inReplyTo":"06c552de-80a0-9cd0-d016-07a4ab0afaf0@jeffhostetler.com","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-05T17:13:05Z","receivedAt":"2017-09-05T17:13:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 5 September 2017 at 18:39, Jeff Hostetler wrote:\n>\n>\n> On 9/1/2017 7:50 PM, Jonathan Nieder wrote:\n>>\n>> Hi,\n>>\n>> Johannes Schindelin wrote:\n>>>\n>>> On Wed, 30 Aug 2017, Jeff Hostetler wrote:\n>>\n>>\n>>>> This is to address concerns raised by ThreadSanitizer on the mailing\n>>>> list about threaded unprotected R/W access to map.size with my previous\n>>>> \"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).\n>>\n>>\n>> Nice!\n>>\n>> What does the message from TSan look like? (The full message doesn't\n>> need to go in the commit message, but a snippet can help.) How can I\n>> reproduce it?\n>\n>\n> I'll let Martin common on how to run TSan; I'm just going on\n> what he reported in the \"tsan: t3008...\" message from the URL\n> I quoted. I didn't think to copy that text into the commit\n> message because it is just stack traces and too long, but I\n> could include a snippet.\n\nI ran the test suite with ThreadSanitizer:\n\n$ make SANITIZE=thread test\n\nAny failures were then inspected:\n\n$ cd t\n$ ./t3008-ls-files-lazy-init-name-hash.sh --verbose\n\nThat can be done with or without ma/ts-cleanups. That series adds a file\n.tsan-suppressions, which can be used by defining the environment\nvariable\n\nTSAN_OPTIONS=\"suppressions=/some/absolute/path/.tsan-suppressions\"\n\nin order to suppress some findings which are indeed races, but which are\n\"not a problem in practice\".\n\nMartin\n"},{"id":"327621","messageId":"xmqqo9qoaab4.fsf@gitster.mtv.corp.google.com","threadId":"46590","inReplyTo":"20170830185922.10107-2-git@jeffhostetler.com","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T01:24:31Z","receivedAt":"2017-09-06T01:24:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> From: Jeff Hostetler \n>\n> This is to address concerns raised by ThreadSanitizer on the\n> mailing list about threaded unprotected R/W access to map.size with my previous\n> \"disallow rehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4). See:\n> https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n>\n> Add API to hashmap to disable item counting and to disable automatic rehashing. \n> Also include APIs to re-enable item counting and automatica rehashing.\n>\n> When item counting is disabled, the map.size field is invalid. So to\n> prevent accidents, the field has been renamed and an accessor function\n> hashmap_get_size() has been added. All direct references to this\n> field have been been updated. And the name of the field changed\n> to map.private_size to communicate thie.\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> attr.c | 14 ++++---\n> builtin/describe.c | 2 +-\n> hashmap.c | 31 +++++++++++-----\n> hashmap.h | 98 ++++++++++++++++++++++++++++++++++++++-----------\n> name-hash.c | 6 ++-\n> t/helper/test-hashmap.c | 2 +-\n> 6 files changed, 113 insertions(+), 40 deletions(-)\n\nI feel somewhat stupid to say this, especially after seeing many\npeople applaud this patch, but I do not seem to be able to even\nbuild Git with this patch. I am getting:\n\n common-main.o: In function `hashmap_get_size':\n /home/gitster/w/git.git/hashmap.h:260: multiple definition of `hashmap_get_size'\n fast-import.o:/home/gitster/w/git.git/hashmap.h:260: first defined here\n libgit.a(argv-array.o): In function `hashmap_get_size':\n /home/gitster/w/git.git/hashmap.h:260: multiple definition of `hashmap_get_size'\n fast-import.o:/home/gitster/w/git.git/hashmap.h:260: first defined here\n libgit.a(attr.o): In function `hashmap_get_size':\n ...\n\nand wonder if others are building with different options or something..\n\n> diff --git a/hashmap.h b/hashmap.h\n> index 7a8fa7f..7b8e6f4 100644\n> --- a/hashmap.h\n> +++ b/hashmap.h\n> @@ -253,6 +253,19 @@ static inline void hashmap_entry_init(void *entry, unsigned int hash)\n> }\n> \n> /*\n> + * Return the number of items in the map.\n> + */\n> +inline unsigned int hashmap_get_size(struct hashmap *map)\n\nI think this must become static inline like everybody else in this\nfile, at least.\n\nI also wonder if this header is excessively inlining too many\nfunctions without measuring first, but that is a different story.\n"},{"id":"327631","messageId":"xmqqwp5c7aqr.fsf@gitster.mtv.corp.google.com","threadId":"46590","inReplyTo":"20170902081747.lca2kkzpniykdxy2@sigill.intra.peff.net","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T03:43:24Z","receivedAt":"2017-09-06T03:43:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Sep 02, 2017 at 01:31:19AM +0200, Johannes Schindelin wrote:\n>\n>> BTW this made me think that we may have a problem in our code since\n>> switching from my original hashmap implementation to the bucket one added\n>> in 6a364ced497 (add a hashtable implementation that supports O(1) removal,\n>> 2013-11-14): while it is not expected that there are many collisions, the\n>> \"grow_at\" logic still essentially assumes the number of buckets to be\n>> equal to the number of hashmap entries.\n>\n> I'm confused about what the problem is. If I am reading the code\n> correctly, \"size\" is always the number of elements and \"grow_at\" is the\n> table size times a load factor. Those are the same numbers you'd use to\n> decide to grow in an open-address table.\n>\n> It's true that this does not take into account the actual number of\n> collisions we see (or the average per bucket, or however you want to\n> count it). But generally nor do open-address schemes (and certainly our\n> other hash tables just use load factor to decide when to grow).\n\nAre we comparing the hashmap.[ch] with the hash.[ch] added in\n9027f53c (\"Do linear-time/space rename logic for exact renames\",\n2007-10-25)? I am a bit confused because Johannes calls it \"my\"\noriginal.\n\nUnless the real person in this discussion thread sending the\nmessages under Johannes's name is Linus, that is ;-). Or maybe the\n\"original\" being compared is something other than the series with\n6a364ced497 replaced with its hashmap.[ch]?\n\nIn any case, I do think your reading of the code is correct in that\nthe comparison between size and grow-at/shrink-at is done correctly\nwith the true load factor of the table, not how many buckets out of\nthe possible buckets are filled. \n\nOld one used to grow at 50% full and never shrunk it, but the\ncurrent one grows at 80% and shrinks at a bit below 40%; I agree\nwith Dscho's feeling (in part not quoted above) that 50% vs 80%\ndoesn't seem to have been backed by any numbers, but optimizing the\nload factor is outside the scope of this series, I would think.\n\n6a364ced (\"add a hashtable implementation that supports O(1)\nremoval\", 2013-11-14) credits less frequent resizing for gain of\ninsert performance, but my hunch is that the need for frequent\nresizing in the version before it primarily comes from the fact that\nthe table started empty (as opposed to having an initial size of 64,\nwhich is what the current implementation uses).\n"},{"id":"327656","messageId":"495c2fde-5592-e878-347f-dc7be7517724@jeffhostetler.com","threadId":"46590","inReplyTo":"xmqqo9qoaab4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] hashmap: add API to disable item counting when threaded","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-09-06T15:33:33Z","receivedAt":"2017-09-06T15:33:41Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/5/2017 9:24 PM, Junio C Hamano wrote:\n> Jeff Hostetler writes:\n> \n>> From: Jeff Hostetler \n>>\n> \n> I feel somewhat stupid to say this, especially after seeing many\n> people applaud this patch, but I do not seem to be able to even\n> build Git with this patch. I am getting:\n> \n> common-main.o: In function `hashmap_get_size':\n> /home/gitster/w/git.git/hashmap.h:260: multiple definition of `hashmap_get_size'\n> fast-import.o:/home/gitster/w/git.git/hashmap.h:260: first defined here\n> libgit.a(argv-array.o): In function `hashmap_get_size':\n> /home/gitster/w/git.git/hashmap.h:260: multiple definition of `hashmap_get_size'\n> fast-import.o:/home/gitster/w/git.git/hashmap.h:260: first defined here\n> libgit.a(attr.o): In function `hashmap_get_size':\n> ...\n> \n> and wonder if others are building with different options or something..\n\nThat's odd. I'm not seeing that. My Ubuntu VM is reporting:\n $ gcc --version\n gcc (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005\n\nI'll change it to static in the next version. Hopefully that will\ntake care of it.\n\nThanks,\nJeff\n\n"},{"id":"327657","messageId":"20170906154348.14287-1-git@jeffhostetler.com","threadId":"46590","inReplyTo":"20170830185922.10107-1-git@jeffhostetler.com","subject":"[PATCH v2] hashmap: address ThreadSanitizer concerns","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-09-06T15:43:47Z","receivedAt":"2017-09-06T15:44:08Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nVersion 2 addresses the comments and suggestions on version 1.\nIt removes the explicit disable/enable rehash and just relies\non the state of hashmap counting.\nIt changes the declaration of the hashmap_get_size() to be\nstatic to avoid issues seen on some compilers.\nIt uses BUG() rather than die() for an error condition.\nIt adds a comment describing why lazy-init needs to disable\ncouting.\nIt fixes line length problems.\nIt add details from TSan in the commit message.\n\nJeff Hostetler (1):\n hashmap: add API to disable item counting when threaded\n\n attr.c | 15 ++++++-----\n builtin/describe.c | 2 +-\n hashmap.c | 26 +++++++++++-------\n hashmap.h | 72 ++++++++++++++++++++++++++++++++++---------------\n name-hash.c | 10 +++++--\n t/helper/test-hashmap.c | 3 ++-\n 6 files changed, 88 insertions(+), 40 deletions(-)\n\n-- \n2.9.3\n\n"},{"id":"327658","messageId":"20170906154348.14287-2-git@jeffhostetler.com","threadId":"46590","inReplyTo":"20170906154348.14287-1-git@jeffhostetler.com","subject":"[PATCH v2] hashmap: add API to disable item counting when threaded","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-09-06T15:43:48Z","receivedAt":"2017-09-06T15:44:16Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nThis is to address concerns raised by ThreadSanitizer on the mailing list\nabout threaded unprotected R/W access to map.size with my previous \"disallow\nrehash\" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).\n\nSee:\nhttps://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/\n\nAdd API to hashmap to disable item counting and thus automatic rehashing.\nAlso include API to later re-enable them.\n\nWhen item counting is disabled, the map.size field is invalid. So to\nprevent accidents, the field has been renamed and an accessor function\nhashmap_get_size() has been added. All direct references to this\nfield have been been updated. And the name of the field changed\nto map.private_size to communicate this.\n\nHere is the relevant output from ThreadSanitizer showing the problem:\n\nWARNING: ThreadSanitizer: data race (pid=10554)\n Read of size 4 at 0x00000082d488 by thread T2 (mutexes: write M16):\n #0 hashmap_add hashmap.c:209\n #1 hash_dir_entry_with_parent_and_prefix name-hash.c:302\n #2 handle_range_dir name-hash.c:347\n #3 handle_range_1 name-hash.c:415\n #4 lazy_dir_thread_proc name-hash.c:471\n #5 \n\n Previous write of size 4 at 0x00000082d488 by thread T1 (mutexes: write M31):\n #0 hashmap_add hashmap.c:209\n #1 hash_dir_entry_with_parent_and_prefix name-hash.c:302\n #2 handle_range_dir name-hash.c:347\n #3 handle_range_1 name-hash.c:415\n #4 handle_range_dir name-hash.c:380\n #5 handle_range_1 name-hash.c:415\n #6 lazy_dir_thread_proc name-hash.c:471\n #7 \n\nMartin gives instructions for running TSan on test t3008 in this post:\nhttps://public-inbox.org/git/CAN0heSoJDL9pWELD6ciLTmWf-a=oyxe4EXXOmCKvsG5MSuzxsA@mail.gmail.com/\n\nSigned-off-by: Jeff Hostetler \n---\n attr.c | 15 ++++++-----\n builtin/describe.c | 2 +-\n hashmap.c | 26 +++++++++++-------\n hashmap.h | 72 ++++++++++++++++++++++++++++++++++---------------\n name-hash.c | 10 +++++--\n t/helper/test-hashmap.c | 3 ++-\n 6 files changed, 88 insertions(+), 40 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 56961f0..195aa6d 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -149,10 +149,12 @@ struct all_attrs_item {\n static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)\n {\n \tint i;\n+\tunsigned int size;\n \n \thashmap_lock(map);\n \n-\tif (map->map.size < check->all_attrs_nr)\n+\tsize = hashmap_get_size(&map->map);\n+\tif (size < check->all_attrs_nr)\n \t\tdie(\"BUG: interned attributes shouldn't be deleted\");\n \n \t/*\n@@ -161,13 +163,13 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)\n \t * field), reallocate the provided attr_check instance's all_attrs\n \t * field and fill each entry with its corresponding git_attr.\n \t */\n-\tif (map->map.size != check->all_attrs_nr) {\n+\tif (size != check->all_attrs_nr) {\n \t\tstruct attr_hash_entry *e;\n \t\tstruct hashmap_iter iter;\n \t\thashmap_iter_init(&map->map, &iter);\n \n-\t\tREALLOC_ARRAY(check->all_attrs, map->map.size);\n-\t\tcheck->all_attrs_nr = map->map.size;\n+\t\tREALLOC_ARRAY(check->all_attrs, size);\n+\t\tcheck->all_attrs_nr = size;\n \n \t\twhile ((e = hashmap_iter_next(&iter))) {\n \t\t\tconst struct git_attr *a = e->value;\n@@ -235,10 +237,11 @@ static const struct git_attr *git_attr_internal(const char *name, int namelen)\n \n \tif (!a) {\n \t\tFLEX_ALLOC_MEM(a, name, name, namelen);\n-\t\ta->attr_nr = g_attr_hashmap.map.size;\n+\t\ta->attr_nr = hashmap_get_size(&g_attr_hashmap.map);\n \n \t\tattr_hashmap_add(&g_attr_hashmap, a->name, namelen, a);\n-\t\tassert(a->attr_nr == (g_attr_hashmap.map.size - 1));\n+\t\tassert(a->attr_nr ==\n+\t\t (hashmap_get_size(&g_attr_hashmap.map) - 1));\n \t}\n \n \thashmap_unlock(&g_attr_hashmap);\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 89ea1cd..1e3cbc7 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -505,7 +505,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \n \thashmap_init(&names, (hashmap_cmp_fn) commit_name_cmp, NULL, 0);\n \tfor_each_rawref(get_name, NULL);\n-\tif (!names.size && !always)\n+\tif (!hashmap_get_size(&names) && !always)\n \t\tdie(_(\"No names found, cannot describe anything.\"));\n \n \tif (argc == 0) {\ndiff --git a/hashmap.c b/hashmap.c\nindex 9b6a128..d42f01f 100644\n--- a/hashmap.c\n+++ b/hashmap.c\n@@ -116,9 +116,6 @@ static void rehash(struct hashmap *map, unsigned int newsize)\n \tunsigned int i, oldsize = map->tablesize;\n \tstruct hashmap_entry **oldtable = map->table;\n \n-\tif (map->disallow_rehash)\n-\t\treturn;\n-\n \talloc_table(map, newsize);\n \tfor (i = 0; i < oldsize; i++) {\n \t\tstruct hashmap_entry *e = oldtable[i];\n@@ -166,6 +163,12 @@ void hashmap_init(struct hashmap *map, hashmap_cmp_fn equals_function,\n \twhile (initial_size > size)\n \t\tsize <<= HASHMAP_RESIZE_BITS;\n \talloc_table(map, size);\n+\n+\t/*\n+\t * Keep track of the number of items in the map and\n+\t * allow the map to automatically grow as necessary.\n+\t */\n+\tmap->do_count_items = 1;\n }\n \n void hashmap_free(struct hashmap *map, int free_entries)\n@@ -206,9 +209,11 @@ void hashmap_add(struct hashmap *map, void *entry)\n \tmap->table[b] = entry;\n \n \t/* fix size and rehash if appropriate */\n-\tmap->size++;\n-\tif (map->size > map->grow_at)\n-\t\trehash(map, map->tablesize << HASHMAP_RESIZE_BITS);\n+\tif (map->do_count_items) {\n+\t\tmap->private_size++;\n+\t\tif (map->private_size > map->grow_at)\n+\t\t\trehash(map, map->tablesize << HASHMAP_RESIZE_BITS);\n+\t}\n }\n \n void *hashmap_remove(struct hashmap *map, const void *key, const void *keydata)\n@@ -224,9 +229,12 @@ void *hashmap_remove(struct hashmap *map, const void *key, const void *keydata)\n \told->next = NULL;\n \n \t/* fix size and rehash if appropriate */\n-\tmap->size--;\n-\tif (map->size < map->shrink_at)\n-\t\trehash(map, map->tablesize >> HASHMAP_RESIZE_BITS);\n+\tif (map->do_count_items) {\n+\t\tmap->private_size--;\n+\t\tif (map->private_size < map->shrink_at)\n+\t\t\trehash(map, map->tablesize >> HASHMAP_RESIZE_BITS);\n+\t}\n+\n \treturn old;\n }\n \ndiff --git a/hashmap.h b/hashmap.h\nindex 7a8fa7f..7cb29a6 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -183,7 +183,7 @@ struct hashmap {\n \tconst void *cmpfn_data;\n \n \t/* total number of entries (0 means the hashmap is empty) */\n-\tunsigned int size;\n+\tunsigned int private_size; /* use hashmap_get_size() */\n \n \t/*\n \t * tablesize is the allocated size of the hash table. A non-0 value\n@@ -196,8 +196,7 @@ struct hashmap {\n \tunsigned int grow_at;\n \tunsigned int shrink_at;\n \n-\t/* See `hashmap_disallow_rehash`. */\n-\tunsigned disallow_rehash : 1;\n+\tunsigned int do_count_items : 1;\n };\n \n /* hashmap functions */\n@@ -253,6 +252,18 @@ static inline void hashmap_entry_init(void *entry, unsigned int hash)\n }\n \n /*\n+ * Return the number of items in the map.\n+ */\n+static inline unsigned int hashmap_get_size(struct hashmap *map)\n+{\n+\tif (map->do_count_items)\n+\t\treturn map->private_size;\n+\n+\tBUG(\"hashmap_get_size: size not set\");\n+\treturn 0;\n+}\n+\n+/*\n * Returns the hashmap entry for the specified key, or NULL if not found.\n *\n * `map` is the hashmap structure.\n@@ -345,24 +356,6 @@ extern void *hashmap_remove(struct hashmap *map, const void *key,\n int hashmap_bucket(const struct hashmap *map, unsigned int hash);\n \n /*\n- * Disallow/allow rehashing of the hashmap.\n- * This is useful if the caller knows that the hashmap needs multi-threaded\n- * access. The caller is still required to guard/lock searches and inserts\n- * in a manner appropriate to their usage. This simply prevents the table\n- * from being unexpectedly re-mapped.\n- *\n- * It is up to the caller to ensure that the hashmap is initialized to a\n- * reasonable size to prevent poor performance.\n- *\n- * A call to allow rehashing does not force a rehash; that might happen\n- * with the next insert or delete.\n- */\n-static inline void hashmap_disallow_rehash(struct hashmap *map, unsigned value)\n-{\n-\tmap->disallow_rehash = value;\n-}\n-\n-/*\n * Used to iterate over all entries of a hashmap. Note that it is\n * not safe to add or remove entries to the hashmap while\n * iterating.\n@@ -387,6 +380,43 @@ static inline void *hashmap_iter_first(struct hashmap *map,\n \treturn hashmap_iter_next(iter);\n }\n \n+/*\n+ * Disable item counting and automatic rehashing when adding/removing items.\n+ *\n+ * Normally, the hashmap keeps track of the number of items in the map\n+ * and uses it to dynamically resize it. This (both the counting and\n+ * the resizing) can cause problems when the map is being used by\n+ * threaded callers (because the hashmap code does not know about the\n+ * locking strategy used by the threaded callers and therefore, does\n+ * not know how to protect the \"private_size\" counter).\n+ */\n+static inline void hashmap_disable_item_counting(struct hashmap *map)\n+{\n+\tmap->do_count_items = 0;\n+}\n+\n+/*\n+ * Re-enable item couting when adding/removing items.\n+ * If counting is currently disabled, it will force count them.\n+ * It WILL NOT automatically rehash them.\n+ */\n+static inline void hashmap_enable_item_counting(struct hashmap *map)\n+{\n+\tvoid *item;\n+\tunsigned int n = 0;\n+\tstruct hashmap_iter iter;\n+\n+\tif (map->do_count_items)\n+\t\treturn;\n+\n+\thashmap_iter_init(map, &iter);\n+\twhile ((item = hashmap_iter_next(&iter)))\n+\t\tn++;\n+\n+\tmap->do_count_items = 1;\n+\tmap->private_size = n;\n+}\n+\n /* String interning */\n \n /*\ndiff --git a/name-hash.c b/name-hash.c\nindex 0e10f3e..3ac2e57 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -580,9 +580,15 @@ static void lazy_init_name_hash(struct index_state *istate)\n \t\t\tNULL, istate->cache_nr);\n \n \tif (lookup_lazy_params(istate)) {\n-\t\thashmap_disallow_rehash(&istate->dir_hash, 1);\n+\t\t/*\n+\t\t * Disable item counting and automatic rehashing because\n+\t\t * we do per-chain (mod n) locking rather than whole hashmap\n+\t\t * locking and we need to prevent the table-size from changing\n+\t\t * and bucket items from being redistributed.\n+\t\t */\n+\t\thashmap_disable_item_counting(&istate->dir_hash);\n \t\tthreaded_lazy_init_name_hash(istate);\n-\t\thashmap_disallow_rehash(&istate->dir_hash, 0);\n+\t\thashmap_enable_item_counting(&istate->dir_hash);\n \t} else {\n \t\tint nr;\n \t\tfor (nr = 0; nr < istate->cache_nr; nr++)\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 095d739..4956d1d 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -237,7 +237,8 @@ int cmd_main(int argc, const char **argv)\n \t\t} else if (!strcmp(\"size\", cmd)) {\n \n \t\t\t/* print table sizes */\n-\t\t\tprintf(\"%u %u\\n\", map.tablesize, map.size);\n+\t\t\tprintf(\"%u %u\\n\", map.tablesize,\n+\t\t\t hashmap_get_size(&map));\n \n \t\t} else if (!strcmp(\"intern\", cmd) && l1) {\n \n-- \n2.9.3\n\n"}]}