Re: [PATCH v3 3/5] GDB: Only make data actually retrieved into value history available

["Maciej W. Rozycki" <macro@embecosm.com>]

On Tue, 31 Jan 2023, Tom Tromey wrote:

> Maciej> +#if !defined (LONG_MIN)                     /* 0x80000000 for 32-bits */
> Maciej> +#define	LONG_MIN ((long) (~(long) 0 ^ LONG_MAX))
> Maciej> +#endif
> 
> I wasn't aware of this code being here.
> 
> For LONG_*, I tend to think we don't need it.  <limits.h> should provide
> it, and gnulib seems to think there aren't any systems without this:
> 
> https://www.gnu.org/software/gnulib/manual/html_node/limits_002eh.html

 I guess it's still C thinking kicking around here at least on my side.  
Since we've switched to C++11 it's that language standard that we have 
been driven by, and it mandates the presence of these macros.  Except we 
need to switch to <climits>.

 I don't like to maintain a mess, so I'll repost this patch series with
a suitable update included.

> Maciej> +#if !defined (LONGEST_MIN)                 /* 0x8000000000000000 for 64-bits */
> Maciej> +#define	LONGEST_MIN ((LONGEST) (~(LONGEST) 0 ^ LONGEST_MAX))
> Maciej> +#endif
> 
> This one probably belongs in gdbsupport/common-types.h.
> 
> We should clean up this part of defs.h but I can do that.

 Already done with the upcoming <climits> update.

> Maciej> @@ -1950,6 +1955,15 @@ record_latest_value (struct value *val)
> Maciej>       a value on the value history never changes.  */
> Maciej>    if (value_lazy (val))
> Maciej>      value_fetch_lazy (val);
> Maciej> +
> Maciej> +  /* Don't pretend we have anything available there in the history beyond
> Maciej> +     the boundaries of the value recorded.  It's not like inferior memory
> Maciej> +     where there is actual stuff underneath.  */
> Maciej> +  ULONGEST length = value_enclosing_type (val)->length ();
> Maciej> +  mark_value_bits_unavailable (val, LONGEST_MIN, 0 ^ LONGEST_MIN);
> Maciej> +  mark_value_bits_unavailable (val, length * TARGET_CHAR_BIT,
> Maciej> +			       LONGEST_MAX - length * TARGET_CHAR_BIT);
> 
> I'm struggling to understand why this is needed.
> Like it seems to me that a value should always report that bytes outside
> of its content range are unavailable.

 I have investigated whether a check based solely on the length of the 
data type associated with the value recorded would do, but that triggered 
Python regressions:

FAIL: gdb.python/flexible-array-member.exp: python print(ns['items'][0])
FAIL: gdb.python/flexible-array-member.exp: python print(ns['items'][1])
FAIL: gdb.python/flexible-array-member.exp: python print(ns['items'][2])
FAIL: gdb.python/flexible-array-member.exp: python print(zs['items'][0])
FAIL: gdb.python/flexible-array-member.exp: python print(zs['items'][1])
FAIL: gdb.python/flexible-array-member.exp: python print(zs['items'][2])
FAIL: gdb.python/flexible-array-member.exp: python print(zso['items'][0])
FAIL: gdb.python/flexible-array-member.exp: python print(zso['items'][1])
FAIL: gdb.python/flexible-array-member.exp: python print(zso['items'][2])

where flexible array member elements are accessed like:

  (gdb) python ns = gdb.parse_and_eval('ns').dereference()
  (gdb) python print(ns['items'][0])

that is using a value associated with a Python variable created on the fly 
rather than a value history entry.  These now showed as <unavailable> and 
it broke the test cases, which expect actual values, i.e.:

  python print(ns['items'][0])
  <unavailable>
  (gdb) FAIL: gdb.python/flexible-array-member.exp: python print(ns['items'][0])

compared to:

  python print(ns['items'][0])
  101
  (gdb) PASS: gdb.python/flexible-array-member.exp: python print(ns['items'][0])

 So instead I looked into making value history entries a distinct lvalue 
type (it would have a nice side effect of showing "no such vector element" 
when accessing out-of-bounds array elements) replacing `lval_memory', 
`lval_register', etc.  It might also cure the history mutability issue I 
have observed (for which I have filed PR exp/30107 to track BTW).  But 
while possibly a nice clean-up I realised it would be a major task, so 
it's something perhaps for another occasion.

 So finally I came up with just a boolean flag set for entries recorded in 
the value history.  It has passed my regression testing now and I'll be 
posting it shortly along with the rest of the patches.  Unless I hear to 
the contrary, I'm going to follow your earlier approval for the changes 
already handled, but this gives you or someone else a chance to chime in 
just in case an issue gets spotted.

 Thank you for your review.

  Maciej