bug#9983: valgrind warning in draw_glyphs

bug#9983: valgrind warning in draw_glyphs

[Dan Nicolaescu]

trunk with lucid toolkit
valgrind ./temacs -Q
gives this warning (not sure exactly when, just playing with the menus
and tooltips, the only "editing" action was to run a grep)





==2341== Conditional jump or move depends on uninitialised value(s)
==2341==    at 0x44CDDA: draw_glyphs (xdisp.c:22981)
==2341==    by 0x44D9FC: expose_area (xdisp.c:27401)
==2341==    by 0x44DDA8: expose_line (xdisp.c:27426)
==2341==    by 0x45B6AA: expose_window (xdisp.c:27653)
==2341==    by 0x45C0DA: expose_window_tree (xdisp.c:27725)
==2341==    by 0x45C0B0: expose_window_tree (xdisp.c:27723)
==2341==    by 0x45C16E: expose_frame (xdisp.c:27780)
==2341==    by 0x4C188D: handle_one_xevent (xterm.c:6219)
==2341==    by 0x4C2F56: XTread_socket (xterm.c:7148)
==2341==    by 0x4F2F26: read_avail_input (keyboard.c:6821)
==2341==    by 0x4F3049: handle_async_input (keyboard.c:7149)
==2341==    by 0x4F26A4: process_pending_signals (keyboard.c:7165)
==2341==  Uninitialised value was created by a stack allocation
==2341==    at 0x44AAD3: draw_glyphs (xdisp.c:22835)
==2341== 


The warning is for this:
        if (check_mouse_face
              && mouse_beg_col < start && mouse_end_col > i)

it looks like mouse_beg_col and mouse_end_col could be left uninitialized a few lines above.

bug#9983: valgrind warning in draw_glyphs

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Date: Sun, 06 Nov 2011 23:36:42 -0500
> 
> The warning is for this:
>         if (check_mouse_face
>               && mouse_beg_col < start && mouse_end_col > i)
> 
> it looks like mouse_beg_col and mouse_end_col could be left uninitialized a few lines above.

I don't see how.  These variables are initialized in this block:

	  if (row >= mouse_beg_row && row <= mouse_end_row)
	    {
	      check_mouse_face = 1;
	      mouse_beg_col = (row == mouse_beg_row)
		? hlinfo->mouse_face_beg_col : 0;
	      mouse_end_col = (row == mouse_end_row)
		? hlinfo->mouse_face_end_col
		: row->used[TEXT_AREA];
	    }

check_mouse_face starts as zero, and is only set to 1 in this block.
So any test that is conditioned on check_mouse_face being non-zero is
okay with looking at mouse_beg_col and mouse_end_col.

The other variables in the line being flagged, `start' and `i', are
also okay: `start' is one of the call arguments and `i' is computed
right before the line being flagged.

Did I miss something?

bug#9983: valgrind warning in draw_glyphs

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Date: Sun, 06 Nov 2011 23:36:42 -0500
>> 
>> The warning is for this:
>>         if (check_mouse_face
>>               && mouse_beg_col < start && mouse_end_col > i)
>> 
>> it looks like mouse_beg_col and mouse_end_col could be left uninitialized a few lines above.
>
> I don't see how.  These variables are initialized in this block:
>
> 	  if (row >= mouse_beg_row && row <= mouse_end_row)
> 	    {
> 	      check_mouse_face = 1;
> 	      mouse_beg_col = (row == mouse_beg_row)
> 		? hlinfo->mouse_face_beg_col : 0;
> 	      mouse_end_col = (row == mouse_end_row)
> 		? hlinfo->mouse_face_end_col
> 		: row->used[TEXT_AREA];
> 	    }
>
> check_mouse_face starts as zero, and is only set to 1 in this block.
> So any test that is conditioned on check_mouse_face being non-zero is
> okay with looking at mouse_beg_col and mouse_end_col.
>
> The other variables in the line being flagged, `start' and `i', are
> also okay: `start' is one of the call arguments and `i' is computed
> right before the line being flagged.
>
> Did I miss something?

Hmm, you might be right.  Telling valgrind to attach gdb at that point:

(gdb) info local
overlap_hl = <optimized out>
hlinfo = <optimized out>
mouse_beg_col = 0x0
check_mouse_face = 0x0
dummy_x = 0x0
h = <optimized out>
t = <optimized out>
mouse_end_col = 0x50b3a22
head = 0x7feffe3a0
tail = 0x7feffe3a0
s = <optimized out>
clip_head = 0x0
clip_tail = 0x0
i = <optimized out>
j = <optimized out>
x_reached = 0x184
last_x = 0x2ec
area_left = 0x1c
f = 0x5157bf0

Maybe a compiler problem, evaluating the && in the wrong order?

