diff mbox

[v2] Fix argument passing in mi_run_cmd_full

Message ID 1400017636-28020-1-git-send-email-simon.marchi@ericsson.com
State Committed
Headers

Commit Message

Simon Marchi May 13, 2014, 9:47 p.m. UTC 
  Again, this patch was accepted almost a year ago, I want to make sure
that it is still OK.

Original submission: https://sourceware.org/ml/gdb-patches/2013-06/msg00739.html

Passing arguments did not work when use_mi_command was set.

gdb/testsuite/ChangeLog:
2014-05-13  Simon Marchi  <simon.marchi@ericsson.com>

	* lib/mi-support.exp (mi_run_cmd_full): Set arguments by calling
	"-exec-arguments" or "set args" before running the inferior.
---
 gdb/testsuite/lib/mi-support.exp | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Tom Tromey May 15, 2014, 6:40 p.m. UTC | #1 
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> Again, this patch was accepted almost a year ago, I want to make sure
Simon> that it is still OK.

Simon> Original submission:
Simon> https://sourceware.org/ml/gdb-patches/2013-06/msg00739.html

Simon> 2014-05-13  Simon Marchi  <simon.marchi@ericsson.com>

Simon> 	* lib/mi-support.exp (mi_run_cmd_full): Set arguments by calling
Simon> 	"-exec-arguments" or "set args" before running the inferior.

Yes, still ok, thank you.

Tom
Simon Marchi May 15, 2014, 7:46 p.m. UTC | #2 
On 14-05-15 02:40 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Simon> Again, this patch was accepted almost a year ago, I want to make sure
> Simon> that it is still OK.
> 
> Simon> Original submission:
> Simon> https://sourceware.org/ml/gdb-patches/2013-06/msg00739.html
> 
> Simon> 2014-05-13  Simon Marchi  <simon.marchi@ericsson.com>
> 
> Simon> 	* lib/mi-support.exp (mi_run_cmd_full): Set arguments by calling
> Simon> 	"-exec-arguments" or "set args" before running the inferior.
> 
> Yes, still ok, thank you.
> 
> Tom
> 

Pushed, thanks!
Tom Tromey May 19, 2014, 7:39 p.m. UTC | #3 
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> 2014-05-13  Simon Marchi  <simon.marchi@ericsson.com>
Simon> 	* lib/mi-support.exp (mi_run_cmd_full): Set arguments by calling
Simon> 	"-exec-arguments" or "set args" before running the inferior.

This patch causes a test failure here:

Running ../../../binutils-gdb/gdb/testsuite/gdb.mi/mi-start.exp ...
FAIL: gdb.mi/mi-start.exp: run to main (unknown output after running)

This failure isn't present in the previous revision.

I've appended the relevant section of gdb.log in case that is helpful.

Tom

(gdb) 
220-exec-run
=thread-group-started,id="i1",pid="21201"
=thread-created,id="1",group-id="i1"
220^running
*running,thread-id="all"
(gdb) 
mi_expect_stop: expecting: \*stopped,reason="breakpoint-hit",disp="del",bkptno="
[0-9]+",frame={addr="0x[0-9A-Fa-f]+",func="main",args=\[\],(?:file="[^
]*.*mi-start.c",fullname="(/[^\n]*/|\\\\[^\\]+\\[^\n]+\\|\\[^\\][^\n]*\\|[a-zA-Z
]:[^\n]*\\).*mi-start.c",line="[0-9]+"|from=".*mi-start.c")},thread-id="[0-9]+",
stopped-threads=[^
]*
(=thread-selected,id="[0-9]+"
|=(?:breakpoint-created|breakpoint-deleted)[^
]+"
)*[(]gdb[)] 
$
=library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x8
6-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-gro
up="i1"
=library-loaded,id="/lib64/libm.so.6",target-name="/lib64/libm.so.6",host-name="
/lib64/libm.so.6",symbols-loaded="0",thread-group="i1"
=library-loaded,id="/lib64/libc.so.6",target-name="/lib64/libc.so.6",host-name="
/lib64/libc.so.6",symbols-loaded="0",thread-group="i1"
=thread-exited,id="1",group-id="i1"
=thread-group-exited,id="i1",exit-code="0"
*stopped,reason="exited-normally"
(gdb) 
got =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linu
x-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread
-group="i1"
=library-loaded,id="/lib64/libm.so.6",target-name="/lib64/libm.so.6",host-name="
/lib64/libm.so.6",symbols-loaded="0",thread-group="i1"
=library-loaded,id="/lib64/libc.so.6",target-name="/lib64/libc.so.6",host-name="
/lib64/libc.so.6",symbols-loaded="0",thread-group="i1"
=thread-exited,id="1",group-id="i1"
=thread-group-exited,id="i1",exit-code="0"
*stopped,reason="exited-normally"
(gdb) 

