Skip to contentSkip to conversation

git-blame segfault

DISCUSSION Markus Trippelsdorf 2 participants 5 messages Jun 15, 2016

lore
Share & tools
lore
#0 lore raw

When git is compiled with current gcc and "-march=native" git-blame segfaults:

For example:

% gdb --args /var/tmp/git/git-blame gcc/tree-object-size.c
...
Program received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
(gdb) bt
#0 0x0000000000000000 in ?? ()
#1 0x000000000051240d in xdl_emit_hunk_hdr (s1=s1@entry=30, c1=<optimized out>, s2=s2@entry=30, c2=c2@entry=6, func=func@entry=0x7fffffffd2d8 "", funclen=0,

    ecb=ecb@entry=0x7fffffffd580) at xdiff/xutils.c:460
#2  0x0000000000512af7 in xdl_emit_diff (xe=0x7fffffffd390, xscr=<optimized out>, ecb=0x7fffffffd580, xecfg=0x7fffffffd590) at xdiff/xemit.c:237
#3  0x0000000000510a5d in xdl_diff (mf1=mf1@entry=0x7fffffffd510, mf2=mf2@entry=0x7fffffffd520, xpp=xpp@entry=0x7fffffffd570, xecfg=xecfg@entry=0x7fffffffd590, 
    ecb=ecb@entry=0x7fffffffd580) at xdiff/xdiffi.c:601
#4  0x000000000050b005 in xdi_diff (mf1=<optimized out>, mf2=<optimized out>, xpp=xpp@entry=0x7fffffffd570, xecfg=xecfg@entry=0x7fffffffd590, xecb=xecb@entry=0x7fffffffd580)
    at xdiff-interface.c:136
#5  0x00000000004104df in diff_hunks (file_a=<optimized out>, file_b=<optimized out>, ctxlen=ctxlen@entry=0, hunk_func=hunk_func@entry=0x411320 <blame_chunk_cb>, 
    cb_data=cb_data@entry=0x7fffffffd830) at builtin/blame.c:105
#6  0x0000000000412b54 in pass_blame_to_parent (parent=0x11da810, target=0x11dab50, sb=0x7fffffffd6e0) at builtin/blame.c:815
#7  pass_blame (opt=0, origin=0x11dab50, sb=0x7fffffffd6e0) at builtin/blame.c:1281
#8  assign_blame (opt=<optimized out>, sb=0x7fffffffd6e0) at builtin/blame.c:1559
#9  cmd_blame (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin/blame.c:2523

#10 0x00000000004060b5 in run_builtin (argv=0x7fffffffe528, argc=2, p=0x578bd8 <commands.22612+120>) at git.c:314
#11 handle_internal_command (argc=2, argv=0x7fffffffe528) at git.c:478
#12 0x0000000000405772 in main (argc=2, av=<optimized out>) at git.c:575
(gdb) up
#1 0x000000000051240d in xdl_emit_hunk_hdr (s1=s1@entry=30, c1=<optimized out>, s2=s2@entry=30, c2=c2@entry=6, func=func@entry=0x7fffffffd2d8 "", funclen=0,

    ecb=ecb@entry=0x7fffffffd580) at xdiff/xutils.c:460

460 if (ecb->outf(ecb->priv, &mb, 1) < 0)
(gdb) l
455 }
456 buf[nb++] = '\n';
457

458             mb.ptr = buf;
459             mb.size = nb;
460             if (ecb->outf(ecb->priv, &mb, 1) < 0)

461 return -1;
462
463 return 0;
464 }
(gdb) p *ecb
$1 = {
priv = 0x7fffffffd830,
outf = 0x0
}

If I leave xecfg uninitialized in the following function the issue goes away.

Quoted 1 line

From builtin/blame.c:

