From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 462413858D37 for ; Thu, 6 Oct 2022 19:34:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 462413858D37 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-245-5f03rADFM7WndPKPsRR2xQ-1; Thu, 06 Oct 2022 15:34:48 -0400 X-MC-Unique: 5f03rADFM7WndPKPsRR2xQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 90E0385A583; Thu, 6 Oct 2022 19:34:47 +0000 (UTC) Received: from [10.2.17.232] (unknown [10.2.17.232]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3878C200D8C0; Thu, 6 Oct 2022 19:34:47 +0000 (UTC) Message-ID: Date: Thu, 6 Oct 2022 12:34:46 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.1 Subject: Re: [PATCH] gdb: allow env var specifications in cmds 'set log', 'source', 'shell' To: "Peikes, Wendy" , "gdb-patches@sourceware.org" References: From: Keith Seitz In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Oct 2022 19:34:54 -0000 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