{"thread":{"id":"28760","subject":"general protection faults with \"git grep\" version 1.7.7.1","startedAt":"2011-10-24T20:11:53Z","lastAt":"2011-10-25T20:24:40Z","messageCount":15,"participants":["Markus Trippelsdorf","Richard W.M. Jones","Bernt Hansen","Jeff King","Thomas Rast","Jim Meyering"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"178245","messageId":"20111024201153.GA1647@x4.trippels.de","threadId":"28760","inReplyTo":null,"subject":"general protection faults with \"git grep\" version 1.7.7.1","fromName":"Markus Trippelsdorf","fromEmail":"markus@trippelsdorf.de","sentAt":"2011-10-24T20:11:53Z","receivedAt":"2011-10-24T20:11:53Z","isPatch":false,"sender":{"key":"markus@trippelsdorf.de","avatar":null},"body":"Suddenly I'm getting strange protection faults when I run \"git grep\" on\nthe gcc tree:\n\ngit[4245] general protection ip:7f291f01461f sp:7fff5618a8b0 error:0 in libc-2.14.90.so[7f291ef9a000+15d000]\n\n % gdb git\nGNU gdb (Gentoo 7.3.1 p1) 7.3.1\nCopyright (C) 2011 Free Software Foundation, Inc.\nLicense GPLv3+: GNU GPL version 3 or later \nThis is free software: you are free to change and redistribute it.\nThere is NO WARRANTY, to the extent permitted by law. Type \"show copying\"\nand \"show warranty\" for details.\nThis GDB was configured as \"x86_64-pc-linux-gnu\".\nFor bug reporting instructions, please see:\n...\nReading symbols from /usr/bin/git...done.\n(gdb) run grep composite_pointer_type\nStarting program: /usr/bin/git grep composite_pointer_type\nwarning: no loadable sections found in added symbol-file system-supplied DSO at 0x7ffff7ffa000\n[Thread debugging using libthread_db enabled]\n[New Thread 0x7ffff7859700 (LWP 18367)]\n[New Thread 0x7ffff7058700 (LWP 18368)]\n[New Thread 0x7ffff6857700 (LWP 18369)]\n[New Thread 0x7ffff6056700 (LWP 18370)]\n[New Thread 0x7ffff5855700 (LWP 18371)]\n[New Thread 0x7ffff5054700 (LWP 18372)]\n[New Thread 0x7ffff4853700 (LWP 18373)]\n[New Thread 0x7ffff4052700 (LWP 18374)]\n\nProgram received signal SIGSEGV, Segmentation fault.\n_int_malloc (av=0x7ffff7bbb600, bytes=21) at malloc.c:3463\n3463 while ((pp = catomic_compare_and_exchange_val_acq (fb, victim->fd, victim))\n(gdb) bt\n#0 _int_malloc (av=0x7ffff7bbb600, bytes=21) at malloc.c:3463\n#1 0x00007ffff78d7300 in __GI___libc_malloc (bytes=21) at malloc.c:2924\n#2 0x00007ffff78dc692 in __GI___strdup (s=0x7ffff2665760 \"gcc/ada/i-cexten.ads\") at strdup.c:43\n#3 0x00000000004d5069 in xstrdup (str=0x7ffff2665760 \"gcc/ada/i-cexten.ads\") at wrapper.c:23\n#4 0x000000000042c448 in grep_file_async (filename=0x7ffff2665760 \"gcc/ada/i-cexten.ads\", name=0x59fee0 \"gcc/ada/i-cexten.ads\", \n opt=) at builtin/grep.c:148\n#5 grep_file (opt=0x7fffffffbfc0, filename=0x7ffff2665760 \"gcc/ada/i-cexten.ads\") at builtin/grep.c:459\n#6 0x000000000042ddb0 in grep_cache (cached=0, pathspec=0x7fffffffbf70, opt=0x7fffffffbfc0) at builtin/grep.c:528\n#7 cmd_grep (argc=, argv=0x7ffff2665760, prefix=0x0) at builtin/grep.c:1062\n#8 0x00000000004045b0 in run_builtin (argv=0x7fffffffe110, argc=2, p=0x536ba0) at git.c:308\n#9 handle_internal_command (argc=2, argv=0x7fffffffe110) at git.c:466\n#10 0x00000000004047ac in run_argv (argv=0x7fffffffdfa0, argcp=0x7fffffffdfac) at git.c:512\n#11 main (argc=2, argv=0x7fffffffe110) at git.c:585\n(gdb) \n\nor:\n\n % git grep composite_pointer_type\n*** glibc detected *** git: double free or corruption (fasttop): 0x0000000001919800 ***\n======= Backtrace: =========\n/lib64/libc.so.6(+0x792b6)[0x7f6ad1d392b6]\ngit[0x42bebb]\n/lib64/libpthread.so.0(+0x7c9e)[0x7f6ad202ec9e]\n/lib64/libc.so.6(clone+0x6d)[0x7f6ad1d99b8d]\n======= Memory map: ========\n00400000-00536000 r-xp 00000000 08:12 2310166 /usr/bin/git\n00536000-0053d000 rw-p 00136000 08:12 2310166 /usr/bin/git\n0053d000-0058b000 rw-p 00000000 00:00 0\n01906000-01927000 rw-p 00000000 00:00 0 [heap]\n...\n\nAnd strange output:\n...\ngcc/cp/typeck.c: result_type = composite_pointer_type (type0, type1, op0, op1,\ngcc/cp/typeck.c: result_type = composite_pointer_type (type0, type1, op0, op1,\nerror: 'gcc/testsuite/ada/acats/tests/c5/c54a24a.ada': short read No such file or directory\nerror: 'gcc/testsuite/ada/acats/tests/c5/c54a13a.ada': short read No such file or directory\nerror: 'gcc/testsuite/ada/acats/tests/c6/c64104m.ada': short read No such file or directory\nerror: 'gcc/testsuite/ada/acats/tests/cd/cd7101f.dep': short read No such file or directory\nerror: 'gcc/testsuite/ada/acats/tests/ce/ce3904a.ada': short read No such file or directory\nerror: 'gcc/testsuite/g++.dg/abi/pr39188-3b.C': short read No such file or directory\nerror: '<90>Dz^A': short read Is a directory\n\nNote that all the above files actually exist.\nAll of this started with version v1.7.7.1, which I installed today. I\nnever had any problems with git before.\nAny ideas what might be going on?\n-- \nMarkus\n"},{"id":"178247","messageId":"20111024214949.GA5237@amd.home.annexia.org","threadId":"28760","inReplyTo":"20111024201153.GA1647@x4.trippels.de","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Richard W.M. Jones","fromEmail":"rjones@redhat.com","sentAt":"2011-10-24T21:49:49Z","receivedAt":"2011-10-24T21:49:49Z","isPatch":false,"sender":{"key":"rjones@redhat.com","avatar":"https://gravatar.com/avatar/6bc2ebbc861c9a4776b76939247a95d742d150e3feb2878a3b1ffd4d950329db?d=mp&s=160"},"body":"On Mon, Oct 24, 2011 at 10:11:53PM +0200, Markus Trippelsdorf wrote:\n> Suddenly I'm getting strange protection faults when I run \"git grep\" on\n> the gcc tree:\n\nJim Meyering and I are trying to chase what looks like a similar or\nidentical bug in git-grep. We've not got much further than gdb and\nvalgrind so far, but see:\n\nhttps://bugzilla.redhat.com/show_bug.cgi?id=747377\n\nIt's slightly suspicious that this bug only started to happen with the\nlatest glibc, but that could be coincidence, or could be just that\nglibc exposes a latent bug in git-grep.\n\nRich.\n\n-- \nRichard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones\nvirt-df lists disk usage of guests without needing to install any\nsoftware inside the virtual machine. Supports Linux and Windows.\nhttp://et.redhat.com/~rjones/virt-df/\n"},{"id":"178251","messageId":"20111024225836.GA1678@x4.trippels.de","threadId":"28760","inReplyTo":"20111024214949.GA5237@amd.home.annexia.org","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Markus Trippelsdorf","fromEmail":"markus@trippelsdorf.de","sentAt":"2011-10-24T22:58:36Z","receivedAt":"2011-10-24T22:58:36Z","isPatch":false,"sender":{"key":"markus@trippelsdorf.de","avatar":null},"body":"On 2011.10.24 at 22:49 +0100, Richard W.M. Jones wrote:\n> On Mon, Oct 24, 2011 at 10:11:53PM +0200, Markus Trippelsdorf wrote:\n> > Suddenly I'm getting strange protection faults when I run \"git grep\" on\n> > the gcc tree:\n> \n> Jim Meyering and I are trying to chase what looks like a similar or\n> identical bug in git-grep. We've not got much further than gdb and\n> valgrind so far, but see:\n> \n> https://bugzilla.redhat.com/show_bug.cgi?id=747377\n> \n> It's slightly suspicious that this bug only started to happen with the\n> latest glibc, but that could be coincidence, or could be just that\n> glibc exposes a latent bug in git-grep.\n\nThanks for the pointer.\n\nCompiling git with -O1 \"solves\" the problem for me. \nThis issue is independent of the exact git version being used (I tried\nthree different ones and always hit the problem).\nIt happens always on the _second_ run of \"git grep\" on my machine. The\nfirst run always succeeds. So this might be a cache related issue.\n\n-- \nMarkus\n"},{"id":"178255","messageId":"878voaym7k.fsf@norang.ca","threadId":"28760","inReplyTo":"20111024225836.GA1678@x4.trippels.de","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Bernt Hansen","fromEmail":"bernt@norang.ca","sentAt":"2011-10-25T00:00:15Z","receivedAt":"2011-10-25T00:00:15Z","isPatch":false,"sender":{"key":"bernt@norang.ca","avatar":null},"body":"Markus Trippelsdorf writes:\n\n> On 2011.10.24 at 22:49 +0100, Richard W.M. Jones wrote:\n>> On Mon, Oct 24, 2011 at 10:11:53PM +0200, Markus Trippelsdorf wrote:\n>> > Suddenly I'm getting strange protection faults when I run \"git grep\" on\n>> > the gcc tree:\n>> \n>> Jim Meyering and I are trying to chase what looks like a similar or\n>> identical bug in git-grep. We've not got much further than gdb and\n>> valgrind so far, but see:\n>> \n>> https://bugzilla.redhat.com/show_bug.cgi?id=747377\n>> \n>> It's slightly suspicious that this bug only started to happen with the\n>> latest glibc, but that could be coincidence, or could be just that\n>> glibc exposes a latent bug in git-grep.\n>\n> Thanks for the pointer.\n>\n> Compiling git with -O1 \"solves\" the problem for me. \n> This issue is independent of the exact git version being used (I tried\n> three different ones and always hit the problem).\n> It happens always on the _second_ run of \"git grep\" on my machine. The\n> first run always succeeds. So this might be a cache related issue.\n\nHi,\n\nI updated from an old commit 2883969 (Sync with maint, 2011-10-15)\nto origin/master 10b2a48 (Merge branch 'maint', 2011-10-23) today and\npromptly got segfaults on git status in my org-mode repository.\n\nGoing back to the old commit makes it work again.\n\nGit bisect identifies the following commit as the problem:\n\n[2548183badb98d62079beea62f9d2e1f47e99902] fix phantom untracked files when core.ignorecase is set\n\nI'm doing make && make install to my local home directory.\n\nOn the above commit I get this:\n\n--8<---------------cut here---------------start------------->8---\nbernt@gollum:~/git/org$ git status\n*** glibc detected *** git: free(): invalid next size (normal): 0x08ce1a88 ***\n======= Backtrace: =========\n/lib/i686/cmov/libc.so.6(+0x6b281)[0xb749d281]\n/lib/i686/cmov/libc.so.6(+0x6cad8)[0xb749ead8]\n/lib/i686/cmov/libc.so.6(cfree+0x6d)[0xb74a1bbd]\n/lib/i686/cmov/libc.so.6(+0x5c0a0)[0xb748e0a0]\n/lib/i686/cmov/libc.so.6(fopen64+0x2c)[0xb749067c]\ngit[0x80ba49c]\ngit[0x810d0ab]\ngit[0x80627af]\ngit[0x804b867]\ngit[0x804ba73]\n/lib/i686/cmov/libc.so.6(__libc_start_main+0xe6)[0xb7448c76]\ngit[0x804b141]\n======= Memory map: ========\n08048000-0814f000 r-xp 00000000 08:01 6555228 /home/bernt/git/bin/git\n0814f000-08154000 rw-p 00106000 08:01 6555228 /home/bernt/git/bin/git\n08154000-0819c000 rw-p 00000000 00:00 0 \n08cd7000-08cf8000 rw-p 00000000 00:00 0 [heap]\nb7300000-b7321000 rw-p 00000000 00:00 0 \nb7321000-b7400000 ---p 00000000 00:00 0 \nb740f000-b742c000 r-xp 00000000 08:01 4603917 /lib/libgcc_s.so.1\nb742c000-b742d000 rw-p 0001c000 08:01 4603917 /lib/libgcc_s.so.1\nb742d000-b742e000 rw-p 00000000 00:00 0 \nb742e000-b7430000 r-xp 00000000 08:01 4620339 /lib/i686/cmov/libdl-2.11.2.so\nb7430000-b7431000 r--p 00001000 08:01 4620339 /lib/i686/cmov/libdl-2.11.2.so\nb7431000-b7432000 rw-p 00002000 08:01 4620339 /lib/i686/cmov/libdl-2.11.2.so\nb7432000-b7572000 r-xp 00000000 08:01 4622760 /lib/i686/cmov/libc-2.11.2.so\nb7572000-b7574000 r--p 0013f000 08:01 4622760 /lib/i686/cmov/libc-2.11.2.so\nb7574000-b7575000 rw-p 00141000 08:01 4622760 /lib/i686/cmov/libc-2.11.2.so\nb7575000-b7578000 rw-p 00000000 00:00 0 \nb7578000-b758d000 r-xp 00000000 08:01 4620333 /lib/i686/cmov/libpthread-2.11.2.so\nb758d000-b758e000 r--p 00014000 08:01 4620333 /lib/i686/cmov/libpthread-2.11.2.so\nb758e000-b758f000 rw-p 00015000 08:01 4620333 /lib/i686/cmov/libpthread-2.11.2.so\nb758f000-b7592000 rw-p 00000000 00:00 0 \nb7592000-b76cf000 r-xp 00000000 08:01 794957 /usr/lib/i686/cmov/libcrypto.so.0.9.8\nb76cf000-b76e7000 rw-p 0013c000 08:01 794957 /usr/lib/i686/cmov/libcrypto.so.0.9.8\nb76e7000-b76ea000 rw-p 00000000 00:00 0 \nb76ea000-b76fd000 r-xp 00000000 08:01 286811 /usr/lib/libz.so.1.2.3.4\nb76fd000-b76fe000 rw-p 00013000 08:01 286811 /usr/lib/libz.so.1.2.3.4\nb771b000-b771d000 rw-p 00000000 00:00 0 \nb771d000-b771e000 r-xp 00000000 00:00 0 [vdso]\nb771e000-b7739000 r-xp 00000000 08:01 4604271 /lib/ld-2.11.2.so\nb7739000-b773a000 r--p 0001a000 08:01 4604271 /lib/ld-2.11.2.so\nb773a000-b773b000 rw-p 0001b000 08:01 4604271 /lib/ld-2.11.2.so\nbfa3d000-bfa52000 rw-p 00000000 00:00 0 [stack]\nAborted (core dumped)\n--8<---------------cut here---------------end--------------->8---\n\nLet me know if I can provide any more information.\n\nRegards\nBernt\n"},{"id":"178258","messageId":"20111025055310.GB1902@sigill.intra.peff.net","threadId":"28760","inReplyTo":"878voaym7k.fsf@norang.ca","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-25T05:53:11Z","receivedAt":"2011-10-25T05:53:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 24, 2011 at 08:00:15PM -0400, Bernt Hansen wrote:\n\n> I updated from an old commit 2883969 (Sync with maint, 2011-10-15)\n> to origin/master 10b2a48 (Merge branch 'maint', 2011-10-23) today and\n> promptly got segfaults on git status in my org-mode repository.\n> \n> Going back to the old commit makes it work again.\n> \n> Git bisect identifies the following commit as the problem:\n> \n> [2548183badb98d62079beea62f9d2e1f47e99902] fix phantom untracked files when core.ignorecase is set\n\nI think this is a separate problem. See this thread and patch:\n\n http://thread.gmane.org/gmane.comp.version-control.git/184094/focus=184148\n\n-Peff\n"},{"id":"178262","messageId":"87zkgpxr4s.fsf@norang.ca","threadId":"28760","inReplyTo":"20111025055310.GB1902@sigill.intra.peff.net","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Bernt Hansen","fromEmail":"bernt@norang.ca","sentAt":"2011-10-25T11:11:31Z","receivedAt":"2011-10-25T11:11:31Z","isPatch":false,"sender":{"key":"bernt@norang.ca","avatar":null},"body":"Jeff King writes:\n\n> On Mon, Oct 24, 2011 at 08:00:15PM -0400, Bernt Hansen wrote:\n>\n>> I updated from an old commit 2883969 (Sync with maint, 2011-10-15)\n>> to origin/master 10b2a48 (Merge branch 'maint', 2011-10-23) today and\n>> promptly got segfaults on git status in my org-mode repository.\n>> \n>> Going back to the old commit makes it work again.\n>> \n>> Git bisect identifies the following commit as the problem:\n>> \n>> [2548183badb98d62079beea62f9d2e1f47e99902] fix phantom untracked files when core.ignorecase is set\n>\n> I think this is a separate problem. See this thread and patch:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/184094/focus=184148\n\nThanks,\n\nI'll look at that thread.\n\n-Bernt\n"},{"id":"178265","messageId":"201110251550.22248.trast@student.ethz.ch","threadId":"28760","inReplyTo":"20111024214949.GA5237@amd.home.annexia.org","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-10-25T13:50:21Z","receivedAt":"2011-10-25T13:50:21Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"[Shawn, Peff, Nicolas: maybe you can say something on the\n(non)raciness of xmalloc() in parallel with read_sha1_file(). See the\nlast paragraph below.]\n\nRichard W.M. Jones wrote:\n> On Mon, Oct 24, 2011 at 10:11:53PM +0200, Markus Trippelsdorf wrote:\n> > Suddenly I'm getting strange protection faults when I run \"git grep\" on\n> > the gcc tree:\n> \n> Jim Meyering and I are trying to chase what looks like a similar or\n> identical bug in git-grep. We've not got much further than gdb and\n> valgrind so far, but see:\n> \n> https://bugzilla.redhat.com/show_bug.cgi?id=747377\n> \n> It's slightly suspicious that this bug only started to happen with the\n> latest glibc, but that could be coincidence, or could be just that\n> glibc exposes a latent bug in git-grep.\n\nI'm tempted to write this off as a GCC bug. If that's ok for you,\nI'll leave further investigation and communication with the GCC folks\nto you.\n\nMy findings are as follows:\n\nIt's easy to reproduce the behavior described in the above bug report,\nusing an F16 beta install in a VM. I gave the VM two cores, but\ndidn't test what happens with only one. By \"easy\" I mean I didn't\nhave to do any fiddling and it crashes at least one out of two times.\n\nI looked at how git builds grep.o by saying\n\n rm builtin/grep.o; make V=1\n\nI then modified this to give me the assembly output from the compiler\n\n gcc -S -s builtin/grep.o -c -MF builtin/.depend/grep.o.d -MMD -MP -g -O2 -Wall -I. -DHAVE_PATHS_H -DSHA1_HEADER='' -DNO_STRLCPY -DNO_MKSTEMPS builtin/grep.c\n\nand looked at the result. To interpret the output, I would like to\nremind you of the following snippets:\n\n #define grep_lock() pthread_mutex_lock(&grep_mutex)\n #define grep_unlock() pthread_mutex_unlock(&grep_mutex)\n...\n static struct work_item *get_work(void)\n {\n struct work_item *ret;\n\n grep_lock();\n while (todo_start == todo_end && !all_work_added) {\n pthread_cond_wait(&cond_add, &grep_mutex);\n }\n\n...\n }\n...\n static void *run(void *arg)\n {\n int hit = 0;\n struct grep_opt *opt = arg;\n\n while (1) {\n struct work_item *w = get_work();\n...\n }\n...\n }\n\nGetting back to assembly, near the beginning of run() I see (labels\nand .p2align snipped):\n\n\t.loc 1 162 0\n\tmovl\ttodo_end(%rip), %ebx\n\t.loc 1 125 0\n\tmovl\t$grep_mutex, %edi\n\tcall\tpthread_mutex_lock\n\t.loc 1 126 0\n\tmovl\ttodo_start(%rip), %eax\n\tcmpl\t%ebx, %eax\n\nI should say that I don't really know much about assembly, in\nparticular not enough to write two correct lines of it. But I can't\nhelp noticing that it moved the load of todo_end *out of* the section\nwhere grep_mutex is locked. And the comment near the top of the file\ndoes say that the whole todo_* family is supposed to be protected by\nthat mutex. What's extra odd is that the .loc seems to indicate that\nthe moved load comes from work_done() instead of get_work(), which is\nan entirely separate locked section!\n\nUn-inlining the get_work helper using __attribute__((noinline)) makes\nthe assembly\n\n\tmovl\t$grep_mutex, %edi\n\tcall\tpthread_mutex_lock\n\t.loc 1 127 0\n\tmovl\ttodo_start(%rip), %eax\n\tcmpl\ttodo_end(%rip), %eax\n\tje\t.L15\n\ninstead; i.e., the load is now after the lock. (Note that line\nnumbers were wiggled by inserting an __attribute__ line.) The\nbeginning of run() turns into exactly the same code if I instead\nprohibit inlining of work_done().\n\nSo AFAICS, we're just unlucky to hit a GCC optimizer bug that voids\nall guarantees given on locks.\n\n\nThat being said, I'm not entirely convinced that the code in\nbuiltin/grep.c works in the face of memory pressure. It guards\nagainst concurrent access to read_sha1_file() with the\nread_sha1_mutex, but any call to xmalloc() outside of that mutex can\nstill potentially invoke the try_to_free_routine. Maybe one of the\npack experts can say whether this is safe. (However, I implemented\nlocking around try_to_free_routine as a quick hack and it did not fix\nthe issue discussed in the bug report.)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"178269","messageId":"87y5w9ayoa.fsf@rho.meyering.net","threadId":"28760","inReplyTo":"201110251550.22248.trast@student.ethz.ch","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-10-25T15:17:09Z","receivedAt":"2011-10-25T15:17:09Z","isPatch":false,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Thomas Rast wrote:\n> [Shawn, Peff, Nicolas: maybe you can say something on the\n> (non)raciness of xmalloc() in parallel with read_sha1_file(). See the\n> last paragraph below.]\n>\n> Richard W.M. Jones wrote:\n>> On Mon, Oct 24, 2011 at 10:11:53PM +0200, Markus Trippelsdorf wrote:\n>> > Suddenly I'm getting strange protection faults when I run \"git grep\" on\n>> > the gcc tree:\n>>\n>> Jim Meyering and I are trying to chase what looks like a similar or\n>> identical bug in git-grep. We've not got much further than gdb and\n>> valgrind so far, but see:\n>>\n>> https://bugzilla.redhat.com/show_bug.cgi?id=747377\n>>\n>> It's slightly suspicious that this bug only started to happen with the\n>> latest glibc, but that could be coincidence, or could be just that\n>> glibc exposes a latent bug in git-grep.\n>\n> I'm tempted to write this off as a GCC bug. If that's ok for you,\n> I'll leave further investigation and communication with the GCC folks\n> to you.\n>\n> My findings are as follows:\n>\n> It's easy to reproduce the behavior described in the above bug report,\n> using an F16 beta install in a VM. I gave the VM two cores, but\n> didn't test what happens with only one. By \"easy\" I mean I didn't\n> have to do any fiddling and it crashes at least one out of two times.\n>\n> I looked at how git builds grep.o by saying\n>\n> rm builtin/grep.o; make V=1\n>\n> I then modified this to give me the assembly output from the compiler\n>\n> gcc -S -s builtin/grep.o -c -MF builtin/.depend/grep.o.d -MMD -MP -g -O2 -Wall -I. -DHAVE_PATHS_H -DSHA1_HEADER='' -DNO_STRLCPY -DNO_MKSTEMPS builtin/grep.c\n...\n> So AFAICS, we're just unlucky to hit a GCC optimizer bug that voids\n> all guarantees given on locks.\n\nThanks for the investigation.\nActually, isn't gcc -O2's code-motion justified?\nWhile we *know* that those globals may be modified asynchronously,\nbuiltin/grep.c forgot to tell gcc about that.\nOnce you do that (via \"volatile\"), gcc knows not to move things.\n\nThis patch solved the problem for me:\n\n>From 8521b8033b8ecbff2e459f9e0070beb712b9b73d Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Tue, 25 Oct 2011 17:07:05 +0200\nSubject: [PATCH] declare grep's thread-related global scalars to be\n \"volatile\"\n\nThis avoids heap corruption problems that would otherwise\narise when gcc -O2 moves code out of critical sections.\nFor details, see http://bugzilla.redhat.com/747377 and\nhttp://thread.gmane.org/gmane.comp.version-control.git/184184/focus=184205\n\nSigned-off-by: Jim Meyering \n---\n builtin/grep.c | 12 ++++++------\n 1 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 7d0779f..38f92de 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -64,14 +64,14 @@ struct work_item {\n */\n #define TODO_SIZE 128\n static struct work_item todo[TODO_SIZE];\n-static int todo_start;\n-static int todo_end;\n-static int todo_done;\n+static volatile int todo_start;\n+static volatile int todo_end;\n+static volatile int todo_done;\n\n-/* Has all work items been added? */\n-static int all_work_added;\n+/* Have all work items been added? */\n+static volatile int all_work_added;\n\n-/* This lock protects all the variables above. */\n+/* This lock protects all of the above variables. */\n static pthread_mutex_t grep_mutex;\n\n /* Used to serialize calls to read_sha1_file. */\n--\n1.7.7.419.g87009\n"},{"id":"178271","messageId":"20111025153212.GC1678@x4.trippels.de","threadId":"28760","inReplyTo":"87y5w9ayoa.fsf@rho.meyering.net","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Markus Trippelsdorf","fromEmail":"markus@trippelsdorf.de","sentAt":"2011-10-25T15:32:12Z","receivedAt":"2011-10-25T15:32:12Z","isPatch":false,"sender":{"key":"markus@trippelsdorf.de","avatar":null},"body":"On 2011.10.25 at 17:17 +0200, Jim Meyering wrote:\n> Thomas Rast wrote:\n> > [Shawn, Peff, Nicolas: maybe you can say something on the\n> > (non)raciness of xmalloc() in parallel with read_sha1_file(). See the\n> > last paragraph below.]\n> >\n> > Richard W.M. Jones wrote:\n> >> On Mon, Oct 24, 2011 at 10:11:53PM +0200, Markus Trippelsdorf wrote:\n> >> > Suddenly I'm getting strange protection faults when I run \"git grep\" on\n> >> > the gcc tree:\n> >>\n> >> Jim Meyering and I are trying to chase what looks like a similar or\n> >> identical bug in git-grep. We've not got much further than gdb and\n> >> valgrind so far, but see:\n> >>\n> >> https://bugzilla.redhat.com/show_bug.cgi?id=747377\n> >>\n> >> It's slightly suspicious that this bug only started to happen with the\n> >> latest glibc, but that could be coincidence, or could be just that\n> >> glibc exposes a latent bug in git-grep.\n> >\n> > I'm tempted to write this off as a GCC bug. If that's ok for you,\n> > I'll leave further investigation and communication with the GCC folks\n> > to you.\n> >\n> > My findings are as follows:\n> >\n> > It's easy to reproduce the behavior described in the above bug report,\n> > using an F16 beta install in a VM. I gave the VM two cores, but\n> > didn't test what happens with only one. By \"easy\" I mean I didn't\n> > have to do any fiddling and it crashes at least one out of two times.\n> >\n> > I looked at how git builds grep.o by saying\n> >\n> > rm builtin/grep.o; make V=1\n> >\n> > I then modified this to give me the assembly output from the compiler\n> >\n> > gcc -S -s builtin/grep.o -c -MF builtin/.depend/grep.o.d -MMD -MP -g -O2 -Wall -I. -DHAVE_PATHS_H -DSHA1_HEADER='' -DNO_STRLCPY -DNO_MKSTEMPS builtin/grep.c\n> ...\n> > So AFAICS, we're just unlucky to hit a GCC optimizer bug that voids\n> > all guarantees given on locks.\n> \n> Thanks for the investigation.\n> Actually, isn't gcc -O2's code-motion justified?\n> While we *know* that those globals may be modified asynchronously,\n> builtin/grep.c forgot to tell gcc about that.\n> Once you do that (via \"volatile\"), gcc knows not to move things.\n> \n> This patch solved the problem for me:\n\nYes. This fixes the issue here also.\n\n(BTW the only recent pthread related change in glibc was the removal\nof the gettimeofday vsyscall)\n\n-- \nMarkus\n"},{"id":"178272","messageId":"20111025153720.GA6640@sigill.intra.peff.net","threadId":"28760","inReplyTo":"201110251550.22248.trast@student.ethz.ch","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-25T15:37:20Z","receivedAt":"2011-10-25T15:37:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 25, 2011 at 03:50:21PM +0200, Thomas Rast wrote:\n\n> That being said, I'm not entirely convinced that the code in\n> builtin/grep.c works in the face of memory pressure. It guards\n> against concurrent access to read_sha1_file() with the\n> read_sha1_mutex, but any call to xmalloc() outside of that mutex can\n> still potentially invoke the try_to_free_routine. Maybe one of the\n> pack experts can say whether this is safe. (However, I implemented\n> locking around try_to_free_routine as a quick hack and it did not fix\n> the issue discussed in the bug report.)\n\nYes, I think it needs to set try_to_free_routine. See this thread:\n\n http://thread.gmane.org/gmane.comp.version-control.git/180446\n\nwhich discusses a possible subtlety with doing so.\n\n-Peff\n"},{"id":"178275","messageId":"201110251800.28054.trast@student.ethz.ch","threadId":"28760","inReplyTo":"87y5w9ayoa.fsf@rho.meyering.net","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-10-25T16:00:27Z","receivedAt":"2011-10-25T16:00:27Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jim Meyering wrote:\n> Thomas Rast wrote:\n> > [GCC moves access to a file-static variable across pthread_mutex_lock()]\n> \n> Thanks for the investigation.\n> Actually, isn't gcc -O2's code-motion justified?\n> While we *know* that those globals may be modified asynchronously,\n> builtin/grep.c forgot to tell gcc about that.\n\nI'm somewhat unwilling to believe that:\n\n* \"volatile\" enforces three unrelated things, see e.g. [1].\n\n* Removing \"static\" would do the same as it prevents the compiler from\n proving at compile-time that pthread_mutex_lock() cannot affect the\n variable in question.\n\n If this is correct, it also means that all code in all pthreads\n tutorials I can find works merely by the accident of not declaring\n their variables \"static\".\n\n Furthermore, a future smarter compiler with better link-time\n optimization might again prove the same and eliminate the\n \"superfluous\" load.\n\nHowever, as a result of the discussion I now have a shorter testcase:\n\n #include \n\n int y;\n static int x;\n\n pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;\n\n void test ()\n {\n y = x;\n pthread_mutex_lock(&m);\n x = x + 1;\n pthread_mutex_unlock(&m);\n }\n\nGCC 4.6.1 on F16 again assumes 'x' was not modified across the lock.\nI also tested GCC 4.5.1 and 4.4.5, which instead issue a direct\nadd-to-memory instruction\n\n addl $1, x(%rip)\n\nin the locked part.\n\nIn the event that you and GCC 4.6.1 are right, I still vote for\nremoving 'static' instead of adding 'volatile' so as to allow basic\noptimizations.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"178276","messageId":"201110251807.53893.trast@student.ethz.ch","threadId":"28760","inReplyTo":"201110251800.28054.trast@student.ethz.ch","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-10-25T16:07:53Z","receivedAt":"2011-10-25T16:07:53Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast wrote:\n> Jim Meyering wrote:\n> > Thomas Rast wrote:\n> > > [GCC moves access to a file-static variable across pthread_mutex_lock()]\n> > \n> > Thanks for the investigation.\n> > Actually, isn't gcc -O2's code-motion justified?\n> > While we *know* that those globals may be modified asynchronously,\n> > builtin/grep.c forgot to tell gcc about that.\n> \n> I'm somewhat unwilling to believe that:\n> \n> * \"volatile\" enforces three unrelated things, see e.g. [1].\n\nArgh, forgot my reference:\n[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2016.html\n section \"Existing portable uses\"\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"178279","messageId":"87sjmhauyo.fsf@rho.meyering.net","threadId":"28760","inReplyTo":"201110251800.28054.trast@student.ethz.ch","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-10-25T16:37:19Z","receivedAt":"2011-10-25T16:37:19Z","isPatch":false,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Thomas Rast wrote:\n> Jim Meyering wrote:\n>> Thomas Rast wrote:\n>> > [GCC moves access to a file-static variable across pthread_mutex_lock()]\n>>\n>> Thanks for the investigation.\n>> Actually, isn't gcc -O2's code-motion justified?\n>> While we *know* that those globals may be modified asynchronously,\n>> builtin/grep.c forgot to tell gcc about that.\n>\n> I'm somewhat unwilling to believe that:\n\nYou're right to be skeptical.\nI should have stuck with \"using volatile works around the problem for me\".\nThe real problem seems to be in glibc, with its addition of\nthe \"leaf\" attribute to those synchronization primitives:\n\n http://bugzilla.redhat.com/747377#c22\n"},{"id":"178283","messageId":"201110251854.43369.trast@student.ethz.ch","threadId":"28760","inReplyTo":"87sjmhauyo.fsf@rho.meyering.net","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-10-25T16:54:43Z","receivedAt":"2011-10-25T16:54:43Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jim Meyering wrote:\n> Thomas Rast wrote:\n> > Jim Meyering wrote:\n> >> Thomas Rast wrote:\n> >> > [GCC moves access to a file-static variable across pthread_mutex_lock()]\n> >>\n> >> Thanks for the investigation.\n> >> Actually, isn't gcc -O2's code-motion justified?\n> >> While we *know* that those globals may be modified asynchronously,\n> >> builtin/grep.c forgot to tell gcc about that.\n> >\n> > I'm somewhat unwilling to believe that:\n> \n> You're right to be skeptical.\n> I should have stuck with \"using volatile works around the problem for me\".\n> The real problem seems to be in glibc, with its addition of\n> the \"leaf\" attribute to those synchronization primitives:\n> \n> http://bugzilla.redhat.com/747377#c22\n\nAha. Glad you found it :-)\n\nMeanwhile I read\n\n http://www.hpl.hp.com/techreports/2004/HPL-2004-209.html\n\nwhich discusses a similar issue in section 4.3, but is very\ninteresting on its own. It's funny how it says\n\n We know of at least three optimizing compilers (two of them\n production compilers) that performed this transformation at some\n point during their lifetime; usually at least partially reversing\n the decision when the implications on multi-threaded code became\n known.\n\nI guess that would be four now if it was literally the same problem.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"178295","messageId":"87zkgoakfr.fsf@rho.meyering.net","threadId":"28760","inReplyTo":"201110251854.43369.trast@student.ethz.ch","subject":"Re: general protection faults with \"git grep\" version 1.7.7.1","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-10-25T20:24:40Z","receivedAt":"2011-10-25T20:24:40Z","isPatch":false,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Thomas Rast wrote:\n...\n>> The real problem seems to be in glibc, with its addition of\n>> the \"leaf\" attribute to those synchronization primitives:\n>>\n>> http://bugzilla.redhat.com/747377#c22\n>\n> Aha. Glad you found it :-)\n>\n> Meanwhile I read\n>\n> http://www.hpl.hp.com/techreports/2004/HPL-2004-209.html\n>\n> which discusses a similar issue in section 4.3, but is very\n> interesting on its own. It's funny how it says\n>\n> We know of at least three optimizing compilers (two of them\n> production compilers) that performed this transformation at some\n> point during their lifetime; usually at least partially reversing\n> the decision when the implications on multi-threaded code became\n> known.\n>\n> I guess that would be four now if it was literally the same problem.\n\nYep. For those not following the BZ comments at the about URL,\nPOSIX is quite clear. Quoting from\nhttp://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11:\n\n The following functions synchronize memory with respect to other threads:\n\n fork\n pthread_barrier_wait\n pthread_cond_broadcast\n pthread_cond_signal\n pthread_cond_timedwait\n pthread_cond_wait\n pthread_create\n pthread_join\n pthread_mutex_lock\n pthread_mutex_timedlock\n pthread_mutex_trylock\n pthread_mutex_unlock\n pthread_spin_lock\n pthread_spin_trylock\n pthread_spin_unlock\n pthread_rwlock_rdlock\n pthread_rwlock_timedrdlock\n pthread_rwlock_timedwrlock\n pthread_rwlock_tryrdlock\n pthread_rwlock_trywrlock\n pthread_rwlock_unlock\n pthread_rwlock_wrlock\n sem_post\n sem_timedwait\n sem_trywait\n sem_wait\n semctl\n semop\n wait\n waitpid\n\nglibc's addition of the leaf attribute to any of those\nappears to make gcc violate that.\n"}]}