GNU bug report logs - #9990
valgrind warning in add_row_entry

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9990 in the body.
You can then email your comments to 9990 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dan Nicolaescu <dann <at> gnu.org>
To: bug-gnu-emacs <at> gnu.org
Subject: valgrind warning in add_row_entry
Date: Tue, 08 Nov 2011 09:27:06 -0500

valgrind ./temacs -Q gets this warning:

==7487== Use of uninitialised value of size 8
==7487==    at 0x4140F4: update_window (dispnew.c:4212)
==7487==    by 0x414F32: update_window_tree (dispnew.c:3326)
==7487==    by 0x414F0E: update_window_tree (dispnew.c:3324)
==7487==    by 0x4181FD: update_frame (dispnew.c:3253)
==7487==    by 0x443EDB: redisplay_internal (xdisp.c:13175)
==7487==    by 0x4F6F47: read_char (keyboard.c:2443)
==7487==    by 0x4F9406: read_key_sequence.constprop.14 (keyboard.c:9290)
==7487==    by 0x4FB0D4: command_loop_1 (keyboard.c:1447)
==7487==    by 0x560015: internal_condition_case (eval.c:1499)
==7487==    by 0x4EE4AD: command_loop_2 (keyboard.c:1158)
==7487==    by 0x55FEF7: internal_catch (eval.c:1256)
==7487==    by 0x4EFA36: recursive_edit_1 (keyboard.c:1137)
==7487== 
==7487== 
==7487== ---- Attach to debugger ? --- [Return/N/n/Y/y/C/c] ---- Y

The line in question is:

4212      entry = row_table[i];


(gdb) p i
$1 = 0x157
(gdb) p row_table[i]
$2 = (struct row_entry *) 0x0
(gdb) p row_table_size
$3 = 0x193

Is it possible for the contents of row_table to be uninitialized?  Is this warning a false positive?

Message #8 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dan Nicolaescu <dann <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Tue, 08 Nov 2011 19:17:07 +0200

> From: Dan Nicolaescu <dann <at> gnu.org>
> Date: Tue, 08 Nov 2011 09:27:06 -0500
> 
> 
> valgrind ./temacs -Q gets this warning:
> 
> ==7487== Use of uninitialised value of size 8
> ==7487==    at 0x4140F4: update_window (dispnew.c:4212)
> ==7487==    by 0x414F32: update_window_tree (dispnew.c:3326)
> ==7487==    by 0x414F0E: update_window_tree (dispnew.c:3324)
> ==7487==    by 0x4181FD: update_frame (dispnew.c:3253)
> ==7487==    by 0x443EDB: redisplay_internal (xdisp.c:13175)
> ==7487==    by 0x4F6F47: read_char (keyboard.c:2443)
> ==7487==    by 0x4F9406: read_key_sequence.constprop.14 (keyboard.c:9290)
> ==7487==    by 0x4FB0D4: command_loop_1 (keyboard.c:1447)
> ==7487==    by 0x560015: internal_condition_case (eval.c:1499)
> ==7487==    by 0x4EE4AD: command_loop_2 (keyboard.c:1158)
> ==7487==    by 0x55FEF7: internal_catch (eval.c:1256)
> ==7487==    by 0x4EFA36: recursive_edit_1 (keyboard.c:1137)
> ==7487== 
> ==7487== 
> ==7487== ---- Attach to debugger ? --- [Return/N/n/Y/y/C/c] ---- Y
> 
> The line in question is:
> 
> 4212      entry = row_table[i];
> 
> 
> (gdb) p i
> $1 = 0x157
> (gdb) p row_table[i]
> $2 = (struct row_entry *) 0x0
> (gdb) p row_table_size
> $3 = 0x193
> 
> Is it possible for the contents of row_table to be uninitialized?  Is this warning a false positive?

row_table and row_table_size are static variables.  So at least in
temacs they should be initialized to zero, by this code in
scrolling_window:

  n = desired_matrix->nrows;
  n += current_matrix->nrows;
  if (row_table_size < 3 * n)
    {
      ptrdiff_t size = next_almost_prime (3 * n);
      row_table = xnrealloc (row_table, size, sizeof *row_table);
      row_table_size = size;
      memset (row_table, 0, size * sizeof *row_table);
    }

Because row_table_size is initially zero, the first call to
scrolling_window will allocate row_table[] and zero it out.

The only call to add_row_entry, the function where line 4212 belongs,
is in the same scrolling_window, a few lines _below_ the above
fragment that allocates and zeroes out row_table[].

So I don't see how row_table[i] could be uninitialized for any i that
is less than row_table_size.

Does valgrind know that row_table_size is initially zero because it is
static?

