[rfc] Call init_wait_for_inferior before get_offsets

[rfc] Call init_wait_for_inferior before get_offsets

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

The flag `inserted' in bp loc is not managed correctly in the case that
gdb gets reply from gdbserver on packet "qOffset".

In order to illustrate this problem clearly, here is a simple callgraph,

remote_start_remote
  |
  +-> get_offsets
  |     |
  |     `-> objfile_relocate // if gdbserver replies offsets
  |           |
  |           `-> breakpoint_re_set//[1] insert bp and loc->inserted = 1
  |
  `-> start_remote
        |
        +-> init_wait_for_inferior
        |     |
        |     `-> breakpoint_init_inferior //[2] loc->inserted = 0
        |
        `-> post_create_inferior
              |
              `-> breakpoint_re_set//[3] insert bp again, and
loc-inserted = 1

Supposing we have inserted an breakpoint B on address A1 before any
target is created.  Then, we create a target remote.  When gdbserver
replies qOffset packet with an offset map (usually, in uclinux,
gdbserver will do this), objfile_relocate is called, and
breakpoint_re_set [1] is called, so breakpoint B is inserted on address
A2, which is the relocated address of A1.  Then, in [2],
breakpoint_init_inferior is called, so the bp loc corresponding to B is
reset, and flag `inserted' is set to zero.  Finally, when goes to
post_create_inferior, breakpoint_re_set is called, since breakpoint B's
loc's flag `inserted' has been zero (cleaned in [2]), gdb will insert
breakpoint again at the same address A2, however breakpoint instruction
has on address A2.  gdb will read breakpoint instruction into shadow,
and store the same breakpoint instruction on address A2.  The original
instruction is lost!.

The cause of this problem is breakpoint_init_inferioris called later
than breakpoint_re_set.  This patch is to move init_wait_for_inferior
forward, so that it can be called before breakpoint_re_set.

This problem is revealed by following fail in my uclinux port, and this
patch fixes this fail.

  FAIL: gdb.base/break-always.exp: continue to breakpoint: bar

This patch is regression tested on x86_64-known-linux with board file
native-gdbserver.exp as well.  Although there are some mi tests PASSes
and FAILs(timeout), they are caused by instable tests, instead of my patch.

I am still not very familiar with infrun.c and remote.c, so comments are
welcome.

-- 
Yao (齐尧)

