diff mbox series

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

Message ID 20241220200501.324191-2-christina.schimpe@intel.com
State New
Headers
Series Add CET shadow stack support |

Commit Message

Christina Schimpe Dec. 20, 2024, 8:04 p.m. UTC 
  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.
---
 gdb/testsuite/gdb.base/libsegfault.exp            | 2 +-
 gdb/testsuite/gdb.threads/attach-slow-waitpid.exp | 2 +-
 gdb/testsuite/lib/gdb.exp                         | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

Comments

Guinevere Larsen Jan. 28, 2025, 1:45 p.m. UTC | #1 
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.
Christina Schimpe Jan. 30, 2025, 1:07 p.m. UTC | #2 
> -----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?

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
Tom de Vries Jan. 30, 2025, 2:27 p.m. UTC | #3 
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
Christina Schimpe Jan. 30, 2025, 4:39 p.m. UTC | #4 
> -----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
diff mbox series

Patch

diff --git a/gdb/testsuite/gdb.base/libsegfault.exp b/gdb/testsuite/gdb.base/libsegfault.exp
index 2c16fe8932a..fb62bdb8746 100644
--- a/gdb/testsuite/gdb.base/libsegfault.exp
+++ b/gdb/testsuite/gdb.base/libsegfault.exp
@@ -42,7 +42,7 @@  proc gdb_spawn_with_ld_preload {lib cmdline_opts} {
 	# ASan runtime does not come first in initial library list; you should
 	# either link runtime to your application or manually preload it with
 	# LD_PRELOAD.
-	set_sanitizer_default ASAN_OPTIONS verify_asan_link_order 0
+	append_environment ASAN_OPTIONS verify_asan_link_order 0
 
 	gdb_spawn_with_cmdline_opts $cmdline_opts
     }
diff --git a/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp b/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
index 28d70daad8c..abe8d434558 100644
--- a/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
+++ b/gdb/testsuite/gdb.threads/attach-slow-waitpid.exp
@@ -83,7 +83,7 @@  proc gdb_spawn_with_ld_preload {lib} {
 	# ASan runtime does not come first in initial library list; you should
 	# either link runtime to your application or manually preload it with
 	# LD_PRELOAD.
-	set_sanitizer_default ASAN_OPTIONS verify_asan_link_order 0
+	append_environment ASAN_OPTIONS verify_asan_link_order 0
 
 	gdb_start
     }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 7ee2043f0f8..a86f534528c 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -76,11 +76,11 @@  proc set_sanitizer { env_var var_id val } {
 
 # Add VAR_ID=VAL to ENV_VAR, unless ENV_VAR already contains a VAR_ID setting.
 
-proc set_sanitizer_default { env_var var_id val } {
+proc append_environment { env_var var_id val } {
     set_sanitizer_1 $env_var $var_id $val 1
 }
 
-set_sanitizer_default TSAN_OPTIONS suppressions \
+append_environment TSAN_OPTIONS suppressions \
     $srcdir/../tsan-suppressions.txt
 
 # When using ThreadSanitizer we may run into the case that a race is detected,
@@ -89,14 +89,14 @@  set_sanitizer_default TSAN_OPTIONS suppressions \
 # Try to prevent this by setting history_size to the maximum (7) by default.
 # See also the ThreadSanitizer docs (
 # https://github.com/google/sanitizers/wiki/ThreadSanitizerFlags ).
-set_sanitizer_default TSAN_OPTIONS history_size 7
+append_environment TSAN_OPTIONS history_size 7
 
 # If GDB is built with ASAN (and because there are leaks), it will output a
 # leak report when exiting as well as exit with a non-zero (failure) status.
 # This can affect tests that are sensitive to what GDB prints on stderr or its
 # exit status.  Add `detect_leaks=0` to the ASAN_OPTIONS environment variable
 # (which will affect any spawned sub-process) to avoid this.
-set_sanitizer_default ASAN_OPTIONS detect_leaks 0
+append_environment ASAN_OPTIONS detect_leaks 0
 
 # List of procs to run in gdb_finish.
 set gdb_finish_hooks [list]