Re: [PATCH] Handle partially optimized out values similarly to unavailable values (Re: [patchv2] Fix crash on optimized-out entry data values)

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Mon, 14 Jul 2014 20:11:41 +0200, Pedro Alves wrote:
[...]
> --- a/gdb/value.c
> +++ b/gdb/value.c
[...]
> @@ -339,6 +330,15 @@ struct value
>       value to be available.  This is filled in at value read time.  The
>       unavailable ranges are tracked in bits.  */
>    VEC(range_s) *unavailable;
> +
> +  /* Likewise, but for optimized out contents (a chunk of the value of
> +     a variable that does not actually exist in the program).  If LVAL
> +     is lval_register, this is a register ($pc, $sp, etc., never a
> +     program variable) that has not been saved in the frame.  Not
> +     saved registers and optimized-out program variables values are
> +     treated pretty much the same, except not-saved registers have a
> +     different string representation and related error strings.  */
> +  VEC(range_s) *optimized_out;

I miss here explicit description if a bit of the range can appear on both 
'unavailable' and 'optimized_out'.  IMO it should not.


>  };
>  
>  int
[...]
> +/* Helper function for value_contents_eq.  The only difference is that
> +   this function is bit rather than byte based.
> +
> +   Compare LENGTH bits of VAL1's contents starting at OFFSET1 bits
> +   with LENGTH bits of VAL2's contents starting at OFFSET2 bits.
> +   Return true if the available bits match.  */
> +
> +static int
> +value_contents_bits_eq (const struct value *val1, int offset1,
> +			const struct value *val2, int offset2,
> +			int length)
> +{
> +  /* Each array element corresponds to a ranges source (unavailable,
> +     optimized out).  '1' is for VAL1, '2' for VAL2.  */
> +  struct ranges_and_idx rp1[2], rp2[2];
> +
> +  /* See function description in value.h.  */
> +  gdb_assert (!val1->lazy && !val2->lazy);
> +
> +  /* We shouldn't be trying to compare past the end of the values.  */
> +  gdb_assert (offset1 + length
> +	      <= TYPE_LENGTH (val1->enclosing_type) * TARGET_CHAR_BIT);
> +  gdb_assert (offset2 + length
> +	      <= TYPE_LENGTH (val2->enclosing_type) * TARGET_CHAR_BIT);
> +
> +  memset (&rp1, 0, sizeof (rp1));
> +  memset (&rp2, 0, sizeof (rp2));
> +  rp1[0].ranges = val1->unavailable;
> +  rp2[0].ranges = val2->unavailable;
> +  rp1[1].ranges = val1->optimized_out;
> +  rp2[1].ranges = val2->optimized_out;
> +
> +  while (length > 0)
> +    {
> +      ULONGEST l, h;
> +      int i;
> +
> +      for (i = 0; i < 2; i++)
> +	{
> +	  ULONGEST l_tmp, h_tmp;
> +
> +	  /* The contents only match equal if the invalid/unavailable
> +	     contents ranges match as well.  */
> +	  if (!find_first_range_overlap_and_match (&rp1[i], &rp2[i],
> +						   offset1, offset2, length,
> +						   &l_tmp, &h_tmp))
> +	    return 0;
> +
> +	  /* We're interested in the lowest/first range found.  */
> +	  if (i == 0 || l_tmp < l)

Here should be:
   	  if (i == 0 || l_tmp < l || (l_tmp == l && h_tmp < h))

It could skip [0]'s part which is non-equal otherwise.


> +	    {
> +	      l = l_tmp;
> +	      h = h_tmp;
> +	    }
> +	}
> +
> +      /* Compare the available/valid contents.  */
>        if (memcmp_with_bit_offsets (val1->contents, offset1,
> -				   val2->contents, offset2, l1) != 0)
> +				   val2->contents, offset2, l) != 0)
>  	return 0;
>  
> -      length -= h1;
> -      offset1 += h1;
> -      offset2 += h1;
> +      length -= h;
> +      offset1 += h;
> +      offset2 += h;
>      }
>  
>    return 1;
>  }
[...]
> @@ -1566,7 +1671,6 @@ value_copy (struct value *arg)
>    VALUE_FRAME_ID (val) = VALUE_FRAME_ID (arg);
>    VALUE_REGNUM (val) = VALUE_REGNUM (arg);
>    val->lazy = arg->lazy;
> -  val->optimized_out = arg->optimized_out;

Why 'optimized_out' no longer needs to be copied?


>    val->embedded_offset = value_embedded_offset (arg);
>    val->pointed_to_offset = arg->pointed_to_offset;
>    val->modifiable = arg->modifiable;
[...]
> @@ -3662,10 +3753,11 @@ value_fetch_lazy (struct value *val)
>        if (value_lazy (parent))
>  	value_fetch_lazy (parent);
>  
> -      if (!value_bits_valid (parent,
> -			     TARGET_CHAR_BIT * offset + value_bitpos (val),
> -			     value_bitsize (val)))
> -	set_value_optimized_out (val, 1);
> +      if (value_bits_any_optimized_out (parent,
> +					TARGET_CHAR_BIT * offset + value_bitpos (val),
> +					value_bitsize (val)))
> +	mark_value_bytes_optimized_out (val, value_embedded_offset (val),
> +					TYPE_LENGTH (type));

That was required before but it could handle partially optimized out
bitfields; but maybe it is not useful much.