94 static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,
95 xdl_emit_hunk_consume_func_t hunk_func, void *cb_data)
96 {

  97         xpparam_t xpp = {0};
  98         xdemitconf_t xecfg = {0};
  99         xdemitcb_t ecb = {NULL};
 100
 101         xpp.flags = xdl_opts;
 102         xecfg.ctxlen = ctxlen;
 103         xecfg.hunk_func = hunk_func;
 104         ecb.priv = cb_data;
 105         return xdi_diff(file_a, file_b, &xpp, &xecfg, &ecb);

106 }
107

I'm not sure if this a git bug or a gcc bug. In any case I've opened a gcc bug-report here: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59363

Signature · 1 line
Markus
#1 lore raw
Quoted 12 lines · Markus Trippelsdorf

When git is compiled with current gcc and "-march=native" git-blame segfaults:

For example:

% gdb --args /var/tmp/git/git-blame gcc/tree-object-size.c
...
Program received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
(gdb) bt
#0 0x0000000000000000 in ?? ()
#1 0x000000000051240d in xdl_emit_hunk_hdr (s1=s1@entry=30, c1=<optimized out>, s2=s2@entry=30, c2=c2@entry=6, func=func@entry=0x7fffffffd2d8 "", funclen=0,

Quoted 2 lines
    ecb=ecb@entry=0x7fffffffd580) at xdiff/xutils.c:460

#2 0x0000000000512af7 in xdl_emit_diff (xe=0x7fffffffd390, xscr=<optimized out>, ecb=0x7fffffffd580, xecfg=0x7fffffffd590) at xdiff/xemit.c:237

xdl_emit_diff() should not be called, because xecfg->hunk_func should be non-null and called instead. xld_emit_diff() is the one that needs outf to be set.

Quoted 2 lines

#3 0x0000000000510a5d in xdl_diff (mf1=mf1@entry=0x7fffffffd510, mf2=mf2@entry=0x7fffffffd520, xpp=xpp@entry=0x7fffffffd570, xecfg=xecfg@entry=0x7fffffffd590,

    ecb=ecb@entry=0x7fffffffd580) at xdiff/xdiffi.c:601

Here we decide that xecfg->hunk_func is empty, and ef is set to xdl_emit_diff.

Quoted 4 lines

#4 0x000000000050b005 in xdi_diff (mf1=<optimized out>, mf2=<optimized out>, xpp=xpp@entry=0x7fffffffd570, xecfg=xecfg@entry=0x7fffffffd590, xecb=xecb@entry=0x7fffffffd580)
at xdiff-interface.c:136
#5 0x00000000004104df in diff_hunks (file_a=<optimized out>, file_b=<optimized out>, ctxlen=ctxlen@entry=0, hunk_func=hunk_func@entry=0x411320 <blame_chunk_cb>,

    cb_data=cb_data@entry=0x7fffffffd830) at builtin/blame.c:105

As we can see in your code below, ecb.outf is not set here, because it's not needed by hunk_func.
hunk_func is set to blame_chunck_cb, passed by the function below.

Quoted 1 line

#6 0x0000000000412b54 in pass_blame_to_parent (parent=0x11da810, target=0x11dab50, sb=0x7fffffffd6e0) at builtin/blame.c:815

diff_hunks() is called with blame_chunck_cb as hunk_func.

I think the best thing to do is to find out where xecfg->hunk_func loses the blame_chunck_cb value to NULL. You could start with a watchpoint.

Quoted 19 lines

[...]
460 if (ecb->outf(ecb->priv, &mb, 1) < 0)
(gdb) l
455 }
456 buf[nb++] = '\n';
457

458             mb.ptr = buf;
459             mb.size = nb;
460             if (ecb->outf(ecb->priv, &mb, 1) < 0)

461 return -1;
462
463 return 0;
464 }
(gdb) p *ecb
$1 = {
priv = 0x7fffffffd830,
outf = 0x0
}

Quoted 2 lines

If I leave xecfg uninitialized in the following function the issue goes away.

