bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[Dan Nicolaescu]

valgrind gives this warning on emacs -Q, lucid toolkit, compiled -g -O0

==7776== Conditional jump or move depends on uninitialised value(s)
==7776==    at 0x44F58B: redisplay_window (xdisp.c:15575)
==7776==    by 0x4496BF: redisplay_window_0 (xdisp.c:13573)
==7776==    by 0x5D3DBF: internal_condition_case_1 (eval.c:1537)
==7776==    by 0x449632: redisplay_windows (xdisp.c:13553)
==7776==    by 0x44864F: redisplay_internal (xdisp.c:13130)
==7776==    by 0x4463AF: redisplay (xdisp.c:12353)
==7776==    by 0x53A8C6: read_char (keyboard.c:2443)
==7776==    by 0x54895A: read_key_sequence (keyboard.c:9290)
==7776==    by 0x5387ED: command_loop_1 (keyboard.c:1447)
==7776==    by 0x5D3C46: internal_condition_case (eval.c:1499)
==7776==    by 0x5380F7: command_loop_2 (keyboard.c:1158)
==7776==    by 0x5D35D0: internal_catch (eval.c:1256)
==7776== 

The line in question is 
      aggressive =
      scrolling_up
      ? BVAR (current_buffer, scroll_up_aggressively)
      : BVAR (current_buffer, scroll_down_aggressively);
       ^^^^^^^^^^^^^^^^^^
      This one

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Date: Thu, 03 Nov 2011 17:55:03 -0400
> 
>       aggressive =
>       scrolling_up
>       ? BVAR (current_buffer, scroll_up_aggressively)
>       : BVAR (current_buffer, scroll_down_aggressively);
>        ^^^^^^^^^^^^^^^^^^
>       This one

How can this be uninitialized?  This is a buffer-local value of a
variable that is initialized to nil on buffer.c.

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Date: Thu, 03 Nov 2011 17:55:03 -0400
>> 
>>       aggressive =
>>       scrolling_up
>>       ? BVAR (current_buffer, scroll_up_aggressively)
>>       : BVAR (current_buffer, scroll_down_aggressively);
>>        ^^^^^^^^^^^^^^^^^^
>>       This one
>
> How can this be uninitialized?  This is a buffer-local value of a
> variable that is initialized to nil on buffer.c.

The line might be incorrect, are all the variables in that expression
initialized correctly?

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[Eli Zaretskii]

> From: Dan Nicolaescu <dann@gnu.org>
> Cc: 9948@debbugs.gnu.org
> Date: Thu, 03 Nov 2011 19:30:55 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Dan Nicolaescu <dann@gnu.org>
> >> Date: Thu, 03 Nov 2011 17:55:03 -0400
> >> 
> >>       aggressive =
> >>       scrolling_up
> >>       ? BVAR (current_buffer, scroll_up_aggressively)
> >>       : BVAR (current_buffer, scroll_down_aggressively);
> >>        ^^^^^^^^^^^^^^^^^^
> >>       This one
> >
> > How can this be uninitialized?  This is a buffer-local value of a
> > variable that is initialized to nil on buffer.c.
> 
> The line might be incorrect, are all the variables in that expression
> initialized correctly?

I think so, yes.  The only other variables are:

  . current_buffer -- a global variable that always has some value

  . scrolling_up -- computed in the line above this snippet.  It
    depends on margin_pos, which is initialized at the beginning of
    the parent block.

The BVAR macro doesn't reference any variables except its first
argument.

