Re: Re: Segmentation fault with latest git (070c57df)

Re: Re: Segmentation fault with latest git (070c57df)

[Jongman Heo]

On Thu, Jan 31, 2013 at 7:49 AM, Jeff King wrote:
> On Thu, Jan 31, 2013 at 01:35:21AM +0000, Jongman Heo wrote:
>
>> Looks like following commit causes a segmentation fault in my machine
>> (when running git pull or git fetch);
>>
>> commit 8dd5afc926acb9829ebf56e9b78826a5242cd638
>> Author: Junio C Hamano 
>> Date:   Mon Jan 7 12:24:55 2013 -0800
>>
>>     string-list: allow case-insensitive string list
>>
>>
>> In my case, list->cmp (at get_entry_index() function) has an invalid
>> address, obviously not an address of string comparision function,
>> instead it points to 1.
>
> Can you show us a stack trace? The string-list functions are generic and
> get called in a lot of places. It would be useful to know which list is
> causing the problem.
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi,

FYI, gdb backtrace and valgrind output attached below, Thanks.


(gdb) run fetch
Starting program: /home/hjongman/repos/git/git fetch
warning: .dynamic section for "/lib/libc.so.6" is not at the expected address
warning: difference appears to be caused by prelink, adjusting expectations
[Thread debugging using libthread_db enabled]

Program received signal SIGSEGV, Segmentation fault.
0x00000001 in ?? ()
(gdb) bt
#0  0x00000001 in ?? ()
#1  0x0812b457 in get_entry_index (list=0xbfffe7c0, string=0x821ec3c "refs/remotes/origin/HEAD", exact_match=0xbfffe568)
    at string-list.c:14
#2  0x0812bd60 in add_entry (list=0xbfffe7c0, insert_at=-1, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:33
#3  string_list_insert_at_index (list=0xbfffe7c0, insert_at=-1, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:63
#4  0x0812bda0 in string_list_insert (list=0xbfffe7c0, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:57
#5  0x08071838 in add_existing (refname=0x821ec3c "refs/remotes/origin/HEAD", 
    sha1=0x821ec14 "\a\fW\337B\352N\255\314C\320Em\021E`\022C&", <incomplete sequence \303>, flag=1, cbdata=0xbfffe7c0)
    at builtin/fetch.c:570
#6  0x0810af97 in do_one_ref (base=<value optimized out>, fn=0x8071820 <add_existing>, trim=0, flags=<value optimized out>, 
    cb_data=0xbfffe7c0, entry=0x821ec10) at refs.c:525
#7  0x0810bd9f in do_for_each_ref_in_dirs (dir1=0x8215d54, dir2=0x821ea44, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, 
    flags=0, cb_data=0xbfffe7c0) at refs.c:627
#8  0x0810bc8e in do_for_each_ref_in_dirs (dir1=0x8215cac, dir2=0x8226954, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, 
    flags=0, cb_data=0xbfffe7c0) at refs.c:597
#9  0x0810bc8e in do_for_each_ref_in_dirs (dir1=0x8215c0c, dir2=0x8215a54, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, 
    flags=0, cb_data=0xbfffe7c0) at refs.c:597
#10 0x0810bc8e in do_for_each_ref_in_dirs (dir1=0x8215a1c, dir2=0x821862c, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, 
    flags=0, cb_data=0xbfffe7c0) at refs.c:597
#11 0x0810c304 in do_for_each_ref (submodule=<value optimized out>, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, flags=0, 
    cb_data=0xbfffe7c0) at refs.c:1295
#12 0x0810c63b in for_each_ref (fn=0x8071820 <add_existing>, cb_data=0xbfffe7c0) at refs.c:1343
#13 0x0807390a in do_fetch (remote=<value optimized out>, argc=0, argv=0xbfffe9f8) at builtin/fetch.c:699
#14 fetch_one (remote=<value optimized out>, argc=0, argv=0xbfffe9f8) at builtin/fetch.c:949
#15 0x08074251 in cmd_fetch (argc=1, argv=0xbfffe9f8, prefix=0x0) at builtin/fetch.c:992
#16 0x0804b60b in run_builtin (argc=1, argv=0xbfffe9f8) at git.c:281
#17 handle_internal_command (argc=1, argv=0xbfffe9f8) at git.c:443
#18 0x0804ba51 in run_argv (argc=1, argv=0xbfffe9f8) at git.c:489
#19 main (argc=1, argv=0xbfffe9f8) at git.c:564


