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

[Guinevere Larsen <guinevere@redhat.com>]

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.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> ---
>   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(-)
>
> 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]