The other possibility I see is that one or both of
w->current_matrix->nrows and w->desired_matrix->nrows are
uninitialized.  But I think if that were so, Emacs would crash and
burn trying to display such a window.

Yet another possibility is that the argument ROW passed to
add_row_entry is not initialized.  Then row->hash is a random value
and i inside add_row_entry is also a random value.  Can you look at
these two loops inside scrolling_window:

  /* Add rows from the current and desired matrix to the hash table
     row_hash_table to be able to find equal ones quickly.  */

  for (i = first_old; i < last_old; ++i)
    {
      if (MATRIX_ROW (current_matrix, i)->enabled_p)
	{
	  entry = add_row_entry (MATRIX_ROW (current_matrix, i));
	  old_lines[i] = entry;
	  ++entry->old_uses;
	}
      else
	old_lines[i] = NULL;
    }

  for (i = first_new; i < last_new; ++i)
    {
      xassert (MATRIX_ROW_ENABLED_P (desired_matrix, i));
      entry = add_row_entry (MATRIX_ROW (desired_matrix, i));
      ++entry->new_uses;
      entry->new_line_number = i;
      new_lines[i] = entry;
    }

and see what are the values of first_old, last_old, first_new, and
last_new here, and whether the corresponding glyph rows look
reasonable, including their hash values?  Or maybe just look at the
row passed to add_row_entry.  You can display a given glyph_row
structure with the pgrowx command in GDB (but it won't show the hash
value, only how the row will look on the screen).  Another command is
prowx.

Message #11 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Dan Nicolaescu <dann <at> gnu.org>, 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Tue, 08 Nov 2011 19:44:28 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

> Does valgrind know that row_table_size is initially zero because it is
> static?

valgrind does not know anything about variables, only about memory
locations.  It operates at the machine language level.

Andreas.

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

Message #14 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: dann <at> gnu.org, 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Tue, 08 Nov 2011 22:35:54 +0200

> From: Andreas Schwab <schwab <at> linux-m68k.org>
> Cc: Dan Nicolaescu <dann <at> gnu.org>,  9990 <at> debbugs.gnu.org
> Date: Tue, 08 Nov 2011 19:44:28 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Does valgrind know that row_table_size is initially zero because it is
> > static?
> 
> valgrind does not know anything about variables, only about memory
> locations.  It operates at the machine language level.

Does that mean valgrind can potentially flag as uninitialized every
static variable that isn't explicitly initialized at run time?
Because AFAIK static variables are initialized by the linker (is that
right?).

Message #17 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Dan Nicolaescu <dann <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Fri, 11 Nov 2011 00:56:18 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Dan Nicolaescu <dann <at> gnu.org>
>> Date: Tue, 08 Nov 2011 09:27:06 -0500
>> 
>> 
>> valgrind ./temacs -Q gets this warning:
>> 
>> ==7487== Use of uninitialised value of size 8
>> ==7487==    at 0x4140F4: update_window (dispnew.c:4212)
>> ==7487==    by 0x414F32: update_window_tree (dispnew.c:3326)
>> ==7487==    by 0x414F0E: update_window_tree (dispnew.c:3324)
>> ==7487==    by 0x4181FD: update_frame (dispnew.c:3253)
>> ==7487==    by 0x443EDB: redisplay_internal (xdisp.c:13175)
>> ==7487==    by 0x4F6F47: read_char (keyboard.c:2443)
>> ==7487==    by 0x4F9406: read_key_sequence.constprop.14 (keyboard.c:9290)
>> ==7487==    by 0x4FB0D4: command_loop_1 (keyboard.c:1447)
>> ==7487==    by 0x560015: internal_condition_case (eval.c:1499)
>> ==7487==    by 0x4EE4AD: command_loop_2 (keyboard.c:1158)
>> ==7487==    by 0x55FEF7: internal_catch (eval.c:1256)
>> ==7487==    by 0x4EFA36: recursive_edit_1 (keyboard.c:1137)
>> ==7487== 
>> ==7487== 
>> ==7487== ---- Attach to debugger ? --- [Return/N/n/Y/y/C/c] ---- Y
>> 
>> The line in question is:
>> 
>> 4212      entry = row_table[i];
>> 
>> 
>> (gdb) p i
>> $1 = 0x157
>> (gdb) p row_table[i]
>> $2 = (struct row_entry *) 0x0
>> (gdb) p row_table_size
>> $3 = 0x193
>> 
>> Is it possible for the contents of row_table to be uninitialized?  Is this warning a false positive?
>
> row_table and row_table_size are static variables.  So at least in
> temacs they should be initialized to zero, by this code in
> scrolling_window:


>
>   n = desired_matrix->nrows;
>   n += current_matrix->nrows;
>   if (row_table_size < 3 * n)
>     {
>       ptrdiff_t size = next_almost_prime (3 * n);
>       row_table = xnrealloc (row_table, size, sizeof *row_table);
>       row_table_size = size;
>       memset (row_table, 0, size * sizeof *row_table);
>     }
>
> Because row_table_size is initially zero, the first call to
> scrolling_window will allocate row_table[] and zero it out.
>
> The only call to add_row_entry, the function where line 4212 belongs,
> is in the same scrolling_window, a few lines _below_ the above
> fragment that allocates and zeroes out row_table[].
>
> So I don't see how row_table[i] could be uninitialized for any i that
> is less than row_table_size.
>
> Does valgrind know that row_table_size is initially zero because it is
> static?

I think it should.

I got another (maybe) similar one.  For this one I had the option that
shows the location of uninitialized variable.  This happened after doing C-h H.

==4752== Conditional jump or move depends on uninitialised value(s)
==4752==    at 0x4137ED: update_window (dispnew.c:1276)
==4752==    by 0x414F02: update_window_tree (dispnew.c:3326)
==4752==    by 0x4181CD: update_frame (dispnew.c:3253)
==4752==    by 0x440E7B: redisplay_internal (xdisp.c:13175)
==4752==    by 0x4F0A87: read_char (keyboard.c:2443)
==4752==    by 0x4F2F46: read_key_sequence.constprop.14 (keyboard.c:9290)
==4752==    by 0x4F4C14: command_loop_1 (keyboard.c:1447)
==4752==    by 0x559B55: internal_condition_case (eval.c:1499)
==4752==    by 0x4E7FED: command_loop_2 (keyboard.c:1158)
==4752==    by 0x559A37: internal_catch (eval.c:1256)
==4752==    by 0x4E94EE: recursive_edit_1 (keyboard.c:1123)
==4752==    by 0x515CFB: read_minibuf (minibuf.c:677)
==4752==  Uninitialised value was created by a heap allocation
==4752==    at 0x4A0649D: malloc (vg_replace_malloc.c:236)
==4752==    by 0x5407CF: xrealloc (alloc.c:742)
==4752==    by 0x411001: adjust_glyph_matrix (dispnew.c:580)
==4752==    by 0x41148C: allocate_matrices_for_window_redisplay (dispnew.c:1838)
==4752==    by 0x4119DC: adjust_frame_glyphs (dispnew.c:2167)
==4752==    by 0x416BC9: adjust_glyphs (dispnew.c:1860)
==4752==    by 0x4686A7: Fdelete_other_windows_internal (window.c:2809)
==4752==    by 0x55B9FB: Ffuncall (eval.c:2977)
==4752==    by 0x593BE5: exec_byte_code (bytecode.c:785)
==4752==    by 0x55AE2A: eval_sub (eval.c:2328)
==4752==    by 0x559A37: internal_catch (eval.c:1256)
==4752==    by 0x594567: exec_byte_code (bytecode.c:966)
==4752== 


> and see what are the values of first_old, last_old, first_new, and
> last_new here, and whether the corresponding glyph rows look
> reasonable, including their hash values?  Or maybe just look at the
> row passed to add_row_entry.  You can display a given glyph_row
> structure with the pgrowx command in GDB (but it won't show the hash
> value, only how the row will look on the screen).  Another command is
> prowx.

I will do this when it happens again.

Message #20 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dan Nicolaescu <dann <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Fri, 11 Nov 2011 17:30:58 +0200

> From: Dan Nicolaescu <dann <at> gnu.org>
> Cc: 9990 <at> debbugs.gnu.org
> Date: Fri, 11 Nov 2011 00:56:18 -0500
> 
> I got another (maybe) similar one.  For this one I had the option that
> shows the location of uninitialized variable.  This happened after doing C-h H.

Is this reproducible?  If so, can you tell how to reproduce it?