$ valgrind git fetch
==2195== Memcheck, a memory error detector
==2195== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==2195== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info
==2195== Command: git fetch
==2195== 
==2195== Conditional jump or move depends on uninitialised value(s)
==2195==    at 0x812B41F: get_entry_index (string-list.c:10)
==2195==    by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
==2195==    by 0x812BD9F: string_list_insert (string-list.c:57)
==2195==    by 0x8071837: add_existing (fetch.c:570)
==2195==    by 0x810AF96: do_one_ref (refs.c:525)
==2195==    by 0x810BB20: do_for_each_ref_in_dir (refs.c:551)
==2195==    by 0x810BD34: do_for_each_ref_in_dirs (refs.c:623)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810C303: do_for_each_ref (refs.c:1295)
==2195==    by 0x810C63A: for_each_ref (refs.c:1343)
==2195==    by 0x8073909: fetch_one (fetch.c:699)
==2195==    by 0x8074250: cmd_fetch (fetch.c:992)
==2195== 
==2195== Use of uninitialised value of size 4
==2195==    at 0x812B454: get_entry_index (string-list.c:14)
==2195==    by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
==2195==    by 0x812BD9F: string_list_insert (string-list.c:57)
==2195==    by 0x8071837: add_existing (fetch.c:570)
==2195==    by 0x810AF96: do_one_ref (refs.c:525)
==2195==    by 0x810BD9E: do_for_each_ref_in_dirs (refs.c:627)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810C303: do_for_each_ref (refs.c:1295)
==2195==    by 0x810C63A: for_each_ref (refs.c:1343)
==2195==    by 0x8073909: fetch_one (fetch.c:699)
==2195== 
==2195== 
==2195== Process terminating with default action of signal 11 (SIGSEGV)
==2195==  Access not within mapped region at address 0x0
==2195==    at 0x1: ???
==2195==    by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
==2195==    by 0x812BD9F: string_list_insert (string-list.c:57)
==2195==    by 0x8071837: add_existing (fetch.c:570)
==2195==    by 0x810AF96: do_one_ref (refs.c:525)
==2195==    by 0x810BD9E: do_for_each_ref_in_dirs (refs.c:627)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810C303: do_for_each_ref (refs.c:1295)
==2195==    by 0x810C63A: for_each_ref (refs.c:1343)
==2195==    by 0x8073909: fetch_one (fetch.c:699)
==2195==  If you believe this happened as a result of a stack
==2195==  overflow in your program's main thread (unlikely but
==2195==  possible), you can try to increase the size of the
==2195==  main thread stack using the --main-stacksize= flag.
==2195==  The main thread stack size used in this run was 10485760.
==2195== 
==2195== HEAP SUMMARY:
==2195==     in use at exit: 325,604 bytes in 3,579 blocks
==2195==   total heap usage: 3,687 allocs, 108 frees, 518,058 bytes allocated
==2195== 
==2195== LEAK SUMMARY:
==2195==    definitely lost: 72 bytes in 2 blocks
==2195==    indirectly lost: 0 bytes in 0 blocks
==2195==      possibly lost: 31,347 bytes in 460 blocks
==2195==    still reachable: 294,185 bytes in 3,117 blocks
==2195==         suppressed: 0 bytes in 0 blocks
==2195== Rerun with --leak-check=full to see details of leaked memory
==2195== 
==2195== For counts of detected and suppressed errors, rerun with: -v
==2195== Use --track-origins=yes to see where uninitialised values come from
==2195== ERROR SUMMARY: 3 errors from 2 contexts (suppressed: 17 from 8)
Segmentation fault

Re: Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Thu, Jan 31, 2013 at 07:27:04AM +0000, Jongman Heo wrote:

> FYI, gdb backtrace and valgrind output attached below, Thanks.

Thanks, that's helpful.

> #4  0x0812bda0 in string_list_insert (list=0xbfffe7c0, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:57
> #5  0x08071838 in add_existing (refname=0x821ec3c "refs/remotes/origin/HEAD", 
>     sha1=0x821ec14 "\a\fW\337B\352N\255\314C\320Em\021E`\022C&", <incomplete sequence \303>, flag=1, cbdata=0xbfffe7c0)
>     at builtin/fetch.c:570