bug#9983: valgrind warning in draw_glyphs

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> The other variables in the line being flagged, `start' and `i', are
> also okay: `start' is one of the call arguments

Just being an argument does not necessarily make it ok.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#9983: valgrind warning in draw_glyphs

[Dan Nicolaescu]

Dan Nicolaescu <dann@gnu.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Dan Nicolaescu <dann@gnu.org>
>>> Date: Sun, 06 Nov 2011 23:36:42 -0500
>>> 
>>> The warning is for this:
>>>         if (check_mouse_face
>>>               && mouse_beg_col < start && mouse_end_col > i)
>>> 
>>> it looks like mouse_beg_col and mouse_end_col could be left uninitialized a few lines above.
>>
>> I don't see how.  These variables are initialized in this block:
>>
>> 	  if (row >= mouse_beg_row && row <= mouse_end_row)
>> 	    {
>> 	      check_mouse_face = 1;
>> 	      mouse_beg_col = (row == mouse_beg_row)
>> 		? hlinfo->mouse_face_beg_col : 0;
>> 	      mouse_end_col = (row == mouse_end_row)
>> 		? hlinfo->mouse_face_end_col
>> 		: row->used[TEXT_AREA];
>> 	    }
>>
>> check_mouse_face starts as zero, and is only set to 1 in this block.
>> So any test that is conditioned on check_mouse_face being non-zero is
>> okay with looking at mouse_beg_col and mouse_end_col.
>>
>> The other variables in the line being flagged, `start' and `i', are
>> also okay: `start' is one of the call arguments and `i' is computed
>> right before the line being flagged.
>>
>> Did I miss something?
>
> Hmm, you might be right.  Telling valgrind to attach gdb at that point:
>
> (gdb) info local
> overlap_hl = <optimized out>
> hlinfo = <optimized out>
> mouse_beg_col = 0x0
> check_mouse_face = 0x0
> dummy_x = 0x0
> h = <optimized out>
> t = <optimized out>
> mouse_end_col = 0x50b3a22
> head = 0x7feffe3a0
> tail = 0x7feffe3a0
> s = <optimized out>
> clip_head = 0x0
> clip_tail = 0x0
> i = <optimized out>
> j = <optimized out>
> x_reached = 0x184
> last_x = 0x2ec
> area_left = 0x1c
> f = 0x5157bf0
>
> Maybe a compiler problem, evaluating the && in the wrong order?

The assembly in question looks like this:

.LBB3664:
        .loc 1 22981 0
        movl -152(%rbp), %ebx
        cmpl %ebx, -260(%rbp)
        jge  .L7265
        movl -192(%rbp), %eax
        testl            %eax, %eax
        je               .L7265
        .loc 1 22982 0
        movslq -232(%rbp), %rax
        .loc 1 22985 0
        xorl %r14d, %r14d
        cmpq %rax, -120(%rbp)
        setl %r14b
        leal (%r14,%r14,2), %ebx
        movl %ebx, -144(%rbp)

the debugger is stopped on the "jge" line, that means that the "cmpl"
lined did the access.

(gdb) p/d (long long)&mouse_beg_col - (long long)$rbp
$18 = -260
(gdb) p/d (long long)&check_mouse_face - (long long)$rbp
$19 = -192

so it looks like check_mouse_face is tested after mouse_beg_col.
(unless I'm missing something...)

The compiler here is the system compiler on an up to date Fedora15 system.
 gcc --version
gcc (GCC) 4.6.1 20110908 (Red Hat 4.6.1-9)
Copyright (C) 2011 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

bug#9983: valgrind warning in draw_glyphs

[Andreas Schwab]

Dan Nicolaescu <dann@gnu.org> writes:

> so it looks like check_mouse_face is tested after mouse_beg_col.

That is perfectly ok under the as-if rule.  See also
<http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50975>.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#9983: valgrind warning in draw_glyphs

[Dan Nicolaescu]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Dan Nicolaescu <dann@gnu.org> writes:
>
>> so it looks like check_mouse_face is tested after mouse_beg_col.
>
> That is perfectly ok under the as-if rule.  See also

Right.  Closing this bug then.

bug#9983: valgrind warning in draw_glyphs

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  9983@debbugs.gnu.org
> Date: Mon, 07 Nov 2011 13:59:37 +0100
> 
> Dan Nicolaescu <dann@gnu.org> writes:
> 
> > so it looks like check_mouse_face is tested after mouse_beg_col.
> 
> That is perfectly ok under the as-if rule.  See also
> <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50975>.

Where can I find the definition of the "side effects" that would
disallow such optimizations?

bug#9983: valgrind warning in draw_glyphs

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Dan Nicolaescu <dann@gnu.org>,  9983@debbugs.gnu.org
> Date: Mon, 07 Nov 2011 13:21:38 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The other variables in the line being flagged, `start' and `i', are
> > also okay: `start' is one of the call arguments
> 
> Just being an argument does not necessarily make it ok.

Indeed.  I also thought about that, so I checked the callers two
levels up, and came up empty-handed.

I guess the optimization done by GCC 4.6 fooled valgrind as well.

bug#9983: valgrind warning in draw_glyphs

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andreas Schwab <schwab@linux-m68k.org>
>> Cc: Dan Nicolaescu <dann@gnu.org>,  9983@debbugs.gnu.org
>> Date: Mon, 07 Nov 2011 13:21:38 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > The other variables in the line being flagged, `start' and `i', are
>> > also okay: `start' is one of the call arguments
>> 
>> Just being an argument does not necessarily make it ok.
>
> Indeed.  I also thought about that, so I checked the callers two
> levels up, and came up empty-handed.
>
> I guess the optimization done by GCC 4.6 fooled valgrind as well.

Not trying to split hairs, but valgrind wasn't fooled, it did the right
thing at the level that valgrind is working at...