[-- Attachment #2: 0001-move-init_wait_for_inferior-forward.patch --]
[-- Type: text/x-patch, Size: 1023 bytes --]

2011-06-20  Yao Qi  <yao@codesourcery.com>

	* infrun.c (start_remote): Move call init_wait_for_inferior to ...
	* remote.c (remote_start_remote): ... here.

---
 gdb/infrun.c |    1 -
 gdb/remote.c |    2 ++
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 67a74cc..3d39080 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2128,7 +2128,6 @@ start_remote (int from_tty)
 {
   struct inferior *inferior;
 
-  init_wait_for_inferior ();
   inferior = current_inferior ();
   inferior->stop_soon = STOP_QUIETLY_REMOTE;
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 50e671d..559039d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3422,6 +3422,8 @@ remote_start_remote (struct ui_out *uiout, void *opaque)
       /* Always add the main thread.  */
       add_thread_silent (inferior_ptid);
 
+      init_wait_for_inferior ();
+
       get_offsets ();		/* Get text, data & bss offsets.  */
 
       /* If we could not find a description using qXfer, and we know
-- 
1.7.0.4

Re: [rfc] Call init_wait_for_inferior before get_offsets

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2011-06-20  Yao Qi  <yao@codesourcery.com>
Yao> 	* infrun.c (start_remote): Move call init_wait_for_inferior to ...
Yao> 	* remote.c (remote_start_remote): ... here.

I don't know anything about this code, but I did notice that
start_remote is also called from monitor.c.  So it seems like this
either needs some justification or another change.

Tom

Re: [rfc] Call init_wait_for_inferior before get_offsets

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

On 06/22/2011 11:07 PM, Tom Tromey wrote:
>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
> 
> Yao> 2011-06-20  Yao Qi  <yao@codesourcery.com>
> Yao> 	* infrun.c (start_remote): Move call init_wait_for_inferior to ...
> Yao> 	* remote.c (remote_start_remote): ... here.
> 
> I don't know anything about this code, but I did notice that
> start_remote is also called from monitor.c.  So it seems like this
> either needs some justification or another change.

monitor.c should be changed as well.  Sorry for missing this chunk of
change in patch.  Here is a new one.

Regression tested on x86_64-unknown-linux with board file
native-gdbserver.exp.  Some new FAILs/PASSes appears in
gdb.mi/mi-nsmoribund.exp, but it is not caused this patch, IMO.

-- 
Yao (齐尧)

[-- Attachment #2: 0003-move-init_wait_for_inferior-forward.patch --]
[-- Type: text/x-patch, Size: 1369 bytes --]

	gdb/
	* infrun.c (start_remote): Move call init_wait_for_inferior to ...
	* remote.c (remote_start_remote): ... here.
	* monitor.c (monitor_open): ... here.
---
 gdb/infrun.c  |    1 -
 gdb/monitor.c |    2 ++
 gdb/remote.c  |    2 ++
 3 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 67a74cc..3d39080 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2128,7 +2128,6 @@ start_remote (int from_tty)
 {
   struct inferior *inferior;
 
-  init_wait_for_inferior ();
   inferior = current_inferior ();
   inferior->stop_soon = STOP_QUIETLY_REMOTE;
 
diff --git a/gdb/monitor.c b/gdb/monitor.c
index 95e6a63..4775011 100644
--- a/gdb/monitor.c
+++ b/gdb/monitor.c
@@ -838,6 +838,8 @@ monitor_open (char *args, struct monitor_ops *mon_ops, int from_tty)
 
   monitor_printf (current_monitor->line_term);
 
+  init_wait_for_inferior ();
+
   start_remote (from_tty);
 }
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 50e671d..559039d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3422,6 +3422,8 @@ remote_start_remote (struct ui_out *uiout, void *opaque)
       /* Always add the main thread.  */
       add_thread_silent (inferior_ptid);
 
+      init_wait_for_inferior ();
+
       get_offsets ();		/* Get text, data & bss offsets.  */
 
       /* If we could not find a description using qXfer, and we know
-- 
1.7.0.4

Re: [rfc] Call init_wait_for_inferior before get_offsets

[Pedro Alves]

On Thursday 23 June 2011 11:34:41, Yao Qi wrote:

> > Yao> 	* remote.c (remote_start_remote): ... here.

There's another call to get_offsets in the else branch.

-- 
Pedro Alves

Re: [rfc] Call init_wait_for_inferior before get_offsets

[Yao Qi]

On 06/23/2011 06:38 PM, Pedro Alves wrote:
> On Thursday 23 June 2011 11:34:41, Yao Qi wrote:
> 
>>> Yao> 	* remote.c (remote_start_remote): ... here.
> 
> There's another call to get_offsets in the else branch.
> 

Yes, but there has been a call to init_wait_for_inferior () "guarded" at
the beginning of that branch.

  else
    {
      /* Clear WFI global state.  Do this before finding about new
	 threads and inferiors, and setting the current inferior.
	 Otherwise we would clear the proceed status of the current
	 inferior when we want its stop_soon state to be preserved
	 (see notice_new_inferior).  */
      init_wait_for_inferior ();
...
...
      get_offsets ();		/* Get text, data & bss offsets.  */

-- 
Yao (齐尧)

Re: [rfc] Call init_wait_for_inferior before get_offsets

[Pedro Alves]

On Thursday 23 June 2011 14:19:59, Yao Qi wrote:
> On 06/23/2011 06:38 PM, Pedro Alves wrote:
> > On Thursday 23 June 2011 11:34:41, Yao Qi wrote:
> > 
> >>> Yao> 	* remote.c (remote_start_remote): ... here.
> > 
> > There's another call to get_offsets in the else branch.
> > 
> 
> Yes, but there has been a call to init_wait_for_inferior () "guarded" at
> the beginning of that branch.

Bah, and I was the one that put it there even... :-)

> 
>   else
>     {
>       /* Clear WFI global state.  Do this before finding about new
> 	 threads and inferiors, and setting the current inferior.
> 	 Otherwise we would clear the proceed status of the current
> 	 inferior when we want its stop_soon state to be preserved
> 	 (see notice_new_inferior).  */
>       init_wait_for_inferior ();
> ...
> ...
>       get_offsets ();		/* Get text, data & bss offsets.  */

Your patch is okay ...

> +      init_wait_for_inferior ();
> +
>        get_offsets ();          /* Get text, data & bss offsets.  */

... with a small comment above the init_wait_for_inferior call
mentioning the reason it is called before get_offsets.

-- 
Pedro Alves

[committed] [rfc] Call init_wait_for_inferior before get_offsets

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On 06/23/2011 09:46 PM, Pedro Alves wrote:
> Your patch is okay ...
> 
>> > +      init_wait_for_inferior ();
>> > +
>> >        get_offsets ();          /* Get text, data & bss offsets.  */
> ... with a small comment above the init_wait_for_inferior call
> mentioning the reason it is called before get_offsets.
> 

Thanks for the review.  Comment is added new patch.  Committed.

http://sourceware.org/ml/gdb-cvs/2011-06/msg00140.html

-- 
Yao (齐尧)

[-- Attachment #2: 0003-move-init_wait_for_inferior-forward.patch --]
[-- Type: text/x-patch, Size: 1954 bytes --]

	gdb/
	* infrun.c (start_remote): Move call init_wait_for_inferior to ...
	* remote.c (remote_start_remote): ... here.
	* monitor.c (monitor_open): ... here.
---
 gdb/infrun.c  |    1 -
 gdb/monitor.c |    2 ++
 gdb/remote.c  |   11 +++++++++++
 3 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 67a74cc..3d39080 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2128,7 +2128,6 @@ start_remote (int from_tty)
 {
   struct inferior *inferior;
 
-  init_wait_for_inferior ();
   inferior = current_inferior ();
   inferior->stop_soon = STOP_QUIETLY_REMOTE;
 
diff --git a/gdb/monitor.c b/gdb/monitor.c
index 95e6a63..4775011 100644
--- a/gdb/monitor.c
+++ b/gdb/monitor.c
@@ -838,6 +838,8 @@ monitor_open (char *args, struct monitor_ops *mon_ops, int from_tty)
 
   monitor_printf (current_monitor->line_term);
 
+  init_wait_for_inferior ();
+
   start_remote (from_tty);
 }
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 50e671d..f329f20 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3422,6 +3422,17 @@ remote_start_remote (struct ui_out *uiout, void *opaque)
       /* Always add the main thread.  */
       add_thread_silent (inferior_ptid);
 
+      /* init_wait_for_inferior should be called before get_offsets in order
+	 to manage `inserted' flag in bp loc in a correct state.
+	 breakpoint_init_inferior, called from init_wait_for_inferior, set
+	 `inserted' flag to 0, while before breakpoint_re_set, called from
+	 start_remote, set `inserted' flag to 1.  In the initialization of
+	 inferior, breakpoint_init_inferior should be called first, and then
+	 breakpoint_re_set can be called.  If this order is broken, state of
+	 `inserted' flag is wrong, and cause some problems on breakpoint
+	 manipulation.  */
+      init_wait_for_inferior ();
+
       get_offsets ();		/* Get text, data & bss offsets.  */
 
       /* If we could not find a description using qXfer, and we know
-- 
1.7.0.4