> ==4752== Conditional jump or move depends on uninitialised value(s)
> ==4752==    at 0x4137ED: update_window (dispnew.c:1276)
> ==4752==    by 0x414F02: update_window_tree (dispnew.c:3326)
> ==4752==    by 0x4181CD: update_frame (dispnew.c:3253)
> ==4752==    by 0x440E7B: redisplay_internal (xdisp.c:13175)
> ==4752==    by 0x4F0A87: read_char (keyboard.c:2443)
> ==4752==    by 0x4F2F46: read_key_sequence.constprop.14 (keyboard.c:9290)
> ==4752==    by 0x4F4C14: command_loop_1 (keyboard.c:1447)
> ==4752==    by 0x559B55: internal_condition_case (eval.c:1499)
> ==4752==    by 0x4E7FED: command_loop_2 (keyboard.c:1158)
> ==4752==    by 0x559A37: internal_catch (eval.c:1256)
> ==4752==    by 0x4E94EE: recursive_edit_1 (keyboard.c:1123)
> ==4752==    by 0x515CFB: read_minibuf (minibuf.c:677)
> ==4752==  Uninitialised value was created by a heap allocation
> ==4752==    at 0x4A0649D: malloc (vg_replace_malloc.c:236)
> ==4752==    by 0x5407CF: xrealloc (alloc.c:742)
> ==4752==    by 0x411001: adjust_glyph_matrix (dispnew.c:580)
> ==4752==    by 0x41148C: allocate_matrices_for_window_redisplay (dispnew.c:1838)
> ==4752==    by 0x4119DC: adjust_frame_glyphs (dispnew.c:2167)
> ==4752==    by 0x416BC9: adjust_glyphs (dispnew.c:1860)
> ==4752==    by 0x4686A7: Fdelete_other_windows_internal (window.c:2809)
> ==4752==    by 0x55B9FB: Ffuncall (eval.c:2977)
> ==4752==    by 0x593BE5: exec_byte_code (bytecode.c:785)
> ==4752==    by 0x55AE2A: eval_sub (eval.c:2328)
> ==4752==    by 0x559A37: internal_catch (eval.c:1256)
> ==4752==    by 0x594567: exec_byte_code (bytecode.c:966)