FAIL: gdb.mi/mi-start.exp: run to main (unknown output after running)
Simon Marchi May 20, 2014, 4:40 p.m. UTC | #4 
On Mon 19 May 2014 03:39:09 PM EDT, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
>
> Simon> 2014-05-13  Simon Marchi  <simon.marchi@ericsson.com>
> Simon> 	* lib/mi-support.exp (mi_run_cmd_full): Set arguments by calling
> Simon> 	"-exec-arguments" or "set args" before running the inferior.
>
> This patch causes a test failure here:

Ehh, that's what I get for not re-running the testsuite, even after a 
year. I will send something soon about that.

> Running ../../../binutils-gdb/gdb/testsuite/gdb.mi/mi-start.exp ...
> FAIL: gdb.mi/mi-start.exp: run to main (unknown output after running)
>
> This failure isn't present in the previous revision.
>
> I've appended the relevant section of gdb.log in case that is helpful.
>
> Tom
>
> (gdb)
> 220-exec-run
> =thread-group-started,id="i1",pid="21201"
> =thread-created,id="1",group-id="i1"
> 220^running
> *running,thread-id="all"
> (gdb)
> mi_expect_stop: expecting: \*stopped,reason="breakpoint-hit",disp="del",bkptno="
> [0-9]+",frame={addr="0x[0-9A-Fa-f]+",func="main",args=\[\],(?:file="[^
> ]*.*mi-start.c",fullname="(/[^\n]*/|\\\\[^\\]+\\[^\n]+\\|\\[^\\][^\n]*\\|[a-zA-Z
> ]:[^\n]*\\).*mi-start.c",line="[0-9]+"|from=".*mi-start.c")},thread-id="[0-9]+",
> stopped-threads=[^
> ]*
> (=thread-selected,id="[0-9]+"
> |=(?:breakpoint-created|breakpoint-deleted)[^
> ]+"
> )*[(]gdb[)]
> $
> =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x8
> 6-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-gro
> up="i1"
> =library-loaded,id="/lib64/libm.so.6",target-name="/lib64/libm.so.6",host-name="
> /lib64/libm.so.6",symbols-loaded="0",thread-group="i1"
> =library-loaded,id="/lib64/libc.so.6",target-name="/lib64/libc.so.6",host-name="
> /lib64/libc.so.6",symbols-loaded="0",thread-group="i1"
> =thread-exited,id="1",group-id="i1"
> =thread-group-exited,id="i1",exit-code="0"
> *stopped,reason="exited-normally"
> (gdb)
> got =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linu
> x-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread
> -group="i1"
> =library-loaded,id="/lib64/libm.so.6",target-name="/lib64/libm.so.6",host-name="
> /lib64/libm.so.6",symbols-loaded="0",thread-group="i1"
> =library-loaded,id="/lib64/libc.so.6",target-name="/lib64/libc.so.6",host-name="
> /lib64/libc.so.6",symbols-loaded="0",thread-group="i1"
> =thread-exited,id="1",group-id="i1"
> =thread-group-exited,id="i1",exit-code="0"
> *stopped,reason="exited-normally"
> (gdb)
>
> FAIL: gdb.mi/mi-start.exp: run to main (unknown output after running)
Simon Marchi May 20, 2014, 10:24 p.m. UTC | #5 
On Tue 20 May 2014 12:40:01 PM EDT, Simon Marchi wrote:
> On Mon 19 May 2014 03:39:09 PM EDT, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
>>
>> Simon> 2014-05-13  Simon Marchi  <simon.marchi@ericsson.com>
>> Simon> 	* lib/mi-support.exp (mi_run_cmd_full): Set arguments by calling
>> Simon> 	"-exec-arguments" or "set args" before running the inferior.
>>
>> This patch causes a test failure here:
>
> Ehh, that's what I get for not re-running the testsuite, even after a 
> year. I will send something soon about that.

