[v6,1/3] gdb: Do not create variables when parsing expressions

  We do this by introducing a new type of expression for internalvars
that had not been given a value at parse time.

This is not particularly useful on its own, but allows us to parse
expressions without having side-effects on the set of internalvars.

This will then allow us to tab-complete internalvars in expressions
without creating new internalvars, which would otherwise defeat the
purpose of tab completion (completing only variables that have been
set).

In addition, this allows us to make the 'init-if-undefined' command
behave as it is documented to: previously, it treated convenience
vars that have been set (or re-set) to void as undefined. Now, it
only initialized variables that have actually never been defined.

The old behavior can be accessed by instead guarding assignment to
a convenience variable with 'if($_isvoid($var))'.
---
 gdb/NEWS            |  5 ++++
 gdb/ada-exp.h       | 12 +++++++--
 gdb/ax-gdb.c        | 61 +++++++++++++++++++++++++++++++++++++--------
 gdb/cli/cli-utils.c |  2 +-
 gdb/expop.h         | 55 +++++++++++++++++++++++++++++++++++++++-
 gdb/parse.c         |  7 +++---
 gdb/value.c         | 23 ++++++++---------
 7 files changed, 135 insertions(+), 30 deletions(-)
  

> Date: Sun, 06 Apr 2025 01:38:59 +0000
> From: Antonio Rische <nt8r@protonmail.com>
> Cc: Antonio Rische <nt8r@protonmail.com>
> 
>  gdb/NEWS            |  5 ++++
>  gdb/ada-exp.h       | 12 +++++++--
>  gdb/ax-gdb.c        | 61 +++++++++++++++++++++++++++++++++++++--------
>  gdb/cli/cli-utils.c |  2 +-
>  gdb/expop.h         | 55 +++++++++++++++++++++++++++++++++++++++-
>  gdb/parse.c         |  7 +++---
>  gdb/value.c         | 23 ++++++++---------
>  7 files changed, 135 insertions(+), 30 deletions(-)

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 6a557bb4a..07c1193d8 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -116,6 +116,11 @@ qXfer:threads:read
>    subsystem to be disabled at configure time, in the form of
>    --disable-gdb-compile.
>  
> +* The =init-if-undefined command now strictly obeys its documentation to
         ^
That "=" should be removed, I believe.

> +  initialize only convenience variables that have not been set. It
                                                                 ^^
Please leave 2 spaces between sentences, per our conventions.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  

Hi,

Thank you for another revision. I'd really like to see this land!

On 4/5/25 6:38 PM, Antonio Rische wrote:
> We do this by introducing a new type of expression for internalvars
> that had not been given a value at parse time.
> 

I hate to be "that person," but the commit logs for this series
do not read well. I'm not asking for any changes, but please keep
in mind for the future that reading the log should not require
the subject. It's just awkward IMO.

> This is not particularly useful on its own, but allows us to parse
> expressions without having side-effects on the set of internalvars.
> 
> This will then allow us to tab-complete internalvars in expressions
> without creating new internalvars, which would otherwise defeat the
> purpose of tab completion (completing only variables that have been
> set).
> 
> In addition, this allows us to make the 'init-if-undefined' command
> behave as it is documented to: previously, it treated convenience
> vars that have been set (or re-set) to void as undefined. Now, it
> only initialized variables that have actually never been defined.

"initializes"?

> diff --git a/gdb/parse.c b/gdb/parse.c
> index ffefe6fee..8de3e19d2 100644
> --- a/gdb/parse.c
> +++ b/gdb/parse.c
> @@ -240,10 +240,9 @@ parser_state::push_dollar (struct stoken str)
>         return;
>       }
>   
> -  /* Any other names are assumed to be debugger internal variables.  */
> -
> -  push_new<expr::internalvar_operation>
> -    (create_internalvar (copy.c_str () + 1));
> +  /* Any other names are assumed to be debugger internal variables which
> +     have not yet been initialized.  */
> +  push_new<expr::uninit_internalvar_operation> (copy.c_str () + 1);
>   }
>   
>   /* See parser-defs.h.  */

In v5 Tom raised a really good question:

"Really what I'm asking is: if you apply your patch, then change the
last spot using internalvar_operation to instead use the new
uninit_internalvar_operation, does anything fail?"

For giggles, I hacked at parser_state::push_dollar to do just this
and did not detect any test failures, suggesting that we can, indeed,
adapt the existing class's behavior instead of introducing a new
class.

I'd like to ask again: Is there some use case that this would break or
any reason you can think of that would/should prevent modifying
internalvar_operation instead?

Keith
  

Thanks for the feedback, I really appreciate it.

I'll reword the commit messages; I had no idea of the convention to
make body independent of the title. Thanks for the typo catch on
"initialized" that should be "initializes".

With respect to Tom's question on v5: No, no tests fail on master
plus this changeset (up to v6) if we replace the last use of
internalvar_operation with uninit_internalvar_operation.

But if we add the following test cases:

diff --git a/gdb/testsuite/gdb.base/gdbvars.exp b/gdb/testsuite/gdb.base/gdbvars.exp
index 7ec7df36b..1902833e4 100644
--- a/gdb/testsuite/gdb.base/gdbvars.exp
+++ b/gdb/testsuite/gdb.base/gdbvars.exp
@@ -52,6 +52,26 @@ proc test_convenience_variables {} {
     gdb_test "print \$bar"             " = void" \
        "Print contents of uninitialized convenience variable"
 
+    gdb_test_no_output "init-if-undefined \$bar = 1" \
+       "Set a convenience value only if undefined"
+
+    gdb_test "print \$bar"             " = 1" \
+       "Check that init-if-undefined set undefined var"
+
+    gdb_test_no_output "init-if-undefined \$bar = 2" \
+       "Set a convenience var holding integer only if undefined"
+
+    gdb_test "print \$bar"             " = 1" \
+       "Check that init-if-undefined did not overwrite integer var"
+
+    gdb_test_no_output "set \$bar = (void)0" \
+       "Set the convenience value to void"
+
+    gdb_test_no_output "init-if-undefined \$bar = 3" \
+       "Set a convenience var holding void only if undefined"
+
+    gdb_test "print \$bar"             " = void" \
+       "Check that init-if-undefined did not overwrite void var"
 
     gdb_test "print \$str = \"some string\"" \
        " = \"some string\"" \


(Which I believe are a good idea especially as 'init-if-undefined'
seems to have no tests at present; I'll put them in v7) then tests
fail prior to commit #1 of this series, pass afterward, and fail again
after subsequently making the suggested change of the last usage of
internalvar_operation to uninit_internalvar_operation instead:

diff --git a/gdb/parse.c b/gdb/parse.c
index 8de3e19d2..70e1196af 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -218,7 +218,7 @@ parser_state::push_dollar (struct stoken str)
   isym = lookup_only_internalvar (copy.c_str () + 1);
   if (isym)
     {
-      push_new<expr::internalvar_operation> (isym);
+      push_new<expr::uninit_internalvar_operation> (copy.c_str () + 1);
       return;
     }
 

That said, creating a new expr::*_operation class is not the only
possible way we could distinguish between expressions reading from
internalvars that did or did not exist at parse-time. We could fold
them into one class and distinguish with a field rather than with
a distinct subtype.

But I think doing so leads to worse code. Both types inherit from
tuple_holding_operation<T> for different types T; for uninitialized
internalvars we hold the name of the variable as a std::string and
look it up at evaluation time based on its name as a string. For
internalvars that existed at parse time, we simply hold a pointer
to the internalvar.

Holding a string in all cases and always performing late lookup of
the internalvar itself could work for both cases; we would just add
a boolean field to distinguish the two, e.g. for init-if-undefined.
But it's less elegant to defer lookup to expression evaluation time
when we can do it in most cases at parse time. And to avoid changing
the current behavior of `parser_state::push_dollar` would still have
to call `lookup_only_internalvar` at parse time because internalvars
(if they exist) shadow symbols that start with '$' as handled by the
code below the comment about "HP-UX and hppa-linux".

Again, if this is really a big deal I can rewrite the patch as
described above with one class, late lookup and a boolean. Or even
with one class, eager-*or*-late lookup, and a std::variant. But to
me personally this seems like splitting hairs.

Anyhow, thanks for the careful review.

Antonio

On Friday, April 11th, 2025 at 7:54 PM, Keith Seitz <keiths@redhat.com> wrote:

> Hi,
> 
> Thank you for another revision. I'd really like to see this land!
> 
> On 4/5/25 6:38 PM, Antonio Rische wrote:
> 
> > We do this by introducing a new type of expression for internalvars
> > that had not been given a value at parse time.
> 
> 
> I hate to be "that person," but the commit logs for this series
> do not read well. I'm not asking for any changes, but please keep
> in mind for the future that reading the log should not require
> the subject. It's just awkward IMO.
> 
> > This is not particularly useful on its own, but allows us to parse
> > expressions without having side-effects on the set of internalvars.
> > 
> > This will then allow us to tab-complete internalvars in expressions
> > without creating new internalvars, which would otherwise defeat the
> > purpose of tab completion (completing only variables that have been
> > set).
> > 
> > In addition, this allows us to make the 'init-if-undefined' command
> > behave as it is documented to: previously, it treated convenience
> > vars that have been set (or re-set) to void as undefined. Now, it
> > only initialized variables that have actually never been defined.
> 
> 
> "initializes"?
> 
> > diff --git a/gdb/parse.c b/gdb/parse.c
> > index ffefe6fee..8de3e19d2 100644
> > --- a/gdb/parse.c
> > +++ b/gdb/parse.c
> > @@ -240,10 +240,9 @@ parser_state::push_dollar (struct stoken str)
> > return;
> > }
> > 
> > - /* Any other names are assumed to be debugger internal variables. /
> > -
> > - push_newexpr::internalvar_operation
> > - (create_internalvar (copy.c_str () + 1));
> > + / Any other names are assumed to be debugger internal variables which
> > + have not yet been initialized. */
> > + push_newexpr::uninit_internalvar_operation (copy.c_str () + 1);
> > }
> > 
> > /* See parser-defs.h. */
> 
> 
> In v5 Tom raised a really good question:
> 
> "Really what I'm asking is: if you apply your patch, then change the
> last spot using internalvar_operation to instead use the new
> uninit_internalvar_operation, does anything fail?"
> 
> For giggles, I hacked at parser_state::push_dollar to do just this
> and did not detect any test failures, suggesting that we can, indeed,
> adapt the existing class's behavior instead of introducing a new
> class.
> 
> I'd like to ask again: Is there some use case that this would break or
> any reason you can think of that would/should prevent modifying
> internalvar_operation instead?
> 
> Keith