So we are inserting the string from add_existing, which gets the list
from a callback parameter. Which comes from...

> #13 0x0807390a in do_fetch (remote=<value optimized out>, argc=0, argv=0xbfffe9f8) at builtin/fetch.c:699

...here, which does this:

  struct string_list existing_refs = STRING_LIST_INIT_NODUP;
  [...]
  for_each_ref(add_existing, &existing_refs);

And yet we get:

> ==2195== Conditional jump or move depends on uninitialised value(s)
> ==2195==    at 0x812B41F: get_entry_index (string-list.c:10)
> ==2195==    by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
> ==2195==    by 0x812BD9F: string_list_insert (string-list.c:57)
> ==2195==    by 0x8071837: add_existing (fetch.c:570)
> ==2195==    by 0x810AF96: do_one_ref (refs.c:525)
> ==2195==    by 0x810BB20: do_for_each_ref_in_dir (refs.c:551)
> ==2195==    by 0x810BD34: do_for_each_ref_in_dirs (refs.c:623)
> ==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
> ==2195==    by 0x810C303: do_for_each_ref (refs.c:1295)
> ==2195==    by 0x810C63A: for_each_ref (refs.c:1343)
> ==2195==    by 0x8073909: fetch_one (fetch.c:699)
> ==2195==    by 0x8074250: cmd_fetch (fetch.c:992)

which seems odd. cmp should be initialized to NULL, and then we never
touch it (and even if we did, it wouldn't be unitialized, but rather
have the value we put in it).

It's almost like the compiler is getting the initializer wrong. It's a
long shot, but I wonder if the presence of the bitfield could be
triggering a compiler bug (or there is a subtle C rule about bitfield
initializations that I do not know). Just for the sake of my sanity,
what does the following program output for you?

-- >8 --
#include <stdio.h>
#include <stdlib.h>

typedef int (*compare_fn)(const char *, const char *);

struct foo {
  char **items;
  unsigned int nr, alloc;
  unsigned int bitfield:1;
  compare_fn cmp;
};

int main(void)
{
  struct foo f = { NULL, 0, 0, 0 };
  printf("cmp is %lu\n", (unsigned long)f.cmp);
  return 0;
}

Re: Segmentation fault with latest git (070c57df)

[Thomas Rast]

Jeff King <peff@peff.net> writes:

> On Thu, Jan 31, 2013 at 07:27:04AM +0000, Jongman Heo wrote:
>
>> FYI, gdb backtrace and valgrind output attached below, Thanks.
>
> Thanks, that's helpful.
>
>> #4 0x0812bda0 in string_list_insert (list=0xbfffe7c0,
>> string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:57
>> #5  0x08071838 in add_existing (refname=0x821ec3c "refs/remotes/origin/HEAD", 
>>     sha1=0x821ec14 "\a\fW\337B\352N\255\314C\320Em\021E`\022C&",
>> <incomplete sequence \303>, flag=1, cbdata=0xbfffe7c0)
>>     at builtin/fetch.c:570
>
> So we are inserting the string from add_existing, which gets the list
> from a callback parameter. Which comes from...
>
>> #13 0x0807390a in do_fetch (remote=<value optimized out>, argc=0,
>> argv=0xbfffe9f8) at builtin/fetch.c:699
>
> ...here, which does this:
>
>   struct string_list existing_refs = STRING_LIST_INIT_NODUP;
>   [...]
>   for_each_ref(add_existing, &existing_refs);
>
> And yet we get:
>
>> ==2195== Conditional jump or move depends on uninitialised value(s)
>> ==2195==    at 0x812B41F: get_entry_index (string-list.c:10)
>> ==2195==    by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
>> ==2195==    by 0x812BD9F: string_list_insert (string-list.c:57)
>> ==2195==    by 0x8071837: add_existing (fetch.c:570)
>> ==2195==    by 0x810AF96: do_one_ref (refs.c:525)
>> ==2195==    by 0x810BB20: do_for_each_ref_in_dir (refs.c:551)
>> ==2195==    by 0x810BD34: do_for_each_ref_in_dirs (refs.c:623)
>> ==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
>> ==2195==    by 0x810C303: do_for_each_ref (refs.c:1295)
>> ==2195==    by 0x810C63A: for_each_ref (refs.c:1343)
>> ==2195==    by 0x8073909: fetch_one (fetch.c:699)
>> ==2195==    by 0x8074250: cmd_fetch (fetch.c:992)
>
> which seems odd. cmp should be initialized to NULL, and then we never
> touch it (and even if we did, it wouldn't be unitialized, but rather
> have the value we put in it).
>
> It's almost like the compiler is getting the initializer wrong. It's a
> long shot, but I wonder if the presence of the bitfield could be
> triggering a compiler bug (or there is a subtle C rule about bitfield
> initializations that I do not know). Just for the sake of my sanity,
> what does the following program output for you?
>
> -- >8 --
> #include <stdio.h>
> #include <stdlib.h>
>
> typedef int (*compare_fn)(const char *, const char *);
>
> struct foo {
>   char **items;
>   unsigned int nr, alloc;
>   unsigned int bitfield:1;
>   compare_fn cmp;
> };
>
> int main(void)
> {
>   struct foo f = { NULL, 0, 0, 0 };
>   printf("cmp is %lu\n", (unsigned long)f.cmp);
>   return 0;
> }