It seems to tell that some glyph row(s) whose glyphs are reallocated
here:

	  while (row < end)
	    {
	      row->glyphs[LEFT_MARGIN_AREA]
		= xnrealloc (row->glyphs[LEFT_MARGIN_AREA],  <<<<<<<<<<<
			     dim.width, sizeof (struct glyph));

don't have their hash values initialized, and so this comparison
within row_equal_p:

  if (a == b)
    return 1;
  else if (a->hash != b->hash)  <<<<<<<<<<<<<<<<<<<<<
    return 0;
  else
    {

uses uninitialized value.

The strange thing is, the above xnrealloc is not supposed to affect
the row's hash value in any way, it just reallocates its glyphs.  So I
cannot make heads or tails out of this.  And the hash value is
initialized to zero for additional glyph rows added to a glyph matrix,
in this fragment:

  /* Enlarge MATRIX->rows if necessary.  New rows are cleared.  */
  if (matrix->rows_allocated < dim.height)
    {
      int old_alloc = matrix->rows_allocated;
      new_rows = dim.height - matrix->rows_allocated;
      matrix->rows = xpalloc (matrix->rows, &matrix->rows_allocated,
			      new_rows, INT_MAX, sizeof *matrix->rows);
      memset (matrix->rows + old_alloc, 0,
	      (matrix->rows_allocated - old_alloc) * sizeof *matrix->rows);
    }

The call to `memset' should zero out all the fields of each glyph_row
that were just added to enlarge the matrix.

However, I spotted something strange related to the call to
row_equal_p, here:

  /* Skip over rows equal at the bottom.  */
  i = last_new;
  j = last_old;
  while (i - 1 > first_new
         && j - 1 > first_old
         && MATRIX_ROW (current_matrix, i - 1)->enabled_p
	 && (MATRIX_ROW (current_matrix, i - 1)->y
	     == MATRIX_ROW (desired_matrix, j - 1)->y)
	 && !MATRIX_ROW (desired_matrix, j - 1)->redraw_fringe_bitmaps_p
         && row_equal_p (MATRIX_ROW (desired_matrix, i - 1),
                         MATRIX_ROW (current_matrix, j - 1), 1))
    --i, --j;

Some of these conditions use incorrect indices to access the glyph
matrices: `i' should be used for the current matrix and `j' for the
desired matrix.  Some of these conditions use `i' and `j' correctly,
some don't.  So it's possible, for example, that the test of the
enabled_p flag produces incorrect results, and we then proceed calling
row_equal_p on a row which is not enabled and whose hash was not
computed by redisplay.

Message #23 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: dann <at> gnu.org
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Fri, 11 Nov 2011 17:59:22 +0200

> Date: Fri, 11 Nov 2011 17:30:58 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 9990 <at> debbugs.gnu.org
> 
> However, I spotted something strange related to the call to
> row_equal_p, here:
> 
>   /* Skip over rows equal at the bottom.  */
>   i = last_new;
>   j = last_old;
>   while (i - 1 > first_new
>          && j - 1 > first_old
>          && MATRIX_ROW (current_matrix, i - 1)->enabled_p
> 	 && (MATRIX_ROW (current_matrix, i - 1)->y
> 	     == MATRIX_ROW (desired_matrix, j - 1)->y)
> 	 && !MATRIX_ROW (desired_matrix, j - 1)->redraw_fringe_bitmaps_p
>          && row_equal_p (MATRIX_ROW (desired_matrix, i - 1),
>                          MATRIX_ROW (current_matrix, j - 1), 1))
>     --i, --j;
> 
> Some of these conditions use incorrect indices to access the glyph
> matrices: `i' should be used for the current matrix and `j' for the
> desired matrix.  Some of these conditions use `i' and `j' correctly,
> some don't.

Below is a patch I intend to install, if no one finds any thinko in
it.  Dan, can you try this and see if it resolves the valgrind
complaints (assuming you can reproduce them)?

=== modified file 'src/dispnew.c'
--- src/dispnew.c	2011-10-08 10:58:50 +0000
+++ src/dispnew.c	2011-11-11 15:53:27 +0000
@@ -4334,10 +4334,10 @@ scrolling_window (struct window *w, int 
   j = last_old;
   while (i - 1 > first_new
          && j - 1 > first_old
-         && MATRIX_ROW (current_matrix, i - 1)->enabled_p
-	 && (MATRIX_ROW (current_matrix, i - 1)->y
-	     == MATRIX_ROW (desired_matrix, j - 1)->y)
-	 && !MATRIX_ROW (desired_matrix, j - 1)->redraw_fringe_bitmaps_p
+         && MATRIX_ROW (current_matrix, j - 1)->enabled_p
+	 && (MATRIX_ROW (current_matrix, j - 1)->y
+	     == MATRIX_ROW (desired_matrix, i - 1)->y)
+	 && !MATRIX_ROW (desired_matrix, i - 1)->redraw_fringe_bitmaps_p
          && row_equal_p (MATRIX_ROW (desired_matrix, i - 1),
                          MATRIX_ROW (current_matrix, j - 1), 1))
     --i, --j;

Message #26 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Dan Nicolaescu <dann <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Fri, 11 Nov 2011 15:17:34 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Dan Nicolaescu <dann <at> gnu.org>
>> Cc: 9990 <at> debbugs.gnu.org
>> Date: Fri, 11 Nov 2011 00:56:18 -0500
>> 
>> I got another (maybe) similar one.  For this one I had the option that
>> shows the location of uninitialized variable.  This happened after doing C-h H.
>
> Is this reproducible?  If so, can you tell how to reproduce it?

valgrind ./temacs -Q
C-h H

Message #29 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Dan Nicolaescu <dann <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Fri, 11 Nov 2011 15:20:19 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Date: Fri, 11 Nov 2011 17:30:58 +0200
>> From: Eli Zaretskii <eliz <at> gnu.org>
>> Cc: 9990 <at> debbugs.gnu.org
>> 
>> However, I spotted something strange related to the call to
>> row_equal_p, here:
>> 
>>   /* Skip over rows equal at the bottom.  */
>>   i = last_new;
>>   j = last_old;
>>   while (i - 1 > first_new
>>          && j - 1 > first_old
>>          && MATRIX_ROW (current_matrix, i - 1)->enabled_p
>> 	 && (MATRIX_ROW (current_matrix, i - 1)->y
>> 	     == MATRIX_ROW (desired_matrix, j - 1)->y)
>> 	 && !MATRIX_ROW (desired_matrix, j - 1)->redraw_fringe_bitmaps_p
>>          && row_equal_p (MATRIX_ROW (desired_matrix, i - 1),
>>                          MATRIX_ROW (current_matrix, j - 1), 1))
>>     --i, --j;
>> 
>> Some of these conditions use incorrect indices to access the glyph
>> matrices: `i' should be used for the current matrix and `j' for the
>> desired matrix.  Some of these conditions use `i' and `j' correctly,
>> some don't.
>
> Below is a patch I intend to install, if no one finds any thinko in
> it.  Dan, can you try this and see if it resolves the valgrind
> complaints (assuming you can reproduce them)?
>
> === modified file 'src/dispnew.c'
> --- src/dispnew.c	2011-10-08 10:58:50 +0000
> +++ src/dispnew.c	2011-11-11 15:53:27 +0000
> @@ -4334,10 +4334,10 @@ scrolling_window (struct window *w, int 
>    j = last_old;
>    while (i - 1 > first_new
>           && j - 1 > first_old
> -         && MATRIX_ROW (current_matrix, i - 1)->enabled_p
> -	 && (MATRIX_ROW (current_matrix, i - 1)->y
> -	     == MATRIX_ROW (desired_matrix, j - 1)->y)
> -	 && !MATRIX_ROW (desired_matrix, j - 1)->redraw_fringe_bitmaps_p
> +         && MATRIX_ROW (current_matrix, j - 1)->enabled_p
> +	 && (MATRIX_ROW (current_matrix, j - 1)->y
> +	     == MATRIX_ROW (desired_matrix, i - 1)->y)
> +	 && !MATRIX_ROW (desired_matrix, i - 1)->redraw_fringe_bitmaps_p
>           && row_equal_p (MATRIX_ROW (desired_matrix, i - 1),
>                           MATRIX_ROW (current_matrix, j - 1), 1))
>      --i, --j;

I haven't seen the one in row_equal_p anymore (but that one was not easy
to reproduce), the ones in update_window still show up.

Message #32 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dan Nicolaescu <dann <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Sat, 12 Nov 2011 14:04:18 +0200

> From: Dan Nicolaescu <dann <at> gnu.org>
> Cc: 9990 <at> debbugs.gnu.org
> Date: Fri, 11 Nov 2011 15:20:19 -0500
> 
> I haven't seen the one in row_equal_p anymore (but that one was not easy
> to reproduce), the ones in update_window still show up.

I added a function that verifies the hash value of glyph rows before
it is used in row_equal_p, and also in adjust_glyph_matrix, where the
offending reallocation takes place.  I cannot trigger the xasserts
that use this function when I use "C-h H".  Can you?

If the hash values are always correct where they are used, I guess
that excludes the possibility that we use an uninitialized value,
right?

Message #35 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Dan Nicolaescu <dann <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Tue, 15 Nov 2011 11:58:50 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Dan Nicolaescu <dann <at> gnu.org>
>> Cc: 9990 <at> debbugs.gnu.org
>> Date: Fri, 11 Nov 2011 15:20:19 -0500
>> 
>> I haven't seen the one in row_equal_p anymore (but that one was not easy
>> to reproduce), the ones in update_window still show up.
>
> I added a function that verifies the hash value of glyph rows before
> it is used in row_equal_p, and also in adjust_glyph_matrix, where the
> offending reallocation takes place.  I cannot trigger the xasserts
> that use this function when I use "C-h H".  Can you?

No, I cannot.

> If the hash values are always correct where they are used, I guess
> that excludes the possibility that we use an uninitialized value,
> right?

I even added an  xassert (verify_row_hash (row)) in add_row_entry, and
it does not trigger.  Strange...

Message #38 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Dan Nicolaescu <dann <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Tue, 15 Nov 2011 12:21:56 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Dan Nicolaescu <dann <at> gnu.org>
>> Cc: 9990 <at> debbugs.gnu.org
>> Date: Fri, 11 Nov 2011 15:20:19 -0500
>> 
>> I haven't seen the one in row_equal_p anymore (but that one was not easy
>> to reproduce), the ones in update_window still show up.
>
> I added a function that verifies the hash value of glyph rows before
> it is used in row_equal_p, and also in adjust_glyph_matrix, where the
> offending reallocation takes place.  I cannot trigger the xasserts
> that use this function when I use "C-h H".  Can you?

