Re: [PATCH] gdb: allow env var specifications in cmds 'set log', 'source', 'shell'

[Keith Seitz <keiths@redhat.com>]

On 10/5/22 16:19, Peikes, Wendy via Gdb-patches wrote:
> Rationale:
> 
> We at NetApp added a gdb feature allowing users to specify
> environment variable names when invoking gdb commands 'set logging
> file', 'source' & 'shell'.
> 
> This has been a big help especially in gdb macros since it allows
> macro writers to customize macro behavior with environment var
> settings.
> 
> We've implemented this feature for the above three gdb commands but
> it can easily be  extended to other commands such as 'set
> substitute-path'

Thank you for the patch. I agree with you: this seems like a very
useful feature that we should definitely be offering our users.

I've got a few basic comments and questions, if you don't mind.
[Warning: I am going to treat this patch more like an RFC.]

> Usage example below.
> 
> setenv LOG_DIR /tmp
> setenv SCRIPT_DIR "."
> 
> I've created a script 'macroScript' in the current directory ("."); it is referenced in the gdb session.
> 
> Now, run the gdb with the new env var feature:
> 
> gdbWithEnvVarFeature
> ....
> GNU gdb (GDB) 9.1
> Copyright (C) 2020 Free Software Foundation, Inc.
> ...
> 
> (gdb) shell head ${SCRIPT_DIR}/macroScript
> define macro1
>         echo macro1\n
> end
> define macro2
>         echo macro2\n
> end
> macro1
> macro2
> 

I believe this use case already works:

$ export MY_FILE=/tmp/myfile
$ echo "Hello, gdb!" > $MY_FILE
$ ./gdb -batch -ex 'shell cat $MY_FILE'
Hello, gdb!
$

> gdb) source ${SCRIPT_DIR}/macroScript
> macro1
> macro2
> (gdb) set logging file ${LOG_DIR}/logFile
> (gdb) show logging file
> The current logfile is "${LOG_DIR}/logFile".
> (gdb) set logging on
> Copying output to /tmp/logFile.
> Copying debug output to /tmp/logFile.

This is where things start to get interesting...

This patch uses "${" to refer to an environment variable, and
any command desiring to take advantage of this feature must call
contains_env_var and substitute_env_var.

I would think -- perhaps naively -- that implementing this via the
existing convenience variable system would be easier. Did you
consider or try that?

The convenience variable route would also automatically propagate
this new feature to every existing command that already accepts
convenience variables. [Then "source" et al could simply check for
convenience variable substitutions, fixing a bug in those commands,
which currently do not support convenience variables at all.]

This could be pretty easily (famous last words?) accomplished by
expanding the convenience variable API to accept associative arrays
and then internalizing all the environment variables into a new
convenience var called "env" or something descriptive.

Thus, we would use, for example, "set logfile $env(SCRIPT_DIR)/macroScript"
and so on. I think this form (or some variation of it) is more "GDB-like"
and much more explicit than "${}".

As a bonus, we would automatically gain, e.g., "print $env(LD_LIBRARY_PATH)".
AFAIK there is currently no way to use environment variables in the expression
parser.

That is probably being a little too dreamy-eyed, though. Nonetheless,
I would be curious what you and others think. I am asking for no more
than your opinion right now. [Actually I'm in no position to ask for
more than that, either! :-)]

> The patch to gdb9.1 is here; the changed lines are bracket by
> #if defined ALLOW_ENV_VARS
> #endif

If this patch/feature is accepted, you should remove these fenceposts.
I don't see why they'd be needed. Also, we're well past gdb 9 now. Please
rebase your patch to the master branch. [Warning: fprintf_unfiltered doesn't
exist anymore. Use gdb_printf (gdb_stderr, ...).]

Now on to a few comments.

First up, please take a look at the Contribution Checklist:

https://sourceware.org/gdb/wiki/ContributionChecklist

There are a number of formatting errors contained in the patch, so please
read the section of the Checklist dealing with that (and the GNU Coding Standard).
Unfortunately, we do not yet have a formatting script for C/C++ that can handle
this automatically for us. :-(

This is definitely a feature which should be documented in both the user manual
and NEWS. Additionally, I think this patch should be accompanied with tests.

Finally, if you/your employer do not have an FSF assignment on file, you will
need one for this contribution. [I see two previous NetApp gdb contributions in
2000/2001.] This isn't really something I deal with regularly, so I just
raise it as a caveat. This patch definitely does not qualify as "trivial"
IMO.

Thanks again for the patch/rfc. I hope we can get this useful feature in!
Keith