# Re: Re: Segmentation fault with latest git (070c57df) 5 messages from 2013-02-01 to 2013-02-01. Participants: 허종만, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/32805 ## 허종만, 2013-02-01 01:31 Subject: Re: Re: Segmentation fault with latest git (070c57df) Message-ID: <15540714.372461359682286477.JavaMail.weblogic@epml08> URL: https://gitlist.dev/e/15540714.372461359682286477.JavaMail.weblogic%40epml08 ``` > [snip] > Good point. Unfortunately, I can't get either yours or mine to fail, > neither with a recent version of gcc nor with gcc-4.1. But I can't > convince git to fail, either. The only gcc-4.1 I have is Debian's > 4.1.3 release, which is not quite what the OP has. > > > Or perhaps something in the build process went wrong, and fetch.c didn't > > get the memo about the new field in the struct. Depending on stack > > layout, the next variable might be the 'int i' right before the > > 'string_list list' in the code, which could explain the value of 1. > > Yeah, that would make sense to me with respect to the behavior we are > seeing, but that part of the Makefile should be pretty simple and > bug-free, I'd think (and from the original report, it seems like he was > able to reproduce it well enough to bisect). Still, trying a "make clean > && make" might be worth it just to rule that out. > > Puzzled... > > -Peff Hi, all, Thomas's test code also returns "cmp is 0". But "make clean && make" fixes my issue. Sorry for the noise I made. But usually when I build upstream Linux kernel, I don't do "make clean" after git pull.. I didn't expect that I needed "make clean" for git build. Thanks you guys. Best regards, Jongman Heo. ``` ## Junio C Hamano, 2013-02-01 01:40 Subject: Re: Segmentation fault with latest git (070c57df) Message-ID: <7vmwvodd71.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmwvodd71.fsf%40alter.siamese.dyndns.org In-Reply-To: <15540714.372461359682286477.JavaMail.weblogic@epml08> ``` 허종만 writes: > But usually when I build upstream Linux kernel, I don't do "make > clean" after git pull.. I didn't expect that I needed "make > clean" for git build. We don't expect anybody need "make clean", either. There is something wrong in the dependency. ``` ## Jeff King, 2013-02-01 06:36 Subject: Re: Segmentation fault with latest git (070c57df) Message-ID: <20130201063638.GD29973@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130201063638.GD29973%40sigill.intra.peff.net In-Reply-To: <7vmwvodd71.fsf@alter.siamese.dyndns.org> ``` On Thu, Jan 31, 2013 at 05:40:02PM -0800, Junio C Hamano wrote: > 허종만 writes: > > > But usually when I build upstream Linux kernel, I don't do "make > > clean" after git pull.. I didn't expect that I needed "make > > clean" for git build. > > We don't expect anybody need "make clean", either. There is > something wrong in the dependency. Agreed, but I cannot see what. If auto-header-dependencies is on, gcc should find it (it is not even a recursive dependency for builtin/fetch.c). And if it is not on, we should rebuild based on LIB_H, which includes string-list.h (and always has, as far as I can tell). Hmm. I do notice one oddity with the computed header dependencies, though. We build the computed dependency files as a side effect of doing the actual compilation. So before we have run the compilation once, we need some way to say "you _must_ build this, because we do even know the correct dependencies". And to do that, we have each object file depend on any missing .depend dirs, which bootstraps the whole process: we build everything the first time because .depend is missing, and from then on, we use the correct dependencies. But that is not quite right. The .depend directory might exist, but be missing the actual dependency file for a particular object. So if I do: $ make ;# builds all objects and dependency files $ rm builtin/.depend/fetch.o.d $ touch string-list.h $ make we will fail to rebuild builtin/fetch.o properly. It does not see the dependency on string-list (because we have no .d file), nor does it realize that it needs to build the .d file (because it only checks that builtin/.depend exists). It seems like building each object file should depend on its dependency file (but only when COMPUTE_HEADER_DEPENDENCIES is on, of course), since otherwise we cannot know if we have the right dependencies or not. Something like this almost works, I think: diff --git a/Makefile b/Makefile index 6b42f66..a329736 100644 --- a/Makefile +++ b/Makefile @@ -1843,8 +1843,8 @@ missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs)) @mkdir -p $@ missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs)) -dep_file = $(dir $@).depend/$(notdir $@).d -dep_args = -MF $(dep_file) -MMD -MP +dep_file = $(dir $1).depend/$(notdir $1).d +dep_args = -MF $(call dep_file, $@) -MMD -MP ifdef CHECK_HEADER_DEPENDENCIES $(error cannot compute header dependencies outside a normal build. \ Please unset CHECK_HEADER_DEPENDENCIES and try again) @@ -1909,9 +1909,9 @@ $(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) endif ifndef CHECK_HEADER_DEPENDENCIES -$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) +$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(call dep_file, %.o) $(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< -$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) +$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(call dep_file,%.o) $(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< endif But not quite. The problem is that we put the dep file for foo/bar.o into foo/.depend/bar.o. But when we call the dep_file function for the dependency, it sees only "%.o", not "foo/bar.o", so it can't properly split it apart. I don't think there is a way to force expansion before calling the function. And of course I have no idea if this was the problem that we saw, anyway. I have no idea how one would get into this situation short of manually removing the dependency file. -Peff ``` ## Jeff King, 2013-02-01 07:06 Subject: Re: Segmentation fault with latest git (070c57df) Message-ID: <20130201070622.GA700@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130201070622.GA700%40sigill.intra.peff.net In-Reply-To: <20130201063638.GD29973@sigill.intra.peff.net> ``` On Fri, Feb 01, 2013 at 01:36:38AM -0500, Jeff King wrote: > It seems like building each object file should depend on its dependency > file (but only when COMPUTE_HEADER_DEPENDENCIES is on, of course), since > otherwise we cannot know if we have the right dependencies or not. > > Something like this almost works, I think: > [...] > +$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(call dep_file, %.o) Actually that would not work, as we do not have a rule to create .depend/foo.o.d. We can add one, but it gets pretty hairy (and replicates much of the normal build rule). A much simpler way is to just find the missing dep files and force compilation of their matching objects. Like: diff --git a/Makefile b/Makefile index 6b42f66..f94e8b9 100644 --- a/Makefile +++ b/Makefile @@ -1843,8 +1843,14 @@ dep_args = -MF $(dep_file) -MMD -MP @mkdir -p $@ missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs)) +missing_dep_files := $(filter-out $(wildcard $(dep_files)),$(dep_files)) +# we want to rewrite "foo/.depend/bar.o.d" into "foo/bar.o", but +# make's patsubst is not powerful enough to remove something from the middle of +# a string. Hack around it by shelling out. +obj_files_with_missing_deps := $(shell echo $(missing_dep_files:.d=) | tr ' ' '\n' | sed 's,.depend/,,') dep_file = $(dir $@).depend/$(notdir $@).d dep_args = -MF $(dep_file) -MMD -MP +$(obj_files_with_missing_deps): FORCE ifdef CHECK_HEADER_DEPENDENCIES $(error cannot compute header dependencies outside a normal build. \ Please unset CHECK_HEADER_DEPENDENCIES and try again) which does solve the problem, but that shell hack is nasty. It would be much simpler if we stored the dependency for foo/bar.o as ".depend/foo/bar.o.d", rather than "foo/.depend/bar.o.d", as then we would patsubst it away. Or maybe there is some clever way to convince make to do what I want here. Suggestions welcome. -Peff ``` ## Jeff King, 2013-02-01 07:09 Subject: Re: Segmentation fault with latest git (070c57df) Message-ID: <20130201070921.GA970@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130201070921.GA970%40sigill.intra.peff.net In-Reply-To: <20130201063638.GD29973@sigill.intra.peff.net> ``` On Fri, Feb 01, 2013 at 01:36:38AM -0500, Jeff King wrote: > On Thu, Jan 31, 2013 at 05:40:02PM -0800, Junio C Hamano wrote: > > > 허종만 writes: > > > > > But usually when I build upstream Linux kernel, I don't do "make > > > clean" after git pull.. I didn't expect that I needed "make > > > clean" for git build. > > > > We don't expect anybody need "make clean", either. There is > > something wrong in the dependency. > > Agreed, but I cannot see what. If auto-header-dependencies is on, gcc > should find it (it is not even a recursive dependency for > builtin/fetch.c). And if it is not on, we should rebuild based on LIB_H, > which includes string-list.h (and always has, as far as I can tell). By the way, while researching this issue, I noticed this: -- >8 -- Subject: [PATCH] Makefile: add version.h to LIB_H This was forgotten when the file was added by 816fb46, and not noticed because most developers are on modern systems that support COMPUTE_HEADER_DEPENDENCIES. However, people still relying on LIB_H for dependencies might have failed to recompile when this file changed. Found with "make CHECK_HEADER_DEPENDENCIES=yes". Signed-off-by: Jeff King --- I don't see how this could have caused the issue at hand, but it is good to fix nonetheless. I almost wonder if LIB_H should just be set to $(wildcard *.h) or similar, since that is what ends up going into it. And then we would not have to deal with manually keeping it up to date. Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 731b6a8..6b42f66 100644 --- a/Makefile +++ b/Makefile @@ -704,6 +704,7 @@ LIB_H += varint.h LIB_H += userdiff.h LIB_H += utf8.h LIB_H += varint.h +LIB_H += version.h LIB_H += walker.h LIB_H += wildmatch.h LIB_H += wt-status.h -- 1.8.1.2.11.g1a2f572 ```