BTW, why is the hash only using 28 bits instead of the full 32?  Won't
it get a bit more precision if using 32?

Message #41 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dan Nicolaescu <dann <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Tue, 15 Nov 2011 19:44:45 +0200

> From: Dan Nicolaescu <dann <at> gnu.org>
> Cc: 9990 <at> debbugs.gnu.org
> Date: Tue, 15 Nov 2011 11:58:50 -0500
> 
> > I added a function that verifies the hash value of glyph rows before
> > it is used in row_equal_p, and also in adjust_glyph_matrix, where the
> > offending reallocation takes place.  I cannot trigger the xasserts
> > that use this function when I use "C-h H".  Can you?
> 
> No, I cannot.
> 
> > If the hash values are always correct where they are used, I guess
> > that excludes the possibility that we use an uninitialized value,
> > right?
> 
> I even added an  xassert (verify_row_hash (row)) in add_row_entry, and
> it does not trigger.  Strange...

I have a feeling I didn't interpret the valgrind diagnostics
correctly.  But I don't see any alternative interpretation that would
make sense.

Message #44 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dan Nicolaescu <dann <at> gnu.org>, Richard Stallman <rms <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Tue, 15 Nov 2011 19:49:46 +0200

> From: Dan Nicolaescu <dann <at> gnu.org>
> Cc: 9990 <at> debbugs.gnu.org
> Date: Tue, 15 Nov 2011 12:21:56 -0500
> 
> BTW, why is the hash only using 28 bits instead of the full 32?  Won't
> it get a bit more precision if using 32?

I don't know.  I suspect this is simply history inherited from a
similar (but not identical -- why?) code in line_hash_code, which also
uses 28 bits.

Richard, do you remember by chance why only 28 bits are used?

Message #47 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dan Nicolaescu <dann <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Fri, 18 Nov 2011 14:44:30 +0200