Would that mean that gcc is doing some steps in the wrong order ? That is setting xecfg.hunk_func and then emptying the structure ? I've already had a similar bug, but that's very unfortunate.

Quoted 15 lines

From builtin/blame.c:
94 static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,
95 xdl_emit_hunk_consume_func_t hunk_func, void *cb_data)
96 {

  97         xpparam_t xpp = {0};
  98         xdemitconf_t xecfg = {0};
  99         xdemitcb_t ecb = {NULL};
 100
 101         xpp.flags = xdl_opts;
 102         xecfg.ctxlen = ctxlen;
 103         xecfg.hunk_func = hunk_func;
 104         ecb.priv = cb_data;
 105         return xdi_diff(file_a, file_b, &xpp, &xecfg, &ecb);

106 }
107

#2 lore raw
Quoted 35 lines · Antoine Pelisse
Quoted 12 lines · Markus Trippelsdorf

When git is compiled with current gcc and "-march=native" git-blame segfaults:

For example:

% gdb --args /var/tmp/git/git-blame gcc/tree-object-size.c
...
Program received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
(gdb) bt
#0 0x0000000000000000 in ?? ()
#1 0x000000000051240d in xdl_emit_hunk_hdr (s1=s1@entry=30, c1=<optimized out>, s2=s2@entry=30, c2=c2@entry=6, func=func@entry=0x7fffffffd2d8 "", funclen=0,

Quoted 2 lines
    ecb=ecb@entry=0x7fffffffd580) at xdiff/xutils.c:460

#2 0x0000000000512af7 in xdl_emit_diff (xe=0x7fffffffd390, xscr=<optimized out>, ecb=0x7fffffffd580, xecfg=0x7fffffffd590) at xdiff/xemit.c:237

xdl_emit_diff() should not be called, because xecfg->hunk_func should be non-null and called instead. xld_emit_diff() is the one that needs outf to be set.

Quoted 2 lines

#3 0x0000000000510a5d in xdl_diff (mf1=mf1@entry=0x7fffffffd510, mf2=mf2@entry=0x7fffffffd520, xpp=xpp@entry=0x7fffffffd570, xecfg=xecfg@entry=0x7fffffffd590,

    ecb=ecb@entry=0x7fffffffd580) at xdiff/xdiffi.c:601

Here we decide that xecfg->hunk_func is empty, and ef is set to xdl_emit_diff.

Quoted 4 lines

#4 0x000000000050b005 in xdi_diff (mf1=<optimized out>, mf2=<optimized out>, xpp=xpp@entry=0x7fffffffd570, xecfg=xecfg@entry=0x7fffffffd590, xecb=xecb@entry=0x7fffffffd580)
at xdiff-interface.c:136
#5 0x00000000004104df in diff_hunks (file_a=<optimized out>, file_b=<optimized out>, ctxlen=ctxlen@entry=0, hunk_func=hunk_func@entry=0x411320 <blame_chunk_cb>,

    cb_data=cb_data@entry=0x7fffffffd830) at builtin/blame.c:105

As we can see in your code below, ecb.outf is not set here, because it's not needed by hunk_func.
hunk_func is set to blame_chunck_cb, passed by the function below.

Quoted 7 lines
Quoted 1 line

#6 0x0000000000412b54 in pass_blame_to_parent (parent=0x11da810, target=0x11dab50, sb=0x7fffffffd6e0) at builtin/blame.c:815

diff_hunks() is called with blame_chunck_cb as hunk_func.

I think the best thing to do is to find out where xecfg->hunk_func loses the blame_chunck_cb value to NULL. You could start with a watchpoint.

It happens in diff_hunks (from builtin/blame.c):

(gdb) up
#5 0x00000000004104df in diff_hunks (file_a=<optimized out>, file_b=<optimized out>, ctxlen=ctxlen@entry=0, hunk_func=hunk_func@entry=0x411320 <blame_chunk_cb>,

    cb_data=cb_data@entry=0x7fffffffd830) at builtin/blame.c:105
