[v2,1/2,gdb/testsuite] Simplify core_find
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Test passed
|
Commit Message
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:
...
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.
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(-)
Comments
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
@@ -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"
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] } \
@@ -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"