> From: Dan Nicolaescu <dann <at> gnu.org>
> Cc: 9990 <at> debbugs.gnu.org
> Date: Tue, 15 Nov 2011 11:58:50 -0500
> 
> > If the hash values are always correct where they are used, I guess
> > that excludes the possibility that we use an uninitialized value,
> > right?
> 
> I even added an  xassert (verify_row_hash (row)) in add_row_entry, and
> it does not trigger.  Strange...

I added such an assert to the trunk.  I also fixed a couple of
functions that were destroying the validity of hash codes while
manipulating glyph rows.

Could you please see if valgrind still complains about add_row_entry
with the current trunk?

Message #50 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Dan Nicolaescu <dann <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Fri, 18 Nov 2011 14:40:34 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Dan Nicolaescu <dann <at> gnu.org>
>> Cc: 9990 <at> debbugs.gnu.org
>> Date: Tue, 15 Nov 2011 11:58:50 -0500
>> 
>> > If the hash values are always correct where they are used, I guess
>> > that excludes the possibility that we use an uninitialized value,
>> > right?
>> 
>> I even added an  xassert (verify_row_hash (row)) in add_row_entry, and
>> it does not trigger.  Strange...
>
> I added such an assert to the trunk.  I also fixed a couple of
> functions that were destroying the validity of hash codes while
> manipulating glyph rows.
>
> Could you please see if valgrind still complains about add_row_entry
> with the current trunk?

Unfortunately it still complains in:

==11270==    at 0x41314F: adjust_glyph_matrix (dispnew.c:612)
==11270==    by 0x4135FC: allocate_matrices_for_window_redisplay (dispnew.c:1869)
==11270==    by 0x413B8A: adjust_frame_glyphs (dispnew.c:2199)
==11270==    by 0x417137: adjust_glyphs (dispnew.c:1897)
==11270==    by 0x44243E: redisplay_internal (xdisp.c:12715)
==11270==    by 0x4F6CE2: command_loop_1 (keyboard.c:1589)
==11270==    by 0x55BB45: internal_condition_case (eval.c:1499)
==11270==    by 0x4E9EAD: command_loop_2 (keyboard.c:1158)
==11270==    by 0x55BA27: internal_catch (eval.c:1256)
==11270==    by 0x4EB436: recursive_edit_1 (keyboard.c:1137)
==11270==    by 0x4EB76B: Frecursive_edit (keyboard.c:821)
==11270==    by 0x40E62C: main (emacs.c:1707)

==11270==    by 0x415762: update_window (dispnew.c:4244)
==11270==    by 0x4166C2: update_window_tree (dispnew.c:3360)
==11270==    by 0x418617: update_frame (dispnew.c:3287)
==11270==    by 0x44207B: redisplay_internal (xdisp.c:13175)
==11270==    by 0x4F6CE2: command_loop_1 (keyboard.c:1589)
==11270==    by 0x55BB45: internal_condition_case (eval.c:1499)
==11270==    by 0x4E9EAD: command_loop_2 (keyboard.c:1158)
==11270==    by 0x55BA27: internal_catch (eval.c:1256)
==11270==    by 0x4EB436: recursive_edit_1 (keyboard.c:1137)

[line numbers in dispnew.c might be off by a few lines, I have some
debugging printfs inserted there]

Message #53 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dan Nicolaescu <dann <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Fri, 18 Nov 2011 23:08:39 +0200

> From: Dan Nicolaescu <dann <at> gnu.org>
> Cc: 9990 <at> debbugs.gnu.org
> Date: Fri, 18 Nov 2011 14:40:34 -0500
> 
> > I added such an assert to the trunk.  I also fixed a couple of
> > functions that were destroying the validity of hash codes while
> > manipulating glyph rows.
> >
> > Could you please see if valgrind still complains about add_row_entry
> > with the current trunk?
> 
> Unfortunately it still complains in:
> 
> ==11270==    at 0x41314F: adjust_glyph_matrix (dispnew.c:612)

What does this line say in your sources?

Anyway, seems like a different thing, as there's no add_row_entry near
line 612.

Message #56 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Dan Nicolaescu <dann <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Fri, 18 Nov 2011 16:43:11 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Dan Nicolaescu <dann <at> gnu.org>
>> Cc: 9990 <at> debbugs.gnu.org
>> Date: Fri, 18 Nov 2011 14:40:34 -0500
>> 
>> > I added such an assert to the trunk.  I also fixed a couple of
>> > functions that were destroying the validity of hash codes while
>> > manipulating glyph rows.
>> >
>> > Could you please see if valgrind still complains about add_row_entry
>> > with the current trunk?
>> 
>> Unfortunately it still complains in:
>> 
>> ==11270==    at 0x41314F: adjust_glyph_matrix (dispnew.c:612)
>
> What does this line say in your sources?
>

              xassert (!row->enabled_p || verify_row_hash (row));