>        else if (!unpack_value_bits_as_long (value_type (val),
>  				      value_contents_for_printing (parent),
>  				      offset,
[...]
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -33,6 +33,43 @@ struct language_defn;
>  struct value_print_options;
>  struct xmethod_worker;
>  
> +/* Values can be partially 'optimized out' and/or 'unavailable'.
> +   These are distinct states and have different string representations
> +   and related error strings.
> +
> +   'unavailable' has a specific meaning in this context.  It means the
> +   value exists in the program (at the machine level), but GDB has no
> +   means to get to it.  Such a value is normally printed as.  Examples

"printed as." - is the sentence finished?


> +   of how to end up with an unavailable value would be:
> +
> +    - We're inspecting a traceframe, and the memory or registers the
> +      debug information says the value lives on haven't been collected.
> +
> +    - We're inspecting a core dump, the memory or registers the debug
> +      information says the value lives aren't present in the dump
> +      (that is, we have a partial/trimmed core dump, or we don't fully
> +      understand/handle the core dump's format).
> +
> +    - We're doing live debugging, but the debug API has no means to
> +      get at where the value lives in the machine, like e.g., ptrace
> +      not having access to some register or register set.
> +
> +    - Any other similar scenario.
> +
> +  OTOH, "optimized out" is about what the compiler decided to generate
> +  (or not generate).  A chunk of a value that was optimized out does
> +  not actually exist in the program.  There's no way to get at it
> +  short of compiling the program differently.  A register that has not
> +  been saved in a frame is likewise considered optimized out, except
> +  not-saved registers have a different string representation and
> +  related error strings.  E.g., we'll print them as <not-saved>

s/<not-saved>/<unavailable>/?


> +  instead of <optimized out>.
> +
> +  When comparing value contents, optimized out chunks, unavailable
> +  chunks, and valid contents data are all all considered different.
> +  See value_contents_eq for more info.
> +*/
> +
>  /* The structure which defines the type of a value.  It should never
>     be possible for a program lval value to survive over a call to the
>     inferior (i.e. to be put into the history list or an internal
[...]
> @@ -473,35 +508,53 @@ extern void mark_value_bits_unavailable (struct value *value,
>     its enclosing type chunk, you'd do:
>  
>       int len = TYPE_LENGTH (check_typedef (value_enclosing_type (val)));
> -     value_available_contents (val, 0, val, 0, len);
> +     value_contents_eq (val, 0, val, 0, len);
> +
> +   Returns true iff the set of available/valid contents match.
> +
> +   Optimized out contents compare equal with optimized out contents,
> +   and different with any non-optimized-out byte.

English: s/different/differ/

>  
> -   Returns true iff the set of available contents match.  Unavailable
> -   contents compare equal with unavailable contents, and different
> -   with any available byte.  For example, if 'x's represent an
> -   unavailable byte, and 'V' and 'Z' represent different available
> -   bytes, in a value with length 16:
> +   Unavailable contents compare equal with unavailable contents, and
> +   different with any non-unavailable byte.

English: s/different/differ/

>  
> -   offset:   0   4   8   12  16
> -   contents: xxxxVVVVxxxxVVZZ
> +   For example, if 'x's represent an unavailable byte, and 'V' and 'Z'
> +   represent different available/valid bytes, in a value with length
> +   16:
> +
> +     offset:   0   4   8   12  16
> +     contents: xxxxVVVVxxxxVVZZ
>  
>     then:
>  
> -   value_available_contents_eq(val, 0, val, 8, 6) => 1
> -   value_available_contents_eq(val, 0, val, 4, 4) => 1
> -   value_available_contents_eq(val, 0, val, 8, 8) => 0
> -   value_available_contents_eq(val, 4, val, 12, 2) => 1
> -   value_available_contents_eq(val, 4, val, 12, 4) => 0
> -   value_available_contents_eq(val, 3, val, 4, 4) => 0
> -
> -   We only know whether a value chunk is available if we've tried to
> -   read it.  As this routine is used by printing routines, which may
> -   be printing values in the value history, long after the inferior is
> -   gone, it works with const values.  Therefore, this routine must not
> -   be called with lazy values.  */
> -
> -extern int value_available_contents_eq (const struct value *val1, int offset1,
> -					const struct value *val2, int offset2,
> -					int length);
> +     value_contents_eq(val, 0, val, 8, 6) => 1
> +     value_contents_eq(val, 0, val, 4, 4) => 0
> +     value_contents_eq(val, 0, val, 8, 8) => 0
> +     value_contents_eq(val, 4, val, 12, 2) => 1
> +     value_contents_eq(val, 4, val, 12, 4) => 0
> +     value_contents_eq(val, 3, val, 4, 4) => 0
> +
> +   If 'x's represent an unavailable byte, 'o' represents an optimized
> +   out byte, in a value with length 8:
> +
> +   offset:   0   4   8
> +   contents: xxxxoooo
> +
> +   then:
> +
> +     value_contents_eq(val, 0, val, 2, 2) => 1
> +     value_contents_eq(val, 4, val, 6, 2) => 1
> +     value_contents_eq(val, 0, val, 4, 4) => 0
> +
> +   We only know whether a value chunk is unavailable or optimized out
> +   if we've tried to read it.  As this routine is used by printing
> +   routines, which may be printing values in the value history, long
> +   after the inferior is gone, it works with const values.  Therefore,
> +   this routine must not be called with lazy values.  */
[...]


This patch is apparently not suitable for gdb-7.8 which is I guess often
crashing on -O2 -g entry values so there could be some rather minimal crash
avoiding patch instead.


Thanks,
Jan