public inbox for
 help / color / mirror / Atom feed
From: "Maciej W. Rozycki" <>
To: Tom Tromey <>
Cc:, Andrew Burgess <>,
	 Richard Bunt <>
Subject: Re: [PATCH v3 3/5] GDB: Only make data actually retrieved into value history available
Date: Fri, 10 Feb 2023 14:18:14 +0000 (GMT)	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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:

 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])
  (gdb) FAIL: gdb.python/flexible-array-member.exp: python print(ns['items'][0])

compared to:

  python print(ns['items'][0])
  (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.


  reply	other threads:[~2023-02-10 14:18 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-23 23:00 [PATCH v3 0/5] gdb: introduce limited array lengths while printing values Maciej W. Rozycki
2023-01-23 23:13 ` [PATCH v3 1/5] GDB: Ignore `max-value-size' setting with value history accesses Maciej W. Rozycki
2023-01-31 17:58   ` Tom Tromey
2023-02-10 14:17     ` Maciej W. Rozycki
2023-01-23 23:13 ` [PATCH v3 2/5] GDB: Fix the mess with value byte/bit range types Maciej W. Rozycki
2023-01-31 18:09   ` Tom Tromey
2023-02-10 14:18     ` Maciej W. Rozycki
2023-02-10 14:49       ` Tom Tromey
2023-01-23 23:14 ` [PATCH v3 3/5] GDB: Only make data actually retrieved into value history available Maciej W. Rozycki
2023-01-31 18:47   ` Tom Tromey
2023-02-10 14:18     ` Maciej W. Rozycki [this message]
2023-02-10 21:11       ` Tom Tromey
2023-01-23 23:14 ` [PATCH v3 4/5] GDB/testsuite: Add `-nonl' option to `gdb_test' Maciej W. Rozycki
2023-01-31 19:02   ` Tom Tromey
2023-01-23 23:14 ` [PATCH v3 5/5] GDB: Introduce limited array lengths while printing values Maciej W. Rozycki
2023-01-24 12:59   ` Eli Zaretskii
2023-01-31 20:49   ` Tom Tromey
2023-02-10 14:18     ` Maciej W. Rozycki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).