105             return xdi_diff(file_a, file_b, &xpp, &xecfg, &ecb);
(gdb) l
100
101             xpp.flags = xdl_opts;
102             xecfg.ctxlen = ctxlen;
103             xecfg.hunk_func = hunk_func;
104             ecb.priv = cb_data;
105             return xdi_diff(file_a, file_b, &xpp, &xecfg, &ecb);

106 }
107
108 /*
109 * Prepare diff_filespec and convert it using diff textconv API
(gdb) p hunk_func
$1 = (xdl_emit_hunk_consume_func_t) 0x411320 <blame_chunk_cb> (gdb) p xecfg.hunk_func
$2 = (xdl_emit_hunk_consume_func_t) 0x0
(gdb) p xecfg
$3 = {
ctxlen = 0,
interhunkctxlen = 0,
flags = 0,
find_func = 0x0,
find_func_priv = 0x0,
hunk_func = 0x0
}

Quoted 4 lines

Would that mean that gcc is doing some steps in the wrong order ? That is setting xecfg.hunk_func and then emptying the structure ? I've already had a similar bug, but that's very unfortunate.

Yes. I think this might be the case:

(gdb) disass
Dump of assembler code for function diff_hunks:

   0x0000000000410460 <+0>:     sub    $0x58,%rsp
   0x0000000000410464 <+4>:     xor    %eax,%eax
   0x0000000000410466 <+6>:     mov    %eax,%r9d
   0x0000000000410469 <+9>:     add    $0x20,%eax
   0x000000000041046c <+12>:    cmp    $0x20,%eax
   0x000000000041046f <+15>:    movq   $0x0,0x20(%rsp,%r9,1)
   0x0000000000410478 <+24>:    movq   $0x0,0x28(%rsp,%r9,1)
   0x0000000000410481 <+33>:    movq   $0x0,0x30(%rsp,%r9,1)
   0x000000000041048a <+42>:    movq   $0x0,0x38(%rsp,%r9,1)
   0x0000000000410493 <+51>:    jb     0x410466 <diff_hunks+6>
   0x0000000000410495 <+53>:    lea    0x20(%rsp),%r10
   0x000000000041049a <+58>:    mov    %rdx,0x20(%rsp)
   0x000000000041049f <+63>:    mov    %rcx,0x48(%rsp)
   0x00000000004104a4 <+68>:    add    %r10,%rax
   0x00000000004104a7 <+71>:    mov    %r8,0x10(%rsp)
   0x00000000004104ac <+76>:    mov    %rsp,%rdx
   0x00000000004104af <+79>:    movq   $0x0,(%rax)
   0x00000000004104b6 <+86>:    movq   $0x0,0x8(%rax)
   0x00000000004104be <+94>:    lea    0x10(%rsp),%r8
   0x00000000004104c3 <+99>:    movslq 0x171882(%rip),%rax        # 0x581d4c <xdl_opts>
   0x00000000004104ca <+106>:   mov    %r10,%rcx
   0x00000000004104cd <+109>:   movq   $0x0,0x18(%rsp)
   0x00000000004104d6 <+118>:   mov    %rax,(%rsp)
   0x00000000004104da <+122>:   callq  0x50aee0 <xdi_diff>
=> 0x00000000004104df <+127>:   add    $0x58,%rsp
   0x00000000004104e3 <+131>:   retq   

End of assembler dump.

Signature · 1 line
Markus
#3 lore raw
Quoted 35 lines · Markus Trippelsdorf
Quoted 3 lines · Antoine Pelisse

Would that mean that gcc is doing some steps in the wrong order ? That is setting xecfg.hunk_func and then emptying the structure ? I've already had a similar bug, but that's very unfortunate.

Yes. I think this might be the case:

(gdb) disass
Dump of assembler code for function diff_hunks:

   0x0000000000410460 <+0>:     sub    $0x58,%rsp
   0x0000000000410464 <+4>:     xor    %eax,%eax
   0x0000000000410466 <+6>:     mov    %eax,%r9d
   0x0000000000410469 <+9>:     add    $0x20,%eax
   0x000000000041046c <+12>:    cmp    $0x20,%eax
   0x000000000041046f <+15>:    movq   $0x0,0x20(%rsp,%r9,1)
   0x0000000000410478 <+24>:    movq   $0x0,0x28(%rsp,%r9,1)
   0x0000000000410481 <+33>:    movq   $0x0,0x30(%rsp,%r9,1)
   0x000000000041048a <+42>:    movq   $0x0,0x38(%rsp,%r9,1)
   0x0000000000410493 <+51>:    jb     0x410466 <diff_hunks+6>
   0x0000000000410495 <+53>:    lea    0x20(%rsp),%r10
   0x000000000041049a <+58>:    mov    %rdx,0x20(%rsp)
   0x000000000041049f <+63>:    mov    %rcx,0x48(%rsp)
   0x00000000004104a4 <+68>:    add    %r10,%rax
   0x00000000004104a7 <+71>:    mov    %r8,0x10(%rsp)
   0x00000000004104ac <+76>:    mov    %rsp,%rdx
   0x00000000004104af <+79>:    movq   $0x0,(%rax)
   0x00000000004104b6 <+86>:    movq   $0x0,0x8(%rax)
   0x00000000004104be <+94>:    lea    0x10(%rsp),%r8
   0x00000000004104c3 <+99>:    movslq 0x171882(%rip),%rax        # 0x581d4c <xdl_opts>
   0x00000000004104ca <+106>:   mov    %r10,%rcx
   0x00000000004104cd <+109>:   movq   $0x0,0x18(%rsp)
   0x00000000004104d6 <+118>:   mov    %rax,(%rsp)
   0x00000000004104da <+122>:   callq  0x50aee0 <xdi_diff>
=> 0x00000000004104df <+127>:   add    $0x58,%rsp
   0x00000000004104e3 <+131>:   retq   
Quoted 1 line

End of assembler dump.

Should be fixed in gcc soon. For the curious, here is the assembler diff (bad vs. good):

.type diff_hunks, @function
diff_hunks:
.LFB104:

        .cfi_startproc
        subq    $88, %rsp
        .cfi_def_cfa_offset 96
        xorl    %eax, %eax
 .L31:
        movl    %eax, %r9d
        addl    $32, %eax
        cmpl    $32, %eax
        movq    $0, 32(%rsp,%r9)
        movq    $0, 40(%rsp,%r9)
        movq    $0, 48(%rsp,%r9)
        movq    $0, 56(%rsp,%r9)
        jb      .L31
        leaq    32(%rsp), %r10
        movq    %rdx, 32(%rsp)
-       movq    %rcx, 72(%rsp)
-       addq    %r10, %rax
        movq    %r8, 16(%rsp)
+       addq    %r10, %rax
+       leaq    16(%rsp), %r8
        movq    %rsp, %rdx
-       movq    $0, (%rax)
        movq    $0, 8(%rax)
-       leaq    16(%rsp), %r8
+       movq    $0, (%rax)
        movslq  xdl_opts(%rip), %rax
+       movq    %rcx, 72(%rsp)
        movq    %r10, %rcx
        movq    $0, 24(%rsp)
        movq    %rax, (%rsp)
        call    xdi_diff
        addq    $88, %rsp

.cfi_def_cfa_offset 8

Signature · 1 line
Markus
#4 lore raw
Quoted 2 lines · Markus Trippelsdorf

Should be fixed in gcc soon. For the curious, here is the assembler diff (bad vs. good):

Cool, Thanks. Good to know this has nothing to do with Git :-)