I doubt that would help because that stack region would be 0 anyway due
to kernel initialization of new pages.  You'd have to somehow trample
over it first, like below.

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.

---- 8< ----
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef int (*compare_fn)(const char *, const char *);

struct foo {
  char **items;
  unsigned int nr, alloc;
  unsigned int bitfield:1;
  compare_fn cmp;
};

void scramble()
{
  char foo[256];
  memset(foo, 0x42, 256);
}

void init()
{
  struct foo f = { NULL, 0, 0, 0 };
  printf("cmp is %lu\n", (unsigned long)f.cmp);
}

int main(void)
{
  scramble();
  init();
  return 0;
}

Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Thu, Jan 31, 2013 at 09:42:07AM +0100, Thomas Rast wrote:

> > int main(void)
> > {
> >   struct foo f = { NULL, 0, 0, 0 };
> >   printf("cmp is %lu\n", (unsigned long)f.cmp);
> >   return 0;
> > }
> 
> I doubt that would help because that stack region would be 0 anyway due
> to kernel initialization of new pages.  You'd have to somehow trample
> over it first, like below.

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

Re: Re: Re: Re: Re: Segmentation fault with latest git (070c57df)

[Jongman Heo]

Jonathan Nieder wrote:

> Jongman Heo wrote:
>
>>> But it doesn't stimulate any prerequisites in make, which is weird.
>>> What's in builtin/.depend/fetch.o.d?
>[...]
>> please see below~.
>>
>> $ cat builtin/.depend/fetch.o.d            
>> fetch.o: builtin/fetch.c cache.h git-compat-util.h compat/bswap.h \
>
>That's the problem.  See the following thread:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/185625/focus=185680
>
>Currently when COMPUTE_HEADER_DEPENDENCIES=auto git tests for
>dependency generation support by checking the output and exit status
>from the following command:
>
>$(CC) $(ALL_CFLAGS) -c -MF /dev/null -MMD -MP \
>-x c /dev/null -o /dev/null 2>&1
>
>Perhaps this can be improved?  Even something as simple as a ccache
>version test could presumably help a lot.
>
>Hope that helps,
>Jonathan

Hi,