mi-start.exp passes "--start" to mi_run_cmd, which is intended to be an argument to -exec-run. My understanding was that the parameter "args" of mi_run_cmd_full was for arguments to pass to the test program (hence, my patch). A quick grep shows that mi-start.exp is the only place where arguments are actually passed to mi_run_cmd. At the time the patch was written, nowhere in the test suite were parameters passed to mi_run_cmd, so I just assumed that this was the intended behavior. I ended up not even using this fix, since Pedro suggested that to modify the behavior of a test program, I should stop at a breakpoint and modify the value of a global variable instead.

Perhaps the original behavior was not bad after all: $args is passed to the command used to run the program, "run" or "-exec-run", depending on the value of $use_mi_command. Therefore, the meaning/semantic of the $args parameter changes based on the value of $use_mi_command: arguments passed to the test program if false, arguments to -exec-run (such as --start) if true. If you use MI and want to pass arguments to the test program, then you need to manually call -exec-arguments before (or a helper proc could be created for that). If documented properly, I think this way is good.

If we decide to stay with the current behavior where $args always means the arguments to pass to the test program, then we'll need to add a separate parameter to mi_run_cmd_full, mi_run_cmd and mi_run_with_cli to convey the arguments to pass to the run command (to be able to pass the --start). This makes things a little bit messy:

    proc mi_run_cmd {args}

will become

    proc mi_run_cmd {test_program_args run_command_args}

which means that most calls to mi_run_cmd will have to be changed from

    mi_run_cmd

to

    mi_run_cmd "" ""

because of the special vararg-like meaning of the parameter named "args".

In my opinion, I think the old behavior was fine, it just needs to be documented better (clarify what $args means in the mi_run_* procedures).

I would like to hear what you think about this.

Thanks,

Simon
Pedro Alves May 21, 2014, 8:46 a.m. UTC | #6 
On 05/20/2014 11:24 PM, Simon Marchi wrote:

> In my opinion, I think the old behavior was fine, it just needs to be documented better (clarify what $args means in the mi_run_* procedures).

I agree, because it's also the simplest, and as you say,

> I ended up not even using this fix, since Pedro suggested that to modify
> the behavior of a test program, I should stop at a breakpoint and modify
> the value of a global variable instead.

nothing is actually making use of the original fix.
If we end up needing to pass program arguments to mi_run_cmd
in MI mode, we can revisit.

Thanks,
diff mbox

Patch

diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index ad58775..b27b268 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -818,9 +818,11 @@  proc mi_run_cmd_full {use_mi_command args} {
     if {$use_mi_command} {
 	set run_prefix "220-exec-"
 	set run_match "220"
+	set set_args_cmd "-exec-arguments"
     } else {
 	set run_prefix ""
 	set run_match ""
+	set set_args_cmd "set args"
     }
 
     if [target_info exists gdb_init_command] {
@@ -862,7 +864,16 @@  proc mi_run_cmd_full {use_mi_command args} {
 	return 0
     }
 
-    send_gdb "${run_prefix}run $args\n"
+    send_gdb "${set_args_cmd} $args\n"
+    gdb_expect {
+	-re "$mi_gdb_prompt$" { }
+	default {
+	    perror "settings args failed"
+	    return -1
+	}
+    }
+
+    send_gdb "${run_prefix}run\n"
     gdb_expect {
 	-re "${run_match}\\^running\r\n(\\*running,thread-id=\"\[^\"\]+\"\r\n|=thread-created,id=\"1\",group-id=\"\[0-9\]+\"\r\n)*(${library_loaded_re})*(${thread_selected_re})?${mi_gdb_prompt}" {
 	}