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

[Tom de Vries <tdevries@suse.de>]

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

> Thanks!
> 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