Unfortunately, the patch didn't help to me.
Anyway, ccache is the culprit (I'm using ccache 2.4 version).

If I disable ccache using CCACHE_DISABLE=1, then the issue doesn't happen. Thanks.

Best regards,
Jongman Heo.

Re: Segmentation fault with latest git (070c57df)

[Jonathan Nieder]

Jongman Heo wrote:

> Unfortunately, the patch didn't help to me.

Thanks for testing.  Did you apply the patch to the older version of
git that generates builtin/.depend/fetch.o.d or the newer version that
consumes it?

Curious,
Jonathan

Re: Re: Segmentation fault with latest git (070c57df)

[허종만]

> 
[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.

Re: Segmentation fault with latest git (070c57df)

[Junio C Hamano]

허종만 <jongman.heo@samsung.com> 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.

Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Thu, Jan 31, 2013 at 05:40:02PM -0800, Junio C Hamano wrote:

> 허종만 <jongman.heo@samsung.com> 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

Re: Segmentation fault with latest git (070c57df)

[Jeff King]

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

Re: Segmentation fault with latest git (070c57df)

[Jeff King]

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:
> 
> > 허종만 <jongman.heo@samsung.com> 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 <peff@peff.net>
---
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

Segmentation fault with latest git (070c57df)

[Jongman Heo]

Hi all,

Looks like following commit causes a segmentation fault in my machine (when running git pull or git fetch);

commit 8dd5afc926acb9829ebf56e9b78826a5242cd638
Author: Junio C Hamano <gitster@pobox.com>
Date:   Mon Jan 7 12:24:55 2013 -0800

    string-list: allow case-insensitive string list


In my case, list->cmp (at get_entry_index() function) has an invalid address, obviously not an address of string comparision function, instead it points to 1.


Regards,
Jongman Heo.

Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Thu, Jan 31, 2013 at 01:35:21AM +0000, Jongman Heo wrote:

> Looks like following commit causes a segmentation fault in my machine
> (when running git pull or git fetch);
> 
> commit 8dd5afc926acb9829ebf56e9b78826a5242cd638
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Mon Jan 7 12:24:55 2013 -0800
> 
>     string-list: allow case-insensitive string list
> 
> 
> In my case, list->cmp (at get_entry_index() function) has an invalid
> address, obviously not an address of string comparision function,
> instead it points to 1.

Can you show us a stack trace? The string-list functions are generic and
get called in a lot of places. It would be useful to know which list is
causing the problem.

-Peff

Re: Segmentation fault with latest git (070c57df)

[Antoine Pelisse]

In "clean.c" we have a "string_list" created on the stack with
"STRING_LIST_INIT_NODUP" (there are probably others, I stopped at the
first occurrence).
But, "STRING_LIST_INIT_NODUP" doesn't init the "list->cmp" pointer
which can thus be random.

I don't have much time to provide a patch right now (have to go to
work), but will tonight if still needed.

Antoine,



On Thu, Jan 31, 2013 at 7:49 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 31, 2013 at 01:35:21AM +0000, Jongman Heo wrote:
>
>> Looks like following commit causes a segmentation fault in my machine
>> (when running git pull or git fetch);
>>
>> commit 8dd5afc926acb9829ebf56e9b78826a5242cd638
>> Author: Junio C Hamano <gitster@pobox.com>
>> Date:   Mon Jan 7 12:24:55 2013 -0800
>>
>>     string-list: allow case-insensitive string list
>>
>>
>> In my case, list->cmp (at get_entry_index() function) has an invalid
>> address, obviously not an address of string comparision function,
>> instead it points to 1.
>
> Can you show us a stack trace? The string-list functions are generic and
> get called in a lot of places. It would be useful to know which list is
> causing the problem.
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Thu, Jan 31, 2013 at 08:02:06AM +0100, Antoine Pelisse wrote:

> In "clean.c" we have a "string_list" created on the stack with
> "STRING_LIST_INIT_NODUP" (there are probably others, I stopped at the
> first occurrence).
> But, "STRING_LIST_INIT_NODUP" doesn't init the "list->cmp" pointer
> which can thus be random.

I don't think that is the problem. Extra struct members that are not
mentioned by an initializer will get set to 0 or NULL.

A code path that tried to initialize each member individually would run
into problems, but I could not find any such code path (all of them
either use an initializer, are static, or memset the struct to
all-zeroes).

-Peff

Re: Re: Segmentation fault with latest git (070c57df)

[Jongman Heo]

Jonathan Nieder wrote:
> Jongman Heo wrote:
> 
>> Unfortunately, the patch didn't help to me.
>
>Thanks for testing.  Did you apply the patch to the older version of
>git that generates builtin/.depend/fetch.o.d or the newer version that
>consumes it?
>
>Curious,
>Jonathan

Hi, Jonathan,

I applied the patch to newer version. 

This time, I tried to apply the patch to older version of Makefile, and now the issue is fixed~. 
Thanks~!

Best regards,
Jongman Heo

Re: Segmentation fault with latest git (070c57df)

[Junio C Hamano]

Jongman Heo <jongman.heo@samsung.com> writes:

> Jonathan Nieder wrote:
>> Jongman Heo wrote:
>> 
>>> Unfortunately, the patch didn't help to me.
>>
>>Thanks for testing.  Did you apply the patch to the older version of
>>git that generates builtin/.depend/fetch.o.d or the newer version that
>>consumes it?
>>
>>Curious,
>>Jonathan
>
> Hi, Jonathan,
>
> I applied the patch to newer version. 
>
> This time, I tried to apply the patch to older version of Makefile, and now the issue is fixed~. 
> Thanks~!

Yeah, that result is understandable, as .depend/*.o.d files will not
be rebuilt when the rules to build them changes in the Makefile.
Applying the patch to the Makefile in the pristine old tree, run the
build (which will generate .depend/*.o.d files with the corrected
rules), then checking out the new tree and running the build again
without "make clean", with or with the patch applied, would validate
that the patch fixes the issue for old ccache.

Thanks Jonathan for diagnosing, fixing, and thanks Jongman for
testing.

Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Sun, Feb 03, 2013 at 11:13:00PM -0800, Junio C Hamano wrote:

> Yeah, that result is understandable, as .depend/*.o.d files will not
> be rebuilt when the rules to build them changes in the Makefile.
> Applying the patch to the Makefile in the pristine old tree, run the
> build (which will generate .depend/*.o.d files with the corrected
> rules), then checking out the new tree and running the build again
> without "make clean", with or with the patch applied, would validate
> that the patch fixes the issue for old ccache.
> 
> Thanks Jonathan for diagnosing, fixing, and thanks Jongman for
> testing.

Do we want to do anything with the other dependency hole I found here:

  http://article.gmane.org/gmane.comp.version-control.git/215211

It's definitely a potential problem, but I don't think we have any
reports of it happening in practice, so it might not be worth worrying
about. Doing a clean version of the fix here:

  http://article.gmane.org/gmane.comp.version-control.git/215212

would probably involve reorganizing our .depend directory structure,
unless somebody can cook up some clever use of make's patsubst.

-Peff

Re: Segmentation fault with latest git (070c57df)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Do we want to do anything with the other dependency hole I found here:
>
>   http://article.gmane.org/gmane.comp.version-control.git/215211
>
> It's definitely a potential problem, but I don't think we have any
> reports of it happening in practice, so it might not be worth worrying
> about. Doing a clean version of the fix here:
>
>   http://article.gmane.org/gmane.comp.version-control.git/215212
>
> would probably involve reorganizing our .depend directory structure,
> unless somebody can cook up some clever use of make's patsubst.

As I understand how the current set-up works:

 * Initially, we do not have foo.o but foo.c.  We automatically
   build foo.o because it depends on foo.c via the "%.o : %.c" rule,
   and as a side effect, we also build .depend/foo.o.d file;

 * Then, if any real dependency used to build the existing foo.o
   that is recorded in .depend/foo.o.d file changes, foo.o gets
   rebuilt, which would update .depend/foo.o.d again for the next
   invocation.

The case where you lose .depend/foo.o.d is a special case of getting
a wrong information in .depend/foo.o.d, which may happen by using a
broken compiler during the initial build, or going over quota and
getting .depend/foo.o.d truncated, or by other breakages.  The user
may have done "rm -rf .depend" to lose it, or the user may have done
something like this to munge it:

	find -name '.git' -type d -prune -o -print0 |
        xargs -0 sed -i -e 's/foo/bar/g'

forgetting that just like .git, .depend is precious and should not
be touched.

I think this really boils down to where we draw the "this is good
enough" line.  I am not sure if losing the file as in $gmane/215211
is common enough to be special cased to buy us much, while leaving
other ".depend/foo.o.d was updated to contain a wrong info" cases
still broken.

And of course the case where .depend/foo.o.d is munged by mistake
cannot be solved without recompiling everything all the time, so...

Re: Segmentation fault with latest git (070c57df)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> As I understand how the current set-up works:
>
>  * Initially, we do not have foo.o but foo.c.  We automatically
>    build foo.o because it depends on foo.c via the "%.o : %.c" rule,
>    and as a side effect, we also build .depend/foo.o.d file;
>
>  * Then, if any real dependency used to build the existing foo.o
>    that is recorded in .depend/foo.o.d file changes, foo.o gets
>    rebuilt, which would update .depend/foo.o.d again for the next
>    invocation.

This is unrelated to the case you mentioned, but I wonder what
happens if you did this:

 * You are on branch 'next', where foo.c includes (perhaps
   indirectly) frotz.h.  Compile and you get foo.o and also the
   dependency recorded for it, "foo.o: foo.c frotz.h", in the
   .depend/foo.o.d file.

 * You check out branch 'master', where foo.c does not include
   frotz.h.  Indeed, the include file does not even exist on the
   branch.

Do we get confused, because Makefile includes the depend file from
the previous build, finds that you need foo.c and frotz.h up to date
in order to get foo.o, but there is no rule to generate frotz.h?

Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Mon, Feb 04, 2013 at 01:29:41AM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > As I understand how the current set-up works:
> >
> >  * Initially, we do not have foo.o but foo.c.  We automatically
> >    build foo.o because it depends on foo.c via the "%.o : %.c" rule,
> >    and as a side effect, we also build .depend/foo.o.d file;
> >
> >  * Then, if any real dependency used to build the existing foo.o
> >    that is recorded in .depend/foo.o.d file changes, foo.o gets
> >    rebuilt, which would update .depend/foo.o.d again for the next
> >    invocation.
> 
> This is unrelated to the case you mentioned, but I wonder what
> happens if you did this:
> 
>  * You are on branch 'next', where foo.c includes (perhaps
>    indirectly) frotz.h.  Compile and you get foo.o and also the
>    dependency recorded for it, "foo.o: foo.c frotz.h", in the
>    .depend/foo.o.d file.
> 
>  * You check out branch 'master', where foo.c does not include
>    frotz.h.  Indeed, the include file does not even exist on the
>    branch.
> 
> Do we get confused, because Makefile includes the depend file from
> the previous build, finds that you need foo.c and frotz.h up to date
> in order to get foo.o, but there is no rule to generate frotz.h?

No, because the .d files look like this:

  foo.o: frotz.h

  frotz.h:

So make sees that it can build frotz.h, which of course does nothing.
But that's OK, because foo.c doesn't actually include it anymore, and
when we recompile it (as we must, since it is different between the two
branches), we will rewrite the .d file without frotz.h.

-Peff

Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Mon, Feb 04, 2013 at 01:16:08AM -0800, Junio C Hamano wrote:

> I think this really boils down to where we draw the "this is good
> enough" line.  I am not sure if losing the file as in $gmane/215211
> is common enough to be special cased to buy us much, while leaving
> other ".depend/foo.o.d was updated to contain a wrong info" cases
> still broken.

Hmm. Yeah, I was thinking it might be more common than ordinary munging
due to something like an interrupted "git clean -x". But given that:

  1. As far as I can tell, it is not a situation that can happen through
     regular use of checkout/make/etc, and...

  2. We have zero reports of it happening in practice (I only discovered
     it while explicitly trying to break the Makefile), and...

  3. It is just one of many possible breakages, all of which can be
     fixed by "git clean -dx" if you suspect issues...

let's just leave it. Thanks for a sanity check.

-Peff

Re: Segmentation fault with latest git (070c57df)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 04, 2013 at 01:16:08AM -0800, Junio C Hamano wrote:
>
>> I think this really boils down to where we draw the "this is good
>> enough" line.  I am not sure if losing the file as in $gmane/215211
>> is common enough to be special cased to buy us much, while leaving
>> other ".depend/foo.o.d was updated to contain a wrong info" cases
>> still broken.
>
> Hmm. Yeah, I was thinking it might be more common than ordinary munging
> due to something like an interrupted "git clean -x". But given that:
>
>   1. As far as I can tell, it is not a situation that can happen through
>      regular use of checkout/make/etc, and...
>
>   2. We have zero reports of it happening in practice (I only discovered
>      it while explicitly trying to break the Makefile), and...
>
>   3. It is just one of many possible breakages, all of which can be
>      fixed by "git clean -dx" if you suspect issues...
>
> let's just leave it. Thanks for a sanity check.

The only case that worries me is when make or cc gets interrupted.
As long as make removes the ultimate target *.o in such a case, it
is fine to leave a half-written .depend/foo.o.d (or getting it
removed) behind.

How expensive to generate the dependency for a single foo.c file
into a temporary without compiling it into foo.o, and comparing the
resulting dependency with existing .depend/foo.o.d?  If it is very
cheap, it might be worth doing it before linking the final build
product and error the build out when we see discrepancies.  I dunno.

Re: Segmentation fault with latest git (070c57df)

[Jonathan Nieder]

Junio C Hamano wrote:

> The only case that worries me is when make or cc gets interrupted.
> As long as make removes the ultimate target *.o in such a case, it
> is fine to leave a half-written .depend/foo.o.d (or getting it
> removed) behind.

gcc removes the target .o in its signal handler in such a case.  In
cases where it doesn't get a chance to (e.g., sudden power failure),
there is a partially written .o file already in place, the linker
produces errors, and the operator is convinced to run "make clean",
all without .depend's help.

Re: Segmentation fault with latest git (070c57df)

[sam]

Hi,

Has there been any further progress on this. I just encountered a SEGV with
a git apply. This is the latest git version running on Ubuntu 13.04

cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=13.04
DISTRIB_CODENAME=raring
DISTRIB_DESCRIPTION="Ubuntu 13.04"

uname -a
Linux sam-mac 3.8.0-20-generic #31-Ubuntu SMP Mon May 6 17:03:32 UTC 2013
x86_64 x86_64 x86_64 GNU/Linux

git --version
git version 1.8.2.2


samueldoyle@sam-MacBookPro:~/Projects/VMware/perforce_repos/esx50u3ps$ sudo
gdb /usr/bin/git core
GNU gdb (GDB) 7.5.91.20130417-cvs-ubuntu
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
<http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /usr/bin/git...(no debugging symbols found)...done.
[New LWP 28634]

warning: Can't read pathname for load map: Input/output error.
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

warning: no loadable sections found in added symbol-file system-supplied DSO
at 0x7fff94d1a000
Core was generated by `git apply --verbose --check --ignore-whitespace
--directory=/home/sam/P'.
Program terminated with signal 11, Segmentation fault.
#0  __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:31
31	../sysdeps/x86_64/multiarch/../strlen.S: No such file or directory.
(gdb) bt 10
#0  __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:31
#1  0x00007f06ba7c28c6 in __GI___strdup (s=0x0) at strdup.c:41
#2  0x00000000004f4449 in ?? ()
#3  0x000000000040eab0 in ?? ()
#4  0x000000000040f6f5 in ?? ()
#5  0x0000000000405a88 in ?? ()
#6  0x0000000000404ee2 in ?? ()
#7  0x00007f06ba75aea5 in __libc_start_main (main=0x404e30, argc=7,
ubp_av=0x7fff94c3bd08, init=<optimized out>, fini=<optimized out>, 
    rtld_fini=<optimized out>, stack_end=0x7fff94c3bcf8) at libc-start.c:260
#8  0x0000000000405329 in ?? ()


A quick search turned up this post that appears to provide the cause:
http://stackoverflow.com/questions/3608931/why-is-this-program-segfaulting



--
View this message in context: http://git.661346.n2.nabble.com/Segmentation-fault-with-latest-git-070c57df-tp7576614p7585906.html
Sent from the git mailing list archive at Nabble.com.

Re: Segmentation fault with latest git (070c57df)

[sam]

Hmmm nabble embed didn't provide much value there :)

http://pastebin.com/RC8mUPF3



--
View this message in context: http://git.661346.n2.nabble.com/Segmentation-fault-with-latest-git-070c57df-tp7576614p7585907.html
Sent from the git mailing list archive at Nabble.com.

Re: Segmentation fault with latest git (070c57df)

[Ramkumar Ramachandra]

sam wrote:
> gdb /usr/bin/git core

What exactly are you doing?  Is core aliased to something?

> [...]
> Core was generated by `git apply --verbose --check --ignore-whitespace
> --directory=/home/sam/P'.

What is this?

Can you give us clear instructions on how to reproduce the segfault?

Re: Segmentation fault with latest git (070c57df)

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> sam wrote:
>> gdb /usr/bin/git core
>
> What exactly are you doing?  Is core aliased to something?

Sorry about that.  I just realized you're loading a core dump.

Please tell us how to reproduce this, or give us the backtrace with
debugging symbols.