{"thread":{"id":"1800","subject":"git-diff-tree rename detection bug","startedAt":"2005-09-14T16:47:56Z","lastAt":"2005-09-20T10:50:41Z","messageCount":23,"participants":["Wayne Scott","Junio C Hamano","Linus Torvalds","Paul Mackerras","H. Peter Anvin","Josef Weidendorfer","Nicolas Pitre","Matthias Urlichs"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"8536","messageId":"59a6e583050914094777c4fe96@mail.gmail.com","threadId":"1800","inReplyTo":null,"subject":"git-diff-tree rename detection bug","fromName":"Wayne Scott","fromEmail":"wsc9tt@gmail.com","sentAt":"2005-09-14T16:47:56Z","receivedAt":"2005-09-14T16:47:56Z","isPatch":false,"sender":{"key":"wsc9tt@gmail.com","avatar":"https://gravatar.com/avatar/2418bf5fa7f1625a2b9dd049db4ab56110f561610421d6f2559f7c018ce53eb3?d=mp&s=160"},"body":"Look at the diffs between ad6571a78ac74e9fa27e581834709067dba459af and\nit's parent with and without rename detection enabled. (In linux-2.6\ngit tree)\n\n(formated for narrow screens)\n$ REV=ad6571a78ac74e9fa27e581834709067dba459af\n$ git-diff-tree -r $REV^1 $REV | grep termios.h\n:000000 100644 0000000000000000000000000000000000000000\n 237533bb0e9f1a3e640c4906d8b350deafd315b9 A include/asm-powerpc/termios.h\n:100644 000000 97c6287a6cbaa5903ee1a5934a5553e9e485d8e7\n 0000000000000000000000000000000000000000 D include/asm-ppc/termios.h\n:100644 000000 02c3d283aa62bc1b4d7c5d1b22ce03ee4b8771eb\n 0000000000000000000000000000000000000000 D include/asm-ppc64/termios.h\n\n$ git-diff-tree -r -M $REV^1 $REV | grep termios.h\n:000000 100644 0000000000000000000000000000000000000000\n 237533bb0e9f1a3e640c4906d8b350deafd315b9 A include/asm-powerpc/termios.h\n:100644 000000 97c6287a6cbaa5903ee1a5934a5553e9e485d8e7\n 0000000000000000000000000000000000000000 D include/asm-ppc/termios.h\n\nNotice how the the fact that include/asm-ppc64/termios.h is deleted gets lost?\nLooks broken to me.\n\n-Wayne\n"},{"id":"8543","messageId":"7vwtljjzc3.fsf@assigned-by-dhcp.cox.net","threadId":"1800","inReplyTo":"59a6e583050914094777c4fe96@mail.gmail.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-14T18:09:32Z","receivedAt":"2005-09-14T18:09:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wayne Scott writes:\n\n> Look at the diffs between ad6571a78ac74e9fa27e581834709067dba459af and\n> it's parent with and without rename detection enabled. (In linux-2.6\n> git tree)\n\n> Notice how the the fact that include/asm-ppc64/termios.h is\n> deleted gets lost? Looks broken to me.\n\nI suspect that what is deleted is not asm-ppc64/termios.h but\nasm-ppc/termios.h. The below is an output without grep which\nseems to confuse things.\n\n$ git-diff-tree -r -M ad6571a78ac74e9fa27e581834709067dba459af |\n sed -ne 's/^.* \\([AMCRDU]\\)/\\1/p'\nR092 include/asm-ppc64/mman.h include/asm-powerpc/mman.h\nR097 include/asm-ppc64/termbits.h include/asm-powerpc/termbits.h\nR098 include/asm-ppc64/termios.h include/asm-powerpc/termios.h\nD include/asm-ppc/mman.h\nD include/asm-ppc/termbits.h\nD include/asm-ppc/termios.h\n\n$ git-diff-tree -r ad6571a78ac74e9fa27e581834709067dba459af |\n sed -ne 's/^.* \\([AMCRDU]\\)/\\1/p'\nA include/asm-powerpc/mman.h\nA include/asm-powerpc/termbits.h\nA include/asm-powerpc/termios.h\nD include/asm-ppc/mman.h\nD include/asm-ppc/termbits.h\nD include/asm-ppc/termios.h\nD include/asm-ppc64/mman.h\nD include/asm-ppc64/termbits.h\nD include/asm-ppc64/termios.h\n\nThe first 3 A and last 3 D are accounted for in the -M output as\nrenames from asm-ppc64 to asm-powerpc. Middle 3 D from asm-ppc\nare shown in the -M output. So I do not think we are losing\nanything. Am I missing something?\n"},{"id":"8547","messageId":"59a6e583050914114054b1564d@mail.gmail.com","threadId":"1800","inReplyTo":"7vwtljjzc3.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-diff-tree rename detection bug","fromName":"Wayne Scott","fromEmail":"wsc9tt@gmail.com","sentAt":"2005-09-14T18:40:27Z","receivedAt":"2005-09-14T18:40:27Z","isPatch":false,"sender":{"key":"wsc9tt@gmail.com","avatar":"https://gravatar.com/avatar/2418bf5fa7f1625a2b9dd049db4ab56110f561610421d6f2559f7c018ce53eb3?d=mp&s=160"},"body":"On 9/14/05, Junio C Hamano wrote:\n> Wayne Scott writes:\n> \n> > Look at the diffs between ad6571a78ac74e9fa27e581834709067dba459af and\n> > it's parent with and without rename detection enabled. (In linux-2.6\n> > git tree)\n> \n> > Notice how the the fact that include/asm-ppc64/termios.h is\n> > deleted gets lost? Looks broken to me.\n> \n> I suspect that what is deleted is not asm-ppc64/termios.h but\n> asm-ppc/termios.h. The below is an output without grep which\n> seems to confuse things.\n> \n> $ git-diff-tree -r -M ad6571a78ac74e9fa27e581834709067dba459af |\n> sed -ne 's/^.* \\([AMCRDU]\\)/\\1/p'\n> R092 include/asm-ppc64/mman.h include/asm-powerpc/mman.h\n> R097 include/asm-ppc64/termbits.h include/asm-powerpc/termbits.h\n> R098 include/asm-ppc64/termios.h include/asm-powerpc/termios.h\n> D include/asm-ppc/mman.h\n> D include/asm-ppc/termbits.h\n> D include/asm-ppc/termios.h\n> \n> $ git-diff-tree -r ad6571a78ac74e9fa27e581834709067dba459af |\n> sed -ne 's/^.* \\([AMCRDU]\\)/\\1/p'\n> A include/asm-powerpc/mman.h\n> A include/asm-powerpc/termbits.h\n> A include/asm-powerpc/termios.h\n> D include/asm-ppc/mman.h\n> D include/asm-ppc/termbits.h\n> D include/asm-ppc/termios.h\n> D include/asm-ppc64/mman.h\n> D include/asm-ppc64/termbits.h\n> D include/asm-ppc64/termios.h\n> \n> The first 3 A and last 3 D are accounted for in the -M output as\n> renames from asm-ppc64 to asm-powerpc. Middle 3 D from asm-ppc\n> are shown in the -M output. So I do not think we are losing\n> anything. Am I missing something?\n\n\nOdd. I get the same answer on my x86 box:\n$ git-diff-tree -r -M ad6571a78ac74e9fa27e581834709067dba459af | sed\n-ne 's/^.* \\([AMCRDU]\\)/\\1/p'\nR092 include/asm-ppc64/mman.h include/asm-powerpc/mman.h\nR097 include/asm-ppc64/termbits.h include/asm-powerpc/termbits.h\nR098 include/asm-ppc64/termios.h include/asm-powerpc/termios.h\nD include/asm-ppc/mman.h\nD include/asm-ppc/termbits.h\nD include/asm-ppc/termios.h\n\nBut here is the output on my quad xeon running in 64-bit mode: (fedora core 2)\n\n$ git-diff-tree -r -M ad6571a78ac74e9fa27e581834709067dba459af | sed\n-ne 's/^.* \\([AMCRDU]\\)/\\1/p'\nR092 include/asm-ppc64/mman.h include/asm-powerpc/mman.h\nR097 include/asm-ppc64/termbits.h include/asm-powerpc/termbits.h\nA include/asm-powerpc/termios.h\nD include/asm-ppc/mman.h\nD include/asm-ppc/termbits.h\nD include/asm-ppc/termios.h\n\nThis is the same version of git both rebuilt just for this test.\n\nHowever, I noticed a whole collection of errors from valgrind when I\nrun this command line:\n\n==13457== Invalid read of size 4\n==13457== at 0x805402C: locate_rename_dst (diffcore-rename.c:28)\n==13457== by 0x805464B: diffcore_rename (diffcore-rename.c:356)\n==13457== by 0x805249D: diffcore_std (diff.c:1093)\n==13457== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13457== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13457== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13457== by 0x804A884: main (diff-tree.c:551)\n==13457== Address 0x1BBD781C is 20 bytes inside a block of size 71 free'd\n==13457== at 0x1B9003C3: free (vg_replace_malloc.c:235)\n==13457== by 0x805205B: diff_free_filepair (diff.c:775)\n==13457== by 0x805422A: diffcore_rename (diffcore-rename.c:415)\n==13457== by 0x805249D: diffcore_std (diff.c:1093)\n==13457== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13457== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13457== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13457== by 0x804A884: main (diff-tree.c:551)\n==13457== \n==13457== Invalid read of size 1\n==13457== at 0x1B90140D: strcmp (mac_replace_strmem.c:332)\n==13457== by 0x8054038: locate_rename_dst (diffcore-rename.c:28)\n==13457== by 0x805464B: diffcore_rename (diffcore-rename.c:356)\n==13457== by 0x805249D: diffcore_std (diff.c:1093)\n==13457== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13457== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13457== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13457== by 0x804A884: main (diff-tree.c:551)\n==13457== Address 0x1BBD7830 is 40 bytes inside a block of size 71 free'd\n==13457== at 0x1B9003C3: free (vg_replace_malloc.c:235)\n==13457== by 0x805205B: diff_free_filepair (diff.c:775)\n==13457== by 0x805422A: diffcore_rename (diffcore-rename.c:415)\n==13457== by 0x805249D: diffcore_std (diff.c:1093)\n==13457== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13457== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13457== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13457== by 0x804A884: main (diff-tree.c:551)\n==13457== \n==13457== Invalid read of size 1\n==13457== at 0x1B901423: strcmp (mac_replace_strmem.c:332)\n==13457== by 0x8054038: locate_rename_dst (diffcore-rename.c:28)\n==13457== by 0x805464B: diffcore_rename (diffcore-rename.c:356)\n==13457== by 0x805249D: diffcore_std (diff.c:1093)\n==13457== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13457== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13457== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13457== by 0x804A884: main (diff-tree.c:551)\n==13504== Invalid read of size 4\n==13504== at 0x805402C: locate_rename_dst (diffcore-rename.c:28)\n==13504== by 0x805464B: diffcore_rename (diffcore-rename.c:356)\n==13504== by 0x805249D: diffcore_std (diff.c:1093)\n==13504== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13504== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13504== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13504== by 0x804A884: main (diff-tree.c:551)\n==13504== Address 0x1BBD781C is 20 bytes inside a block of size 71 free'd\n==13504== at 0x1B9003C3: free (vg_replace_malloc.c:235)\n==13504== by 0x805205B: diff_free_filepair (diff.c:775)\n==13504== by 0x805422A: diffcore_rename (diffcore-rename.c:415)\n==13504== by 0x805249D: diffcore_std (diff.c:1093)\n==13504== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13504== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13504== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13504== by 0x804A884: main (diff-tree.c:551)\n==13504== \n==13504== Invalid read of size 1\n==13504== at 0x1B90140D: strcmp (mac_replace_strmem.c:332)\n==13504== by 0x8054038: locate_rename_dst (diffcore-rename.c:28)\n==13504== by 0x805464B: diffcore_rename (diffcore-rename.c:356)\n==13504== by 0x805249D: diffcore_std (diff.c:1093)\n==13504== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13504== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13504== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13504== by 0x804A884: main (diff-tree.c:551)\n==13504== Address 0x1BBD7830 is 40 bytes inside a block of size 71 free'd\n==13504== at 0x1B9003C3: free (vg_replace_malloc.c:235)\n==13504== by 0x805205B: diff_free_filepair (diff.c:775)\n==13504== by 0x805422A: diffcore_rename (diffcore-rename.c:415)\n==13504== by 0x805249D: diffcore_std (diff.c:1093)\n==13504== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13504== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13504== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13504== by 0x804A884: main (diff-tree.c:551)\n==13504== \n==13504== Invalid read of size 1\n==13504== at 0x1B901423: strcmp (mac_replace_strmem.c:332)\n==13504== by 0x8054038: locate_rename_dst (diffcore-rename.c:28)\n==13504== by 0x805464B: diffcore_rename (diffcore-rename.c:356)\n==13504== by 0x805249D: diffcore_std (diff.c:1093)\n==13504== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13504== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13504== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13504== by 0x804A884: main (diff-tree.c:551)\n==13504== Address 0x1BBD7831 is 41 bytes inside a block of size 71 free'd\n==13504== at 0x1B9003C3: free (vg_replace_malloc.c:235)\n==13504== by 0x805205B: diff_free_filepair (diff.c:775)\n==13504== by 0x805422A: diffcore_rename (diffcore-rename.c:415)\n==13504== by 0x805249D: diffcore_std (diff.c:1093)\n==13504== by 0x8049B48: call_diff_flush (diff-tree.c:273)\n==13504== by 0x804A225: diff_tree_sha1_top (diff-tree.c:298)\n==13504== by 0x804A30B: diff_tree_commit (diff-tree.c:363)\n==13504== by 0x804A884: main (diff-tree.c:551)\n==13504== \n\nPerhaps that explains the difference.\n\n-Wayne\n"},{"id":"8548","messageId":"7v3bo7jxdn.fsf@assigned-by-dhcp.cox.net","threadId":"1800","inReplyTo":"59a6e583050914094777c4fe96@mail.gmail.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-14T18:51:48Z","receivedAt":"2005-09-14T18:51:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wayne Scott writes:\n\n> Look at the diffs between ad6571a78ac74e9fa27e581834709067dba459af and\n> it's parent with and without rename detection enabled. (In linux-2.6\n> git tree)\n\n> $ git-diff-tree -r -M $REV^1 $REV | grep termios.h\n> :000000 100644 0000000000000000000000000000000000000000\n> 237533bb0e9f1a3e640c4906d8b350deafd315b9 A include/asm-powerpc/termios.h\n> :100644 000000 97c6287a6cbaa5903ee1a5934a5553e9e485d8e7\n> 0000000000000000000000000000000000000000 D include/asm-ppc/termios.h\n>\n> Notice how the the fact that include/asm-ppc64/termios.h is deleted gets lost?\n> Looks broken to me.\n\nIt looks broken to me, too. I rebuilt from a reasonably ancient\nsource (v0.99) and re-run the test but I could not get it to\nproduce 'A' for include/asm-powerpc/termios.h. So I rewound it\nfurther to 4d235c8044a638108b67e22f94b2876657130fc8 commit,\nwhich is really ancient version, but it still says it is renamed\nfrom asm-ppc64 directory. FWIW, all the v0.99* tagged versions\nseem to detect that rename correctly and not lose anything in my\ntests.\n\nWhich version of git do you run and on what platform? It might\nbe that something in the diffcore chain is broken in non-i386\nand/or non-GNU/Linux and/or non-GCC environment.\n\nShoot, I thought it would be a good practice-case for me to use\n'git bisect' in reverse to find the commit that fixed a bug ;-).\nMy copy of linux-2.6 repository for testing is fully packed so I\ncould not try the commit that introduced diffcore-rename.c, but\nthat is what I really wanted to try.\n"},{"id":"8551","messageId":"7vmzmfh2y1.fsf@assigned-by-dhcp.cox.net","threadId":"1800","inReplyTo":"59a6e5830509141208282166c8@mail.gmail.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-14T19:19:50Z","receivedAt":"2005-09-14T19:19:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Could you revert one or both of these commits and try the same\ntest on the 64-bit box you saw problems with please?\n\nCommit: 90a734dc7f37a7bd1f3beec4d33acad559360f6c\nAuthor: Yasushi SHOJI \nDate: Sun Aug 21 16:14:16 2005 +0900\n\n [PATCH] possible memory leak in diff.c::diff_free_filepair()\n\nCommit: 068eac91ce04b9aca163acb1927c3878c45d1a07\nAuthor: Yasushi SHOJI \nDate: Sat Aug 13 19:58:56 2005 +0900\n\n [PATCH] plug memory leak in diff.c::diff_free_filepair()\n"},{"id":"8554","messageId":"Pine.LNX.4.58.0509141321180.26803@g5.osdl.org","threadId":"1800","inReplyTo":"59a6e583050914114054b1564d@mail.gmail.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-09-14T20:24:51Z","receivedAt":"2005-09-14T20:24:51Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 14 Sep 2005, Wayne Scott wrote:\n> \n> However, I noticed a whole collection of errors from valgrind when I\n> run this command line:\n\nI get even more, including:\n\n\t==3234== Use of uninitialised value of size 4\n\t==3234== at 0x80507C1: alloc_filespec (diff.c:224)\n\t==3234== by 0x8052387: diff_addremove (diff.c:1144)\n\t==3234== by 0x8049B74: show_file (diff-tree.c:97)\n\t==3234== by 0x8049E17: diff_tree (diff-tree.c:118)\n\nDamn, too bad valgrind doesn't work on ppc64, so I can't use it on my main \nmachine. It seems to be in development on ppc32, so maybe some day.\n\nI'll look at it on my other machines instead,\n\n\t\tLinus\n"},{"id":"8557","messageId":"Pine.LNX.4.58.0509141334480.26803@g5.osdl.org","threadId":"1800","inReplyTo":"Pine.LNX.4.58.0509141321180.26803@g5.osdl.org","subject":"Fix alloc_filespec() initialization","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-09-14T20:41:24Z","receivedAt":"2005-09-14T20:41:24Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThis simplifies and fixes the initialization of a \"diff_filespec\" when \nallocated.\n\nThe old code would not initialize \"sha1_valid\". Noticed by valgrind.\n\nSigned-off-by: Linus Torvalds \n---\n\nThis does not fix the issue Wayne saw, but I'm not going to look at the \nlater valgrind errors before I've fixed the first ones.\n\nOn Wed, 14 Sep 2005, Linus Torvalds wrote:\n> \n> I get even more, including:\n> \n> \t==3234== Use of uninitialised value of size 4\n> \t==3234== at 0x80507C1: alloc_filespec (diff.c:224)\n> \t==3234== by 0x8052387: diff_addremove (diff.c:1144)\n> \t==3234== by 0x8049B74: show_file (diff-tree.c:97)\n> \t==3234== by 0x8049E17: diff_tree (diff-tree.c:118)\n\ndiff --git a/diff.c b/diff.c\n--- a/diff.c\n+++ b/diff.c\n@@ -214,14 +214,10 @@ struct diff_filespec *alloc_filespec(con\n {\n \tint namelen = strlen(path);\n \tstruct diff_filespec *spec = xmalloc(sizeof(*spec) + namelen + 1);\n+\n+\tmemset(spec, 0, sizeof(*spec));\n \tspec->path = (char *)(spec + 1);\n-\tstrcpy(spec->path, path);\n-\tspec->should_free = spec->should_munmap = 0;\n-\tspec->xfrm_flags = 0;\n-\tspec->size = 0;\n-\tspec->data = NULL;\n-\tspec->mode = 0;\n-\tmemset(spec->sha1, 0, 20);\n+\tmemcpy(spec->path, path, namelen+1);\n \treturn spec;\n }\n \n"},{"id":"8560","messageId":"Pine.LNX.4.58.0509141352010.26803@g5.osdl.org","threadId":"1800","inReplyTo":"7vmzmfh2y1.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-diff-tree rename detection bug","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-09-14T20:53:34Z","receivedAt":"2005-09-14T20:53:34Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 14 Sep 2005, Junio C Hamano wrote:\n>\n> Could you revert one or both of these commits and try the same\n> test on the 64-bit box you saw problems with please?\n> \n> Commit: 90a734dc7f37a7bd1f3beec4d33acad559360f6c\n> Author: Yasushi SHOJI \n> Date: Sun Aug 21 16:14:16 2005 +0900\n> \n> [PATCH] possible memory leak in diff.c::diff_free_filepair()\n> \n> Commit: 068eac91ce04b9aca163acb1927c3878c45d1a07\n> Author: Yasushi SHOJI \n> Date: Sat Aug 13 19:58:56 2005 +0900\n> \n> [PATCH] plug memory leak in diff.c::diff_free_filepair()\n\nUndoing that second one (068eac91ce04b9aca163acb1927c3878c45d1a07) fixes \nthe valgrind errors.\n\n\t\tLinus\n"},{"id":"8565","messageId":"7vfys7fh6e.fsf@assigned-by-dhcp.cox.net","threadId":"1800","inReplyTo":"Pine.LNX.4.58.0509141352010.26803@g5.osdl.org","subject":"Re: git-diff-tree rename detection bug","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-14T21:55:21Z","receivedAt":"2005-09-14T21:55:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Undoing that second one (068eac91ce04b9aca163acb1927c3878c45d1a07) fixes \n> the valgrind errors.\n\nThanks; reverted and applied your other patch and pushed them\nout.\n\nI think the filepairs are sometimes shared so to be able to free\nthem properly, we need to reference count. Ugh.\n"},{"id":"8573","messageId":"17192.56103.803096.526568@cargo.ozlabs.ibm.com","threadId":"1800","inReplyTo":"Pine.LNX.4.58.0509141321180.26803@g5.osdl.org","subject":"Re: git-diff-tree rename detection bug","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2005-09-15T02:23:35Z","receivedAt":"2005-09-15T02:23:35Z","isPatch":false,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Linus Torvalds writes:\n\n> Damn, too bad valgrind doesn't work on ppc64, so I can't use it on my main \n> machine. It seems to be in development on ppc32, so maybe some day.\n\nHow about... today? :-) My port of Valgrind-2.4.1 to ppc32 works\npretty well. You can get it from:\n\nhttp://www.valgrind.org/downloads/variants.html?pmk\n\nI assume you're compiling git as 32-bit executables on your G5. I\ndon't see any reason why the git binaries would need to be 64-bit.\n\nRegards,\nPaul.\n"},{"id":"8574","messageId":"17192.56292.867933.739867@cargo.ozlabs.ibm.com","threadId":"1800","inReplyTo":"17192.56103.803096.526568@cargo.ozlabs.ibm.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2005-09-15T02:26:44Z","receivedAt":"2005-09-15T02:26:44Z","isPatch":false,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"I wrote:\n\n> How about... today? :-) My port of Valgrind-2.4.1 to ppc32 works\n> pretty well. You can get it from:\n\nI meant to say explicitly that it runs quite happily under a ppc64\nkernel, but only does 32-bit executables at present.\n\nPaul.\n"},{"id":"8577","messageId":"Pine.LNX.4.58.0509142029210.26803@g5.osdl.org","threadId":"1800","inReplyTo":"17192.56103.803096.526568@cargo.ozlabs.ibm.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-09-15T03:29:50Z","receivedAt":"2005-09-15T03:29:50Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 15 Sep 2005, Paul Mackerras wrote:\n>\n> Linus Torvalds writes:\n> \n> > Damn, too bad valgrind doesn't work on ppc64, so I can't use it on my main \n> > machine. It seems to be in development on ppc32, so maybe some day.\n> \n> How about... today? :-) My port of Valgrind-2.4.1 to ppc32 works\n> pretty well. You can get it from:\n> \n> http://www.valgrind.org/downloads/variants.html?pmk\n\nAhh. Compiling right now.\n\n> I assume you're compiling git as 32-bit executables on your G5. I\n> don't see any reason why the git binaries would need to be 64-bit.\n\nI use whatever the defaults are. And yes, it seems to me -m32.\n\n\t\tLinus\n"},{"id":"8578","messageId":"Pine.LNX.4.58.0509142032300.26803@g5.osdl.org","threadId":"1800","inReplyTo":"17192.56292.867933.739867@cargo.ozlabs.ibm.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-09-15T03:36:00Z","receivedAt":"2005-09-15T03:36:00Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 15 Sep 2005, Paul Mackerras wrote:\n> \n> I meant to say explicitly that it runs quite happily under a ppc64\n> kernel, but only does 32-bit executables at present.\n\nIt works, but it does end up complaining about things like \n\n\t==23756== Invalid read of size 4\n\t==23756== at 0x25A38990: strlen (in /lib/libc-2.3.5.so)\n\t..\n\t==23756== Address 0x25B86754 is 3 bytes after a block of size 17 alloc'd\n\nwhich seems to be just strlen prefetching the next word or something like \nthat. \n\nBut it's still nicer than not having it at all, even if it appears I'll \nhave to do some filtering of my own.\n\n\t\tLinus\n"},{"id":"8579","messageId":"17192.65054.520959.454610@cargo.ozlabs.ibm.com","threadId":"1800","inReplyTo":"Pine.LNX.4.58.0509142032300.26803@g5.osdl.org","subject":"Re: git-diff-tree rename detection bug","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2005-09-15T04:52:46Z","receivedAt":"2005-09-15T04:52:46Z","isPatch":false,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Linus Torvalds writes:\n\n> It works, but it does end up complaining about things like \n> \n> \t==23756== Invalid read of size 4\n> \t==23756== at 0x25A38990: strlen (in /lib/libc-2.3.5.so)\n> \t..\n> \t==23756== Address 0x25B86754 is 3 bytes after a block of size 17 alloc'd\n> \n> which seems to be just strlen prefetching the next word or something like \n> that. \n\nThe strlen() in glibc for ppc is unbearably clever hand-coded\nassembly, which loads up 8 bytes at a time (once it has the address\n8-byte aligned), and does various ANDs and ORs and ADDs and\nconditional branches. If some of the 8 bytes aren't defined, it will\nin many cases branch one way or the other based on the undefined\nbytes, but end up computing the same result on either branch.\n\nValgrind is right in that strlen is loading up some bytes that are\npast the end of a malloc'd block. In fact those bytes don't end up\naffecting the result, and in fact the load couldn't cause a segfault,\nbut it's not surprising that Valgrind can't see that, since the value\nof the extra bytes can actually affect whether a conditional branch is\ntaken or not, but we end up with the same result either way.\n\nValgrind sets LD_PRELOAD so that you get a simple Valgrind-supplied\nset of string functions, including strlen, from vgpreload_memcheck.so\nrather than the fancy glibc ones. However, that doesn't seem to catch\nthe calls to strlen from inside glibc - the call from vfprintf is a\ndirect branch rather than going through the PLT, for instance.\n\nI could add a suppression to suppress all errors in strlen, but that\nwould mean you would miss real errors, where the string is not\nnull-terminated within the malloc'd block, and strlen runs off the\nend.\n\nI wish I had a good answer for this problem, but I don't. Maybe we\nneed a debugging version of glibc that doesn't use the fancy\nbit-fiddling algorithms in the string functions.\n\n(Just for interest: here are the comments from strlen.S:\n\n 1) Given a word 'x', we can test to see if it contains any 0 bytes\n by subtracting 0x01010101, and seeing if any of the high bits of each\n byte changed from 0 to 1. This works because the least significant\n 0 byte must have had no incoming carry (otherwise it's not the least\n significant), so it is 0x00 - 0x01 == 0xff. For all other\n byte values, either they have the high bit set initially, or when\n 1 is subtracted you get a value in the range 0x00-0x7f, none of which\n have their high bit set. The expression here is\n (x + 0xfefefeff) & ~(x | 0x7f7f7f7f), which gives 0x00000000 when\n there were no 0x00 bytes in the word.\n\n 2) Given a word 'x', we can test to see _which_ byte was zero by\n calculating ~(((x & 0x7f7f7f7f) + 0x7f7f7f7f) | x | 0x7f7f7f7f).\n This produces 0x80 in each byte that was zero, and 0x00 in all\n the other bytes. The '| 0x7f7f7f7f' clears the low 7 bits in each\n byte, and the '| x' part ensures that bytes with the high bit set\n produce 0x00. The addition will carry into the high bit of each byte\n iff that byte had one of its low 7 bits set. We can then just see\n which was the most significant bit set and divide by 8 to find how\n many to add to the index.\n This is from the book 'The PowerPC Compiler Writer's Guide',\n by Steve Hoxey, Faraydon Karim, Bill Hay and Hank Warren.\n)\n\nPaul.\n"},{"id":"8583","messageId":"43290DA0.3030402@zytor.com","threadId":"1800","inReplyTo":"17192.56103.803096.526568@cargo.ozlabs.ibm.com","subject":"Re: git-diff-tree rename detection bug","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-09-15T05:58:56Z","receivedAt":"2005-09-15T05:58:56Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Paul Mackerras wrote:\n> \n> I assume you're compiling git as 32-bit executables on your G5. I\n> don't see any reason why the git binaries would need to be 64-bit.\n> \n\nWell, git seems to assume it can mmap() the entirety of any file under \nits control, so a 64-bit git could handle larger files.\n\nStill, I'm using 32-bit git on ppc64.\n\n\t-hpa\n"},{"id":"8586","messageId":"7vek7qbwws.fsf@assigned-by-dhcp.cox.net","threadId":"1800","inReplyTo":"43290DA0.3030402@zytor.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-15T07:41:23Z","receivedAt":"2005-09-15T07:41:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" writes:\n\n> Paul Mackerras wrote:\n>> I assume you're compiling git as 32-bit executables on your G5. I\n>> don't see any reason why the git binaries would need to be 64-bit.\n>>\n>\n> Well, git seems to assume it can mmap() the entirety of any file under \n> its control, so a 64-bit git could handle larger files.\n>\n> Still, I'm using 32-bit git on ppc64.\n\nWhich is a valid thing to do because git also assumes that a\nfile offset fits within 32-bit as far as I know. Each object in\na packfile should also fit in 32-bit offset and the size of one\npackfile also needs to be within 32-bit offset. I was unsure if\nthe last limitation would cause problems in the real life but we\nwill see in a couple of years ;-).\n"},{"id":"8591","messageId":"7vll1y9243.fsf@assigned-by-dhcp.cox.net","threadId":"1800","inReplyTo":"17192.65054.520959.454610@cargo.ozlabs.ibm.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-15T08:17:16Z","receivedAt":"2005-09-15T08:17:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Mackerras writes:\n\n> The strlen() in glibc for ppc is unbearably clever hand-coded\n> assembly, which loads up 8 bytes at a time (once it has the address\n> 8-byte aligned), and does various ANDs and ORs and ADDs and\n> conditional branches. If some of the 8 bytes aren't defined, it will\n> in many cases branch one way or the other based on the undefined\n> bytes, but end up computing the same result on either branch.\n\nThis reminds me of what I did in my previous life, writing a\nmemory allocation checker -- this was before Valgrind -- and\nfound out that strcpy in the C library that came with Solaris\nhad a similar clever trick. What was interesting was that\ncopying a string starting at the (PAGESIZE-3)th byte on a page\nand NUL terminated at the end of the same page ended up\nprefetching the first word from the next page (please do not ask\nme about the details -- I do not remember the disassembly of\nthat part of the code anymore). It was not an inconvenience for\nour memory checker but was a real bug -- the next page could\nvery well be unaccessible.\n\nThe bug was fixed in the next version of the C library when we\nupdated our Solaris box.\n"},{"id":"8603","messageId":"200509151649.47159.Josef.Weidendorfer@gmx.de","threadId":"1800","inReplyTo":"17192.65054.520959.454610@cargo.ozlabs.ibm.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Josef Weidendorfer","fromEmail":"josef.weidendorfer@gmx.de","sentAt":"2005-09-15T14:49:46Z","receivedAt":"2005-09-15T14:49:46Z","isPatch":false,"sender":{"key":"josef.weidendorfer@gmx.de","avatar":null},"body":"On Thursday 15 September 2005 06:52, Paul Mackerras wrote:\n> Valgrind sets LD_PRELOAD so that you get a simple Valgrind-supplied\n> set of string functions, including strlen, from vgpreload_memcheck.so\n> rather than the fancy glibc ones. However, that doesn't seem to catch\n> the calls to strlen from inside glibc - the call from vfprintf is a\n> direct branch rather than going through the PLT, for instance.\n\nJust curious: Why is it using LD_PRELOAD and not the VGs symbol redirection \nmechanism, which should catch strlen even if used inside of glibc?\n\nJosef\n"},{"id":"8604","messageId":"Pine.LNX.4.58.0509150739460.26803@g5.osdl.org","threadId":"1800","inReplyTo":"43290DA0.3030402@zytor.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-09-15T14:55:34Z","receivedAt":"2005-09-15T14:55:34Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 14 Sep 2005, H. Peter Anvin wrote:\n> \n> Well, git seems to assume it can mmap() the entirety of any file under \n> its control, so a 64-bit git could handle larger files.\n\nRight now git isn't 64-bit clean anyway (well, it should _work_ fine on\n64-bit architectures, but it just can't handle files >32 bits).\n\nI think the core object format should be all perfectly ok (since all the\nsizes etc are in ascii), so this is not a huge deal. If you have an \nexisting git archive and suddenly notice that you need 32+ bit file \nformats, you don't have to throw your archive away and re-generate it from \nscratch in some new format. \n\nIn fact, I think even the streaming pack format is 64-bit-safe: it has \nbinary sizes, but they are all length-encoded, and I think the data \nstructure is safe.\n\nAlso, the index file - in order to not be unnecessarily big, and to be\nable to use standard htonl/ntohl helpers etc - uses 32-bit lengths for\nfiles. Even that _that_ should be 64-bit safe, because the length isn't \nactually _used_ for anything but to verify the curret state, and as with \nthe timestamps etc, it's ok to \"only\" check the low 32 bits. \n\nSo the on-disk format should be capable of handling 64-bit entities.\n\nHOWEVER. I might be wrong. I tried to think about it, but I didn't care\ntoo much, because I think git would suck at truly huge files. If only\nbecause compressing them will take forever. And the _implementation_ uses\n\"unsigned int\" and has never been tested with anything else, so it would \nneed a lot of testing.\n\nAlso, the \"pack index\" file can only handle 32-bit offsets - you can make \na pack-file that is bigger than that, and it should be fine from a \n_streaming_ standpoint (ie in the way we use them for network transport), \nbut you can't index them in .git/objects/packs.\n\nWhich isn't a disaster: you might choose to say that you never pack huge \nfiles. That might be ok for some cases (maybe the huge file is a one-off \nsatellite picture). It would suck if the huge file is a incrementally \ncreated log-file, where packing really would be nice.\n\nIF we ever hit this, and IF people decide that git actually makes sense \nfor those kinds of files, we CAN change the pack index format. It has a \nversion number and everything, so we can even do it gently. Hopefully that \nwould be the only actual on-disk format that would need to change. But \nregardless, the git code itself would need a lot of verification to make \nsure that it handles big files correctly.\n\nI'm not seeing that as a high priority. Maybe in five years ;)\n\n\t\t\tLinus\n"},{"id":"8635","messageId":"Pine.LNX.4.63.0509151647380.31877@localhost.localdomain","threadId":"1800","inReplyTo":"43290DA0.3030402@zytor.com","subject":"Re: git-diff-tree rename detection bug","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2005-09-15T20:57:19Z","receivedAt":"2005-09-15T20:57:19Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 14 Sep 2005, H. Peter Anvin wrote:\n\n> Paul Mackerras wrote:\n> > \n> > I assume you're compiling git as 32-bit executables on your G5. I\n> > don't see any reason why the git binaries would need to be 64-bit.\n> > \n> \n> Well, git seems to assume it can mmap() the entirety of any file under its\n> control, so a 64-bit git could handle larger files.\n\nBut beware that the delta code would break since it currently won't cope \nwith any file offset larger than 32 bits. I reserved the zero byte to \nprefix any extended encoding but felt it wasn't really needed yet and I \njust didn't bother coding it.\n\n\nNicolas\n"},{"id":"8657","messageId":"7v3bo5v9hl.fsf@assigned-by-dhcp.cox.net","threadId":"1800","inReplyTo":"Pine.LNX.4.58.0509142029210.26803@g5.osdl.org","subject":"Re: git-diff-tree rename detection bug","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-16T05:59:18Z","receivedAt":"2005-09-16T05:59:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Thu, 15 Sep 2005, Paul Mackerras wrote:\n>>\n>> Linus Torvalds writes:\n>> \n>> > Damn, too bad valgrind doesn't work on ppc64, so I can't use it on my main \n>> > machine. It seems to be in development on ppc32, so maybe some day.\n\nFWIW, I think I fixed the leak that patch we reverted was\nattempting to fix, and on my i386 box valgrind seems much\nhappier.\n"},{"id":"8676","messageId":"pan.2005.09.16.12.53.23.881501@smurf.noris.de","threadId":"1800","inReplyTo":"Pine.LNX.4.58.0509150739460.26803@g5.osdl.org","subject":"Re: git-diff-tree rename detection bug","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-09-16T12:53:24Z","receivedAt":"2005-09-16T12:53:24Z","isPatch":false,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"Hi, Linus Torvalds wrote:\n\n> I'm not seeing that as a high priority. Maybe in five years ;)\n\nHmm. Just for comparison, how long was the time between \"this will never\nrun on anything but i386\" and \"mkdir arch/alpha\"? ;-)\n\n-- \nMatthias Urlichs | {M:U} IT Design @ m-u-it.de | smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\n...He who laughs does not believe in what he laughs at, but neither\ndoes he hate it. Therefore, laughing at evil means not preparing oneself to\ncombat it, and laughing at good means denying the power through which good is\nself-propagating.\n\t\t-- Umberto Eco, \"The Name of the Rose\"\n"},{"id":"8984","messageId":"17199.59777.784039.885671@cargo.ozlabs.ibm.com","threadId":"1800","inReplyTo":"200509151649.47159.Josef.Weidendorfer@gmx.de","subject":"Re: [Valgrind-developers] Re: git-diff-tree rename detection bug","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2005-09-20T10:50:41Z","receivedAt":"2005-09-20T10:50:41Z","isPatch":false,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Josef Weidendorfer writes:\n\n> Just curious: Why is it using LD_PRELOAD and not the VGs symbol redirection \n> mechanism, which should catch strlen even if used inside of glibc?\n\nValgrind-2.4.1 does symbol redirects for stpcpy and strnlen in\nlibc.so.6, and stpcpy and strchr in ld-linux.so.2. I could extend\nthat list on ppc to include strlen et al., but I don't know if that\nwould solve the problem for ld.so, since strlen doesn't appear in the\nsymbol table for ld.so.\n\nPaul.\n"}]}