bug#9990: valgrind warning in add_row_entry

bug#9990: valgrind warning in add_row_entry

[Dan Nicolaescu]

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?

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@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.

bug#9990: valgrind warning in add_row_entry

[Andreas Schwab]

Eli Zaretskii <eliz@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@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Dan Nicolaescu <dann@gnu.org>,  9990@debbugs.gnu.org
> Date: Tue, 08 Nov 2011 19:44:28 +0100
> 
> Eli Zaretskii <eliz@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?).

bug#9990: valgrind warning in add_row_entry

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@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.

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: 9990@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.

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> Date: Fri, 11 Nov 2011 17:30:58 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 9990@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;

bug#9990: valgrind warning in add_row_entry

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Cc: 9990@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

bug#9990: valgrind warning in add_row_entry

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 11 Nov 2011 17:30:58 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: 9990@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.

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: 9990@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?

bug#9990: valgrind warning in add_row_entry

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Cc: 9990@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...

bug#9990: valgrind warning in add_row_entry

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Cc: 9990@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?

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: 9990@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.

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: 9990@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?

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: 9990@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?

bug#9990: valgrind warning in add_row_entry

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Cc: 9990@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]

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: 9990@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.

bug#9990: valgrind warning in add_row_entry

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Cc: 9990@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;

bug#9990: valgrind warning in add_row_entry

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: 9990@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.

bug#9990: valgrind warning in add_row_entry

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Cc: 9990@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...)

bug#9990: valgrind warning in add_row_entry

[Stefan Kangas]

Dan Nicolaescu <dann@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

bug#9990: valgrind warning in add_row_entry

[Lars Ingebrigtsen]

Stefan Kangas <stefan@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