{"thread":{"id":"32805","subject":"Re: Re: Segmentation fault with latest git (070c57df)","startedAt":"2013-02-01T01:31:27Z","lastAt":"2013-02-01T07:09:21Z","messageCount":5,"participants":["허종만","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"208435","messageId":"15540714.372461359682286477.JavaMail.weblogic@epml08","threadId":"32805","inReplyTo":null,"subject":"Re: Re: Segmentation fault with latest git (070c57df)","fromName":"허종만","fromEmail":"jongman.heo@samsung.com","sentAt":"2013-02-01T01:31:27Z","receivedAt":"2013-02-01T01:31:27Z","isPatch":false,"sender":{"key":"jongman.heo@samsung.com","avatar":null},"body":"\n> \n[snip]\n> Good point. Unfortunately, I can't get either yours or mine to fail,\n> neither with a recent version of gcc nor with gcc-4.1.  But I can't\n> convince git to fail, either. The only gcc-4.1 I have is Debian's\n> 4.1.3 release, which is not quite what the OP has.\n> \n> > Or perhaps something in the build process went wrong, and fetch.c didn't\n> > get the memo about the new field in the struct.  Depending on stack\n> > layout, the next variable might be the 'int i' right before the\n> > 'string_list list' in the code, which could explain the value of 1.\n> \n> Yeah, that would make sense to me with respect to the behavior we are\n> seeing, but that part of the Makefile should be pretty simple and\n> bug-free, I'd think (and from the original report, it seems like he was\n> able to reproduce it well enough to bisect). Still, trying a \"make clean\n> && make\" might be worth it just to rule that out.\n> \n> Puzzled...\n> \n> -Peff\n\nHi, all,\n\nThomas's test code also returns \"cmp is 0\". \nBut \"make clean && make\" fixes my issue.\n\nSorry for the noise I made.\nBut usually when I build upstream Linux kernel, I don't do \"make clean\" after git pull.. \nI didn't expect that I needed \"make clean\" for git build. Thanks you guys.\n\nBest regards,\nJongman Heo."},{"id":"208436","messageId":"7vmwvodd71.fsf@alter.siamese.dyndns.org","threadId":"32805","inReplyTo":"15540714.372461359682286477.JavaMail.weblogic@epml08","subject":"Re: Segmentation fault with latest git (070c57df)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-01T01:40:02Z","receivedAt":"2013-02-01T01:40:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"허종만 <jongman.heo@samsung.com> writes:\n\n> But usually when I build upstream Linux kernel, I don't do \"make\n> clean\" after git pull..  I didn't expect that I needed \"make\n> clean\" for git build.\n\nWe don't expect anybody need \"make clean\", either.  There is\nsomething wrong in the dependency.\n"},{"id":"208448","messageId":"20130201063638.GD29973@sigill.intra.peff.net","threadId":"32805","inReplyTo":"7vmwvodd71.fsf@alter.siamese.dyndns.org","subject":"Re: Segmentation fault with latest git (070c57df)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-01T06:36:38Z","receivedAt":"2013-02-01T06:36:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 31, 2013 at 05:40:02PM -0800, Junio C Hamano wrote:\n\n> 허종만 <jongman.heo@samsung.com> writes:\n> \n> > But usually when I build upstream Linux kernel, I don't do \"make\n> > clean\" after git pull..  I didn't expect that I needed \"make\n> > clean\" for git build.\n> \n> We don't expect anybody need \"make clean\", either.  There is\n> something wrong in the dependency.\n\nAgreed, but I cannot see what. If auto-header-dependencies is on, gcc\nshould find it (it is not even a recursive dependency for\nbuiltin/fetch.c). And if it is not on, we should rebuild based on LIB_H,\nwhich includes string-list.h (and always has, as far as I can tell).\n\nHmm. I do notice one oddity with the computed header dependencies,\nthough. We build the computed dependency files as a side effect of doing\nthe actual compilation. So before we have run the compilation once, we\nneed some way to say \"you _must_ build this, because we do even know the\ncorrect dependencies\".  And to do that, we have each object file depend\non any missing .depend dirs, which bootstraps the whole process: we\nbuild everything the first time because .depend is missing, and from\nthen on, we use the correct dependencies.\n\nBut that is not quite right. The .depend directory might exist, but be\nmissing the actual dependency file for a particular object. So if I do:\n\n  $ make ;# builds all objects and dependency files\n  $ rm builtin/.depend/fetch.o.d\n  $ touch string-list.h\n  $ make\n\nwe will fail to rebuild builtin/fetch.o properly. It does not see the\ndependency on string-list (because we have no .d file), nor does it\nrealize that it needs to build the .d file (because it only checks that\nbuiltin/.depend exists).\n\nIt seems like building each object file should depend on its dependency\nfile (but only when COMPUTE_HEADER_DEPENDENCIES is on, of course), since\notherwise we cannot know if we have the right dependencies or not.\n\nSomething like this almost works, I think:\n\ndiff --git a/Makefile b/Makefile\nindex 6b42f66..a329736 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1843,8 +1843,8 @@ missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))\n \t@mkdir -p $@\n \n missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))\n-dep_file = $(dir $@).depend/$(notdir $@).d\n-dep_args = -MF $(dep_file) -MMD -MP\n+dep_file = $(dir $1).depend/$(notdir $1).d\n+dep_args = -MF $(call dep_file, $@) -MMD -MP\n ifdef CHECK_HEADER_DEPENDENCIES\n $(error cannot compute header dependencies outside a normal build. \\\n Please unset CHECK_HEADER_DEPENDENCIES and try again)\n@@ -1909,9 +1909,9 @@ $(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs)\n endif\n \n ifndef CHECK_HEADER_DEPENDENCIES\n-$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs)\n+$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(call dep_file, %.o)\n \t$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<\n-$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)\n+$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(call dep_file,%.o)\n \t$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<\n endif\n \n\nBut not quite. The problem is that we put the dep file for foo/bar.o\ninto foo/.depend/bar.o. But when we call the dep_file function for the\ndependency, it sees only \"%.o\", not \"foo/bar.o\", so it can't properly\nsplit it apart. I don't think there is a way to force expansion before\ncalling the function.\n\nAnd of course I have no idea if this was the problem that we saw,\nanyway. I have no idea how one would get into this situation short of\nmanually removing the dependency file.\n\n-Peff\n"},{"id":"208449","messageId":"20130201070622.GA700@sigill.intra.peff.net","threadId":"32805","inReplyTo":"20130201063638.GD29973@sigill.intra.peff.net","subject":"Re: Segmentation fault with latest git (070c57df)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-01T07:06:22Z","receivedAt":"2013-02-01T07:06:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 01, 2013 at 01:36:38AM -0500, Jeff King wrote:\n\n> It seems like building each object file should depend on its dependency\n> file (but only when COMPUTE_HEADER_DEPENDENCIES is on, of course), since\n> otherwise we cannot know if we have the right dependencies or not.\n> \n> Something like this almost works, I think:\n> [...]\n> +$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(call dep_file, %.o)\n\nActually that would not work, as we do not have a rule to create\n.depend/foo.o.d. We can add one, but it gets pretty hairy (and\nreplicates much of the normal build rule). A much simpler way is to just\nfind the missing dep files and force compilation of their matching\nobjects. Like:\n\ndiff --git a/Makefile b/Makefile\nindex 6b42f66..f94e8b9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1843,8 +1843,14 @@ dep_args = -MF $(dep_file) -MMD -MP\n \t@mkdir -p $@\n \n missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))\n+missing_dep_files := $(filter-out $(wildcard $(dep_files)),$(dep_files))\n+# we want to rewrite \"foo/.depend/bar.o.d\" into \"foo/bar.o\", but\n+# make's patsubst is not powerful enough to remove something from the middle of\n+# a string. Hack around it by shelling out.\n+obj_files_with_missing_deps := $(shell echo $(missing_dep_files:.d=) | tr ' ' '\\n' | sed 's,.depend/,,')\n dep_file = $(dir $@).depend/$(notdir $@).d\n dep_args = -MF $(dep_file) -MMD -MP\n+$(obj_files_with_missing_deps): FORCE\n ifdef CHECK_HEADER_DEPENDENCIES\n $(error cannot compute header dependencies outside a normal build. \\\n Please unset CHECK_HEADER_DEPENDENCIES and try again)\n\nwhich does solve the problem, but that shell hack is nasty. It would be\nmuch simpler if we stored the dependency for foo/bar.o as\n\".depend/foo/bar.o.d\", rather than \"foo/.depend/bar.o.d\", as then we\nwould patsubst it away. Or maybe there is some clever way to convince\nmake to do what I want here. Suggestions welcome.\n\n-Peff\n"},{"id":"208450","messageId":"20130201070921.GA970@sigill.intra.peff.net","threadId":"32805","inReplyTo":"20130201063638.GD29973@sigill.intra.peff.net","subject":"Re: Segmentation fault with latest git (070c57df)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-01T07:09:21Z","receivedAt":"2013-02-01T07:09:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 01, 2013 at 01:36:38AM -0500, Jeff King wrote:\n\n> On Thu, Jan 31, 2013 at 05:40:02PM -0800, Junio C Hamano wrote:\n> \n> > 허종만 <jongman.heo@samsung.com> writes:\n> > \n> > > But usually when I build upstream Linux kernel, I don't do \"make\n> > > clean\" after git pull..  I didn't expect that I needed \"make\n> > > clean\" for git build.\n> > \n> > We don't expect anybody need \"make clean\", either.  There is\n> > something wrong in the dependency.\n> \n> Agreed, but I cannot see what. If auto-header-dependencies is on, gcc\n> should find it (it is not even a recursive dependency for\n> builtin/fetch.c). And if it is not on, we should rebuild based on LIB_H,\n> which includes string-list.h (and always has, as far as I can tell).\n\nBy the way, while researching this issue, I noticed this:\n\n-- >8 --\nSubject: [PATCH] Makefile: add version.h to LIB_H\n\nThis was forgotten when the file was added by 816fb46, and\nnot noticed because most developers are on modern systems\nthat support COMPUTE_HEADER_DEPENDENCIES.  However, people\nstill relying on LIB_H for dependencies might have failed to\nrecompile when this file changed.\n\nFound with \"make CHECK_HEADER_DEPENDENCIES=yes\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI don't see how this could have caused the issue at hand, but it is good\nto fix nonetheless. I almost wonder if LIB_H should just be set to\n$(wildcard *.h) or similar, since that is what ends up going into it.\nAnd then we would not have to deal with manually keeping it up to date.\n\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 731b6a8..6b42f66 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -704,6 +704,7 @@ LIB_H += varint.h\n LIB_H += userdiff.h\n LIB_H += utf8.h\n LIB_H += varint.h\n+LIB_H += version.h\n LIB_H += walker.h\n LIB_H += wildmatch.h\n LIB_H += wt-status.h\n-- \n1.8.1.2.11.g1a2f572\n"}]}