So I'm really puzzled about this one.  Is there any way to ask
valgrind for a more detailed report?

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[Dan Nicolaescu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dan Nicolaescu <dann@gnu.org>
>> Cc: 9948@debbugs.gnu.org
>> Date: Thu, 03 Nov 2011 19:30:55 -0400
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Dan Nicolaescu <dann@gnu.org>
>> >> Date: Thu, 03 Nov 2011 17:55:03 -0400
>> >> 
>> >>       aggressive =
>> >>       scrolling_up
>> >>       ? BVAR (current_buffer, scroll_up_aggressively)
>> >>       : BVAR (current_buffer, scroll_down_aggressively);
>> >>        ^^^^^^^^^^^^^^^^^^
>> >>       This one
>> >
>> > How can this be uninitialized?  This is a buffer-local value of a
>> > variable that is initialized to nil on buffer.c.
>> 
>> The line might be incorrect, are all the variables in that expression
>> initialized correctly?
>
> I think so, yes.  The only other variables are:
>
>   . current_buffer -- a global variable that always has some value
>
>   . scrolling_up -- computed in the line above this snippet.  It
>     depends on margin_pos, which is initialized at the beginning of
>     the parent block.
>
> The BVAR macro doesn't reference any variables except its first
> argument.
>
> So I'm really puzzled about this one.  Is there any way to ask
> valgrind for a more detailed report?

It looks like there's a --track-origins=yes that might help.
Now just need to reproduce the same issue (which occurred after quite a
bit of random editing/playing around, so it might not be easy...).

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[Paul Eggert]

--track-origins=yes should help, but in the meantime, valgrind's
bug report doesn't necessarily mean that no code ever set scrolling_up.

It could be that scrolling_up was set this way:

      scrolling_up = PT > margin_pos;

but that margin_pos wasn't properly initialized.  For example, suppose
margin_pos was set this way:

          margin_pos = IT_CHARPOS (it1);

This initialization would not be correct if IT_CHARPOS (it1) referenced
an uninitialized variable.

Unfortunately valgrind won't report any error in IT_CHARPOS (it1)
until scrolling_up is used.  That's because it does not report use
of uninitialized storage: it reports only when conditional branches
or syscalls or addresses depend on the contents of the uninitialized
storage.

It's too bad that valgrind works this way.  It'd be more convenient if it
reported use of uninitialized storage right away.  But it'd be hard for
valgrind to do better, without reporting lots of false positives for
structures that contain holes, so it is what it is.

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[Eli Zaretskii]

> Date: Sun, 06 Nov 2011 21:04:40 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> --track-origins=yes should help, but in the meantime, valgrind's
> bug report doesn't necessarily mean that no code ever set scrolling_up.
> 
> It could be that scrolling_up was set this way:
> 
>       scrolling_up = PT > margin_pos;
> 
> but that margin_pos wasn't properly initialized.  For example, suppose
> margin_pos was set this way:
> 
>           margin_pos = IT_CHARPOS (it1);
> 
> This initialization would not be correct if IT_CHARPOS (it1) referenced
> an uninitialized variable.

IT_CHARPOS is defined as follows:

  #define CHARPOS(POS)		(POS).charpos
  #define IT_CHARPOS(IT)	CHARPOS ((IT).current.pos)

And margin_pos is computed as follows:

      EMACS_INT margin_pos = CHARPOS (startp);  <<<<<<<<<<<<<<
      int scrolling_up;
      Lisp_Object aggressive;

      /* If there is a scroll margin at the top of the window, find
	 its character position.  */
      if (margin
	  /* Cannot call start_display if startp is not in the
	     accessible region of the buffer.  This can happen when we
	     have just switched to a different buffer and/or changed
	     its restriction.  In that case, startp is initialized to
	     the character position 1 (BEG) because we did not yet
	     have chance to display the buffer even once.  */
	  && BEGV <= CHARPOS (startp) && CHARPOS (startp) <= ZV)
	{
	  struct it it1;
	  void *it1data = NULL;

	  SAVE_IT (it1, it, it1data);
	  start_display (&it1, w, startp);
	  move_it_vertically (&it1, margin);
	  margin_pos = IT_CHARPOS (it1);  <<<<<<<<<<<<<<<<<<
	  RESTORE_IT (&it, &it, it1data);
	}
      scrolling_up = PT > margin_pos;
      aggressive =
	scrolling_up
	? BVAR (current_buffer, scroll_up_aggressively)
	: BVAR (current_buffer, scroll_down_aggressively);

Both `startp' and `it1' have a valid CHARPOS, the former by virtue of
this (near the beginning of the function):

  SET_TEXT_POS_FROM_MARKER (startp, w->start);

and the latter by virtue of the start_display call above, which
initializes `it1's character position to `startp'.

Again, I don't see how any of this could involve an uninitialized
variable.

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[Paul Eggert]

On 11/06/11 22:00, Eli Zaretskii wrote:
> Again, I don't see how any of this could involve an uninitialized
> variable.

I don't either, but the analysis you gave is not complete.
For example:

  SET_TEXT_POS_FROM_MARKER (startp, w->start);

does not set startp to a valid charpos if w->start
points to uninitialized data.

A problem with valgrind is that this sort of analysis
can be very painful to do, if one wants to do it completely.
I found this out the hard way when trying to apply valgrind
to Emacs's garbage collector....

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[Eli Zaretskii]

> Date: Sun, 06 Nov 2011 22:08:54 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: 9948@debbugs.gnu.org
> 
>   SET_TEXT_POS_FROM_MARKER (startp, w->start);
> 
> does not set startp to a valid charpos if w->start
> points to uninitialized data.

w->startp is the window-start of the window represented by `w'.  It is
a very important player in displaying the window.  If that is
uninitialized, we have a much larger catastrophe on our hands than
some obscure local variable that _could_ be uninitialized ;-)

IOW, if w->startp does not have a valid value, Emacs will crash
pronto.

bug#9948: valgrind warning: Conditional jump or move depends on uninitialised value(s) in redisplay_window

[npostavs]

tags 9948 unreproducible
close 9948
quit

Dan Nicolaescu <dann@gnu.org> writes:

> It looks like there's a --track-origins=yes that might help.
> Now just need to reproduce the same issue (which occurred after quite a
> bit of random editing/playing around, so it might not be easy...).

I guess it was never reproduced.