Re: [PATCH v2 1/2] [gdb/testsuite] Simplify core_find

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> In proc core_find we have:
> ...
> catch "system \"(cd ${coredir}; ulimit -c unlimited; $coredump_filter_cmd; ${binfile} ${arg}; true) >${output_file} 2>&1\""
> ...
>
> We can rewrite this into something more readable with less quote and escape
> magic:

I'm not opposed to this change, but I don't understand the motivation.
Is the "magic" to which you refer the \" ?  Or maybe you getting ahead
of yourself and referencing the change in
gdb.base/corefile-exec-context.exp, which does seem like a nice
cleanup.  To me, the original core_find code was clearer, but the extra
complexity seems worth if for the improvements possible in the test
scripts.

> ...
> set arg [subst -nocommands -novariables $arg]
> set cmd [subst_vars {
>     (cd ${coredir};
>      ulimit -c unlimited;
>      $coredump_filter_cmd;
>      ${binfile} ${arg};
>      true) \
>         >${output_file} 2>&1}]
> catch {
>     system $cmd
> }
> ...
>
> The "set arg [subst ... $arg]" is a bit awkward, and dropping it allows us to
> update test-case gdb.base/corefile-exec-context.exp to use a bit more typical
> setup with string_to_regex.

typo: string_to_regex -> string_to_regexp

Thd discussion of "set arg [subst ... $arg]" confusing.  You introduce
it, then say it's dropped, but never explain why it was introduced, you
just assume the reason is self-evident.  It's not.  At least, not to
me.  I think you should probably just remove that line and explain why
the new code is better -- this would be great as others (me) could read
your commit and learn from it.

>
> Note that what is being tested hasn't changed.
>
> We can print the effective value of arg by adding to the command passed to
> system.  Without this patch using:
> ...
> echo \\\"$arg\\\;
> ...
> and with this patch using:
> ...
> echo "$arg";
> ...
> and in both cases we get:
> ...
> aaaaa bbbbb ccccc ddddd e\ e\ e\ e\ e
> ...
> ---
>  .../gdb.base/corefile-exec-context.exp        | 13 +++++++---
>  gdb/testsuite/lib/gdb.exp                     | 26 ++++++++++++++++---
>  2 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/corefile-exec-context.exp b/gdb/testsuite/gdb.base/corefile-exec-context.exp
> index 9b018533b68..56c68a6a5cd 100644
> --- a/gdb/testsuite/gdb.base/corefile-exec-context.exp
> +++ b/gdb/testsuite/gdb.base/corefile-exec-context.exp
> @@ -69,7 +69,7 @@ gdb_test_multiple "core-file $corefile_1" "load core file no args" {
>  }
>  
>  # Generate a core file, this time pass some arguments to the inferior.
> -set args "aaaaa bbbbb ccccc ddddd e\\\\ e\\\\ e\\\\ e\\\\ e"
> +set args {aaaaa bbbbb ccccc ddddd e\ e\ e\ e\ e}
>  set corefile [core_find $binfile {} $args]
>  if {$corefile == ""} {
>      untested "unable to create corefile"
> @@ -82,8 +82,11 @@ remote_exec build "mv $corefile $corefile_2"
>  # argument list are seen.
>  clean_restart $testfile
>  set saw_generated_line false
> +set re_args [string_to_regexp $args]
> +set re_cmd "[string_to_regexp $binfile] $re_args"
> +set re_line [subst_vars {^Core was generated by `$re_cmd'\.\r\n}]
>  gdb_test_multiple "core-file $corefile_2" "load core file with args" {
> -    -re "^Core was generated by `[string_to_regexp $binfile] $args'\\.\r\n" {
> +    -re $re_line {
>  	set saw_generated_line true
>  	exp_continue
>      }
> @@ -99,7 +102,8 @@ gdb_test_multiple "core-file $corefile_2" "load core file with args" {
>  
>  # Also, the argument list should be available through 'show args'.
>  gdb_test "show args" \
> -    "Argument list to give program being debugged when it is started is \"$args\"\\."
> +    [subst_vars \
> +	 {Argument list to give program being debugged when it is started is "$re_args"\.}]
>  
>  # Move up to 'main'.  Do it this way because we cannot know how many
>  # frames up 'main' actually is.
> @@ -178,8 +182,9 @@ proc check_for_env_var { var_name var_value } {
>  gdb_assert { ![check_for_env_var $env_var_name $env_var_value] } \
>      "environment variable is not set before core file load"
>  
> +set re_cmd "[string_to_regexp $binfile] $re_args"

I don't think this line is needed.  Has RE_CMD changed since it was
first computed?  I don't think BINFILE has, and RE_ARGS is new, and only
computed the once above.  If this line is necessary then maybe a comment
explaining why would be good.

Thank,
Andrew

>  gdb_test "core-file $corefile_3" \
> -    "Core was generated by `[string_to_regexp $binfile] $args'\\.\r\n.*" \
> +    [subst_vars {Core was generated by `$re_cmd'\.\r\n.*}] \
>      "load core file for environment test"
>  
>  gdb_assert { [check_for_env_var $env_var_name $env_var_value] } \
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 9ade9a16818..2cfdbdda09d 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -10312,8 +10312,18 @@ proc core_find {binfile {deletefiles {}} {arg ""} {output_file "/dev/null"}} {
>  	}
>      }
>  
> -    # tclint-disable command-args
> -    catch "system \"(cd ${coredir}; ulimit -c unlimited; $coredump_filter_cmd; ${binfile} ${arg}; true) >${output_file} 2>&1\""
> +    set cmd [subst_vars {
> +	(cd ${coredir};
> +	 ulimit -c unlimited;
> +	 $coredump_filter_cmd;
> +	 ${binfile} ${arg};
> +	 true) \
> +	    >${output_file} 2>&1}]
> +    verbose -log "Executing on build: $cmd"
> +    catch {
> +	system $cmd
> +    }
> +
>      #      remote_exec host "${binfile}"
>      set binfile_basename [file tail $binfile]
>      foreach i [list \
> @@ -10343,8 +10353,16 @@ proc core_find {binfile {deletefiles {}} {arg ""} {output_file "/dev/null"}} {
>  	# ulimit here if we didn't find a core file above.
>  	# Oh, I should mention that any "braindamaged" non-Unix system has
>  	# the same problem. I like the cd bit too, it's really neat'n stuff.
> -	# tclint-disable command-args
> -	catch "system \"(cd ${objdir}/${subdir}; ${binfile}; true) >/dev/null 2>&1\""
> +	set cmd [subst_vars {
> +	    (cd ${objdir}/${subdir};
> +	     ${binfile};
> +	     true) \
> +		>/dev/null 2>&1}]
> +	verbose -log "Executing on build: $cmd"
> +	catch {
> +	    system $cmd
> +	}
> +
>  	foreach i "${objdir}/${subdir}/core ${objdir}/${subdir}/core.coremaker.c ${binfile}.core" {
>  	    if {[remote_file build exists $i]} {
>  		remote_exec build "mv $i $destcore"
> -- 
> 2.51.0