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

["Peikes, Wendy" <Wendy.Peikes@netapp.com>]

Thank you, Keith, for your interest in adding our NetApp environment var feature into the mainstream FSF gdb!

Answering your questions here with your verbiage preceded by >>


>>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.

Actually our patch uses getenv
char *var_value = getenv(varname);

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

Good idea to use convenience vars.  However, for our use case at NetApp, we absolutely need env vars. Our test infrastructure relies on env var settings.

Since you're asking for an opinion, I'll supply one: I believe that the feature is very useful as is, using env vars, and that adding env var support to a few more cmds would make it even more useful.  The convenience var model, while certainly useful, would not directly do what we need at NetApp.  

We could make the convenience var model work for us at NetApp by getting env var values brought in to convenience vars.  One way that comes to mind: have gdb set convenience vars with a certain name pattern (say "env_xyz") to the values of the corresponding env vars.  For example,  gdb automatically sets convenience var $env_xyz to the value of getenv("xyz").  

But then we'd have to implement the convenience var model that you noted below.  The associative array is a great idea.

But here, that's not what we want and need. Kind of like the old "engineering swing cartoon" (can google that if not familiar) where, in the end, the customer only wanted a tire swing.

How's about this:
- I submit based on the feature we find very useful (env var substitution)
- We upload the latest version of gdb and I create a patch based on it
- I fix the formatting errors, remove the fenceposts, and replace the fprintf with gdb_printf 
- I check from my end if we have an FSF assignment on file
 (Is there a way I can check from the RedHat end if NetApp has an FSF assignment?)
- I create a test for it. (Is there documentation on how to present a test for a patch?)
- Then I submit the updated patch (to newest gdb) and test

Does that work for a plan?

Wendy Peikes
Gdb Team
NetApp




-----Original Message-----
From: Keith Seitz <keiths@redhat.com> 
Sent: Thursday, October 6, 2022 12:35 PM
To: Peikes, Wendy <Wendy.Peikes@netapp.com>; gdb-patches@sourceware.org
Subject: Re: [PATCH] gdb: allow env var specifications in cmds 'set log', 'source', 'shell'

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.




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