RE: [PATCH 01/12] gdb, testsuite: Rename set_sanitizer_default to append_environment.

["Schimpe, Christina" <christina.schimpe@intel.com>]

> -----Original Message-----
> From: Tom de Vries <tdevries@suse.de>
> Sent: Thursday, January 30, 2025 3:27 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; Guinevere Larsen
> <guinevere@redhat.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 01/12] gdb, testsuite: Rename set_sanitizer_default to
> append_environment.
> 
> On 1/30/25 14:07, Schimpe, Christina wrote:
> >> -----Original Message-----
> >> From: Guinevere Larsen <guinevere@redhat.com>
> >> Sent: Tuesday, January 28, 2025 2:45 PM
> >> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> >> patches@sourceware.org
> >> Subject: Re: [PATCH 01/12] gdb, testsuite: Rename
> >> set_sanitizer_default to append_environment.
> >>
> >> On 12/20/24 5:04 PM, Schimpe, Christina wrote:
> >>> The procedure set_sanitizer_default has been used for the
> >>> configuration of ASAN specific environment variables.  However, it
> >>> is actually a generic function.  Rename it to append_environment so
> >>> that its purpose is more clear.
> >>
> >> I think this is a good change, but in that case we should also rename
> >> set_sanitizer_1 and set_sanitizer. I think the conversion should be:
> >>
> >> set_sanitizer -> append_environment
> >> set_sanitizer_1 -> append_environment_1 set_sanitizer_default ->
> >> append_environment_default
> >>
> >> Also, I can see that patch 12 (and maybe others) use the
> >> append_environment call. If the user had already set GLIBC_TUNABLES,
> >> the test wouldn't update the value if I understand the TCL code correctly.
> >> Is that the expected outcome? If not, I would suggest the alternative
> >> of renaming
> >> set_sanitizer_1 to append_environment, so that set_sanitizer* can
> >> continue to work as is, and you can manually set the environment
> >> variables in a more obvious way.
> >
> > Thank you for the feedback. Yes, if the environment variable is
> > already configured we don't configure it by calling
> > set_sanitizer_default/append_environment_default, in contrast to
> > set_sanitizer_1.  I think set_sanitizer_1 was introduced later, that's why I
> probably missed it.
> >
> > So I totally agree, we should rename all functions and your suggestion
> >
> >> set_sanitizer -> append_environment
> >> set_sanitizer_1 -> append_environment_1 set_sanitizer_default ->
> >> append_environment_default
> >
> > makes sense to me.
> >
> > This patch is actually independent of this series and wonder if I could post it
> separately, as I expect to merge it sooner then.
> > I am adding Tom to this conversation who introduced these procedures. Does
> the renaming as suggested above make sense to you?
> >
> 
> Hi Christina,
> 
> yes, the renaming is fine.
> 
> I think it's not a bad idea to post this independently.
> 
> Thanks,
> - Tom

Thank you for the quick feedback. I posted the patch now separately:
https://sourceware.org/pipermail/gdb-patches/2025-January/215097.html

Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928