> Anyway, seems like a different thing, as there's no add_row_entry near
> line 612.

And for the other complaint:
==11270==    by 0x415762: update_window (dispnew.c:4244)

is:
   ptrdiff_t i = row->hash % row_table_size;

Message #59 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dan Nicolaescu <dann <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Sat, 19 Nov 2011 10:28:11 +0200

> From: Dan Nicolaescu <dann <at> gnu.org>
> Cc: 9990 <at> debbugs.gnu.org
> Date: Fri, 18 Nov 2011 16:43:11 -0500
> 
> >> Unfortunately it still complains in:
> >> 
> >> ==11270==    at 0x41314F: adjust_glyph_matrix (dispnew.c:612)
> >
> > What does this line say in your sources?
> >
> 
>               xassert (!row->enabled_p || verify_row_hash (row));

So it complains about `row' itself, or maybe about the enabled_p
field, is that right?

> And for the other complaint:
> ==11270==    by 0x415762: update_window (dispnew.c:4244)
> 
> is:
>    ptrdiff_t i = row->hash % row_table_size;

Yes, that was the original one, triggered by "C-h H".  But maybe it is
complaining about `row' itself, even here, not about the hash code?  I
will take another look.

Message #62 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Dan Nicolaescu <dann <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Sun, 20 Nov 2011 16:30:31 -0500

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Dan Nicolaescu <dann <at> gnu.org>
>> Cc: 9990 <at> debbugs.gnu.org
>> Date: Fri, 18 Nov 2011 16:43:11 -0500
>> 
>> >> Unfortunately it still complains in:
>> >> 
>> >> ==11270==    at 0x41314F: adjust_glyph_matrix (dispnew.c:612)
>> >
>> > What does this line say in your sources?
>> >
>> 
>>               xassert (!row->enabled_p || verify_row_hash (row));
>
> So it complains about `row' itself, or maybe about the enabled_p
> field, is that right?

It's either row, row->enabled_p or row->hash.  (In case the warning is
valid...)

Message #65 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Dan Nicolaescu <dann <at> gnu.org>
Cc: 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Mon, 17 Aug 2020 22:34:30 +0000

Dan Nicolaescu <dann <at> gnu.org> writes:

> valgrind ./temacs -Q gets this warning:
>
> ==7487== Use of uninitialised value of size 8
> ==7487==    at 0x4140F4: update_window (dispnew.c:4212)
> ==7487==    by 0x414F32: update_window_tree (dispnew.c:3326)
> ==7487==    by 0x414F0E: update_window_tree (dispnew.c:3324)
> ==7487==    by 0x4181FD: update_frame (dispnew.c:3253)
> ==7487==    by 0x443EDB: redisplay_internal (xdisp.c:13175)
> ==7487==    by 0x4F6F47: read_char (keyboard.c:2443)
> ==7487==    by 0x4F9406: read_key_sequence.constprop.14 (keyboard.c:9290)
> ==7487==    by 0x4FB0D4: command_loop_1 (keyboard.c:1447)
> ==7487==    by 0x560015: internal_condition_case (eval.c:1499)
> ==7487==    by 0x4EE4AD: command_loop_2 (keyboard.c:1158)
> ==7487==    by 0x55FEF7: internal_catch (eval.c:1256)
> ==7487==    by 0x4EFA36: recursive_edit_1 (keyboard.c:1137)
> ==7487==
> ==7487==
> ==7487== ---- Attach to debugger ? --- [Return/N/n/Y/y/C/c] ---- Y
>
> The line in question is:
>
> 4212      entry = row_table[i];
>
>
> (gdb) p i
> $1 = 0x157
> (gdb) p row_table[i]
> $2 = (struct row_entry *) 0x0
> (gdb) p row_table_size
> $3 = 0x193
>
> Is it possible for the contents of row_table to be uninitialized?  Is this warning a false positive?

This is a report with a number of valgrind warnings from 9 years ago.
In this discussion, a number of warnings were fixed but then work
unfortunately seems to have stopped.

Is anyone still working on this or should this be closed?

Best regards,
Stefan Kangas

Message #70 received at 9990 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: Dan Nicolaescu <dann <at> gnu.org>, 9990 <at> debbugs.gnu.org
Subject: Re: bug#9990: valgrind warning in add_row_entry
Date: Thu, 01 Oct 2020 03:55:45 +0200

Stefan Kangas <stefan <at> marxist.se> writes:

> This is a report with a number of valgrind warnings from 9 years ago.
> In this discussion, a number of warnings were fixed but then work
> unfortunately seems to have stopped.
>
> Is anyone still working on this or should this be closed?

There was no response in six weeks, so I'm now closing this bug report.
If there are more valgrind issues (and I'm sure there are), then new
reports should be opened for them.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.