[obv] s390*: Fix build regression, remains execution regression

[obv] s390*: Fix build regression, remains execution regression

[Jan Kratochvil]

Hi Pedro,

this is the obvious part, checked in.

Unfortunately there is still a regression, it crashes during exec from the
wrapper->real inferior on mostly any program (kernel-2.6.32-220.el6.s390x):

61562 pts/1 T  \_ /root/jkratoch/redhat/archer-git/gdb/gdb -nx -x ./transcript.1
61568 pts/1 T      \_ /bin/bash -c exec /root/jkratoch/redhat/archer-git/gdb/testsuite/gdb.base/watchpoint-cond-gone

(gdb) run
Starting program: /root/jkratoch/redhat/archer-git/gdb/testsuite/gdb.base/watchpoint-cond-gone 
Couldn't retrieve watchpoint status: No such process.
Couldn't get registers: No such process.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2011-12/msg00175.html

--- src/gdb/ChangeLog	2011/12/17 06:14:44	1.13641
+++ src/gdb/ChangeLog	2011/12/17 09:43:53	1.13642
@@ -1,3 +1,9 @@
+2011-12-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Fix build regression from the PR threads/10729 fix.
+	* s390-nat.c (s390_insert_watchpoint, s390_remove_watchpoint): Use LP,
+	not LP->PTID.
+
 2011-12-16  Andrey Smirnov  <andrew.smirnov@gmail.com>
 
 	* mi/mi-main.c (mi_cmd_list_thread_groups): Rename `optind' and
--- src/gdb/s390-nat.c	2011/12/14 17:20:31	1.36
+++ src/gdb/s390-nat.c	2011/12/17 09:43:53	1.37
@@ -532,7 +532,7 @@
   watch_base = area;
 
   ALL_LWPS (lp)
-    s390_fix_watch_points (lp->ptid);
+    s390_fix_watch_points (lp);
   return 0;
 }
 
@@ -560,7 +560,7 @@
   xfree (area);
 
   ALL_LWPS (lp)
-    s390_fix_watch_points (lp->ptid);
+    s390_fix_watch_points (lp);
   return 0;
 }
 

Re: [obv] s390*: Fix build regression, remains execution regression

[Pedro Alves]

On Saturday 17 December 2011 09:47:53, Jan Kratochvil wrote:
> Hi Pedro,
> 
> this is the obvious part, checked in.
> 
> Unfortunately there is still a regression, it crashes during exec from the
> wrapper->real inferior on mostly any program (kernel-2.6.32-220.el6.s390x):
> 
> 61562 pts/1 T  \_ /root/jkratoch/redhat/archer-git/gdb/gdb -nx -x ./transcript.1
> 61568 pts/1 T      \_ /bin/bash -c exec /root/jkratoch/redhat/archer-git/gdb/testsuite/gdb.base/watchpoint-cond-gone
> 
> (gdb) run
> Starting program: /root/jkratoch/redhat/archer-git/gdb/testsuite/gdb.base/watchpoint-cond-gone 
> Couldn't retrieve watchpoint status: No such process.
> Couldn't get registers: No such process.

I suspect it's exposed by this change:

2011-12-14  Pedro Alves  <pedro@codesourcery.com>

        PR threads/10729

        * linux-nat.c:
...
        (add_lwp): Call linux_nat_new_thread even on the first LWP.
...

That code used to look like:

  if (num_lwps (GET_PID (ptid)) > 1 && linux_nat_new_thread != NULL)
    linux_nat_new_thread (ptid);

s390_fix_watch_points is the new_thread callback:

  linux_nat_set_new_thread (t, s390_fix_watch_points);


> static void
> s390_fix_watch_points (struct lwp_info *lp)
> {
>   int tid;
> 
>   per_struct per_info;
>   ptrace_area parea;
> 
>   CORE_ADDR watch_lo_addr = (CORE_ADDR)-1, watch_hi_addr = 0;
>   struct watch_area *area;
> 
>   tid = TIDGET (lp->ptid);
>   if (tid == 0)
>     tid = PIDGET (lp->ptid);
> 
>   for (area = watch_base; area; area = area->next)
>     {
>       watch_lo_addr = min (watch_lo_addr, area->lo_addr);
>       watch_hi_addr = max (watch_hi_addr, area->hi_addr);
>     }
> 
>   parea.len = sizeof (per_info);
>   parea.process_addr = (addr_t) & per_info;
>   parea.kernel_addr = offsetof (struct user_regs_struct, per_info);
>   if (ptrace (PTRACE_PEEKUSR_AREA, tid, &parea) < 0)
>     perror_with_name (_("Couldn't retrieve watchpoint status"));
> 
>   if (watch_base)
>     {
>       per_info.control_regs.bits.em_storage_alteration = 1;
>       per_info.control_regs.bits.storage_alt_space_ctl = 1;
>     }
>   else
>     {
>       per_info.control_regs.bits.em_storage_alteration = 0;
>       per_info.control_regs.bits.storage_alt_space_ctl = 0;
>     }
>   per_info.starting_addr = watch_lo_addr;
>   per_info.ending_addr = watch_hi_addr;
> 
>   if (ptrace (PTRACE_POKEUSR_AREA, tid, &parea) < 0)
>     perror_with_name (_("Couldn't modify watchpoint status"));
> }

Looks like we're trying to peek/poke at the shell process,
and failing?

I suspect that something like this pseudo patch:

+s390_new_thread (struct lwp_info *lp)
+{
+	if (num_lwps (GET_PID (ptid)) > 1)
+     s390_fix_watch_points (lp);
+}

-  linux_nat_set_new_thread (t, s390_fix_watch_points);
+  linux_nat_set_new_thread (t, s390_new_thread);

Would fix it.

-- 
Pedro Alves

[patch] s390*: watchpoints regression [Re: [obv] s390*: Fix build regression]

[Jan Kratochvil]

On Sat, 17 Dec 2011 13:29:27 +0100, Pedro Alves wrote:
> Looks like we're trying to peek/poke at the shell process,
> and failing?

Yes.


> I suspect that something like this pseudo patch:
> 
> +s390_new_thread (struct lwp_info *lp)
> +{
> +	if (num_lwps (GET_PID (ptid)) > 1)
> +     s390_fix_watch_points (lp);
> +}

I can confirm it works.

On x86* it now sets DR_CONTROL to 0 already for the wrapper shell.  When there
exist watchpoints they are suppressed so it still sets 0 and it works.  But
I do not think GDB should mess with watchpoint registers already for the
wrapper shell.

In the hypothetical case the wrapper shell sets DR registers they get
inherited now by the inferior.  Not that it needs to be handled but it IMO
suggests DR_CONTROL should be rather set for the real inferior.

But I do not see some easy enough way how to delay the DR setting till the new
inferior so I just verified your code which works.

OK to check it in this way?


Thanks,
Jan


2011-12-17  Pedro Alves  <pedro@codesourcery.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* s390-nat.c (s390_new_thread_lwp, s390_new_thread): New functions.
	(_initialize_s390_nat): Install now s390_new_thread, not
	s390_fix_watch_points.

--- a/gdb/s390-nat.c
+++ b/gdb/s390-nat.c
@@ -667,6 +667,38 @@ s390_read_description (struct target_ops *ops)
 	  tdesc_s390_linux32);
 }
 
+/* Helper for s390_new_thread.  */
+
+static int
+s390_new_thread_lwp (struct lwp_info *lp, void *new_lp_voidp)
+{
+  struct lwp_info *new_lp = new_lp_voidp;
+
+  if (lp == new_lp)
+    {
+      /* Continue traversal.  */
+      return 0;
+    }
+
+  s390_fix_watch_points (new_lp);
+
+  /* Stop traversal.  */
+  return 1;
+}
+
+/* Execute s390_fix_watch_points only when there is at least one other LWP
+   besides the new one.  This excludes the initial shell wrapper process which
+   works around a problem of s390x failing to PTRACE_POKEUSR_AREA when the
+   initial shell wrapper runs.  s390_fix_watch_points is fortunately not needed
+   for the initial creation of a single threaded process - watchpoints are not
+   yet inserted into inferior in such case.  */
+
+static void
+s390_new_thread (struct lwp_info *lp)
+{
+  iterate_over_lwps (pid_to_ptid (GET_PID (lp->ptid)), s390_new_thread_lwp, lp);
+}
+
 void _initialize_s390_nat (void);
 
 void
@@ -695,5 +727,5 @@ _initialize_s390_nat (void)
 
   /* Register the target.  */
   linux_nat_add_target (t);
-  linux_nat_set_new_thread (t, s390_fix_watch_points);
+  linux_nat_set_new_thread (t, s390_new_thread);
 }

Re: [patch] s390*: watchpoints regression [Re: [obv] s390*: Fix build regression]

[Pedro Alves]

On Saturday 17 December 2011 19:15:43, Jan Kratochvil wrote:

> I can confirm it works.
> 
> On x86* it now sets DR_CONTROL to 0 already for the wrapper shell.  When there
> exist watchpoints they are suppressed so it still sets 0 and it works.  But
> I do not think GDB should mess with watchpoint registers already for the
> wrapper shell.

In the loop that runs through the shell, fork-child.c:startup_inferior,
nothing inserts breakpoints/watchpoints.  So nothing ends up
setting lwp->arch_private->debug_registers_changed, and so we should
not be setting DR_CONTROL to 0 for the wrapper shell.  Do you
actually see it happen?

> In the hypothetical case the wrapper shell sets DR registers they get
> inherited now by the inferior.  Not that it needs to be handled but it IMO
> suggests DR_CONTROL should be rather set for the real inferior.
> 
> But I do not see some easy enough way how to delay the DR setting till the new
> inferior so I just verified your code which works.
> 
> OK to check it in this way?

Looks okay to me.

-- 
Pedro Alves

Re: [patch] s390*: watchpoints regression [Re: [obv] s390*: Fix build regression]

[Jan Kratochvil]

On Sat, 17 Dec 2011 20:40:13 +0100, Pedro Alves wrote:
> In the loop that runs through the shell, fork-child.c:startup_inferior,
> nothing inserts breakpoints/watchpoints.  So nothing ends up
> setting lwp->arch_private->debug_registers_changed, and so we should
> not be setting DR_CONTROL to 0 for the wrapper shell.  Do you
> actually see it happen?

I see it happenning.  [attached]

./gdb -nx ./gdb -ex 'watch gdb_stdin' -ex r 
Reading symbols from /home/jkratoch/redhat/gdb-clean/gdb/gdb...done.
Hardware watchpoint 1: gdb_stdin
Starting program: /home/jkratoch/redhat/gdb-clean/gdb/gdb 
DR_CONTROL set 0x0
lrwxrwxrwx 1 jkratoch jkratoch 0 Dec 17 19:41 /proc/12542/exe -> /bin/bash
DR_CONTROL set 0x90101
lrwxrwxrwx 1 jkratoch jkratoch 0 Dec 17 19:41 /proc/12542/exe -> /home/jkratoch/redhat/gdb-clean/gdb/gdb
DR_CONTROL set 0x90100
...


Thanks,
Jan

Re: [patch] s390*: watchpoints regression [repost]

[Jan Kratochvil]

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

On Sat, 17 Dec 2011 20:40:13 +0100, Pedro Alves wrote:
> In the loop that runs through the shell, fork-child.c:startup_inferior,
> nothing inserts breakpoints/watchpoints.  So nothing ends up
> setting lwp->arch_private->debug_registers_changed, and so we should
> not be setting DR_CONTROL to 0 for the wrapper shell.  Do you
> actually see it happen?

I see it happenning.  [attached]

./gdb -nx ./gdb -ex 'watch gdb_stdin' -ex r 
Reading symbols from /home/jkratoch/redhat/gdb-clean/gdb/gdb...done.
Hardware watchpoint 1: gdb_stdin
Starting program: /home/jkratoch/redhat/gdb-clean/gdb/gdb 
DR_CONTROL set 0x0
lrwxrwxrwx 1 jkratoch jkratoch 0 Dec 17 19:41 /proc/12542/exe -> /bin/bash
DR_CONTROL set 0x90101
lrwxrwxrwx 1 jkratoch jkratoch 0 Dec 17 19:41 /proc/12542/exe -> /home/jkratoch/redhat/gdb-clean/gdb/gdb
DR_CONTROL set 0x90100
...


Thanks,
Jan

[-- Attachment #2: wrapper-debug.patch --]
[-- Type: text/plain, Size: 452 bytes --]

--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -403,6 +403,11 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
 	    clear_status = 1;
 	  }
 
+printf("DR_CONTROL set 0x%lx\n",(long)state->dr_control_mirror);
+{char buf[100];
+sprintf(buf,"ls -l /proc/%d/exe", (int)lwp->ptid.pid);
+system(buf);
+}
       amd64_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
 
       lwp->arch_private->debug_registers_changed = 0;

Re: [patch] s390*: watchpoints regression [repost]

[Pedro Alves]

On Saturday 17 December 2011 19:44:54, Jan Kratochvil wrote:
> On Sat, 17 Dec 2011 20:40:13 +0100, Pedro Alves wrote:
> > In the loop that runs through the shell, fork-child.c:startup_inferior,
> > nothing inserts breakpoints/watchpoints.  So nothing ends up
> > setting lwp->arch_private->debug_registers_changed, and so we should
> > not be setting DR_CONTROL to 0 for the wrapper shell.  Do you
> > actually see it happen?
> 
> I see it happenning.  [attached]

Ah, I forgot this:

static void
i386_linux_new_thread (struct lwp_info *lp)
{
  struct arch_lwp_info *info = XCNEW (struct arch_lwp_info);

  info->debug_registers_changed = 1;

  lp->arch_private = info;
}

Hmm.  I'm starting to think that the easiest is jut to
go back at not calling new_thread for the first thread.
Let me give that a try.

-- 
Pedro Alves

Re: [patch] s390*: watchpoints regression [repost]

[Pedro Alves]

On Saturday 17 December 2011 19:56:32, Pedro Alves wrote:
> On Saturday 17 December 2011 19:44:54, Jan Kratochvil wrote:
> > On Sat, 17 Dec 2011 20:40:13 +0100, Pedro Alves wrote:
> > > In the loop that runs through the shell, fork-child.c:startup_inferior,
> > > nothing inserts breakpoints/watchpoints.  So nothing ends up
> > > setting lwp->arch_private->debug_registers_changed, and so we should
> > > not be setting DR_CONTROL to 0 for the wrapper shell.  Do you
> > > actually see it happen?
> > 
> > I see it happenning.  [attached]
> 
> Ah, I forgot this:
> 
> static void
> i386_linux_new_thread (struct lwp_info *lp)
> {
>   struct arch_lwp_info *info = XCNEW (struct arch_lwp_info);
> 
>   info->debug_registers_changed = 1;
> 
>   lp->arch_private = info;
> }
> 
> Hmm.  I'm starting to think that the easiest is jut to
> go back at not calling new_thread for the first thread.
> Let me give that a try.

Seems to work fine.  This should obsolete the s390 patch.

I think GDBserver has a similar problem with --wrapper.

-- 
Pedro Alves

2011-12-17  Pedro Alves  <pedro@codesourcery.com>
            Jan Kratochvil  <jan.kratochvil@redhat.com>

        * linux-nat.c (add_lwp): Don't call linux_nat_new_thread on the
	first LWP.
	* amd64-linux-nat.c (update_debug_registers_callback): Instantiate
	`lwp->arch_private' if NULL.
	(amd64_linux_prepare_to_resume): Do nothing if `lwp->arch_private'
	is NULL.
	* i386-linux-nat.c (update_debug_registers_callback): Instantiate
	`lwp->arch_private' if NULL.
	(i386_linux_prepare_to_resume): Do nothing if `lwp->arch_private'
	is NULL.
---

 gdb/amd64-linux-nat.c |    8 ++++++++
 gdb/i386-linux-nat.c  |    8 ++++++++
 gdb/linux-nat.c       |    2 +-
 3 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
index 288160b..6ee9596 100644
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -343,6 +343,9 @@ amd64_linux_dr_get_status (void)
 static int
 update_debug_registers_callback (struct lwp_info *lwp, void *arg)
 {
+  if (lwp->arch_private == NULL)
+    lwp->arch_private = XCNEW (struct arch_lwp_info);
+
   /* The actual update is done later just before resuming the lwp, we
      just mark that the registers need updating.  */
   lwp->arch_private->debug_registers_changed = 1;
@@ -386,6 +389,11 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
 {
   int clear_status = 0;
 
+  /* This is the main thread still going through the shell, or, no
+     watchpoint has been set yet.  */
+  if (lwp->arch_private == NULL)
+    return;
+
   if (lwp->arch_private->debug_registers_changed)
     {
       struct i386_debug_reg_state *state = i386_debug_reg_state ();
diff --git a/gdb/i386-linux-nat.c b/gdb/i386-linux-nat.c
index 190979b..3c56a22 100644
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -715,6 +715,9 @@ i386_linux_dr_get_status (void)
 static int
 update_debug_registers_callback (struct lwp_info *lwp, void *arg)
 {
+  if (lwp->arch_private == NULL)
+    lwp->arch_private = XCNEW (struct arch_lwp_info);
+
   /* The actual update is done later just before resuming the lwp, we
      just mark that the registers need updating.  */
   lwp->arch_private->debug_registers_changed = 1;
@@ -758,6 +761,11 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
 {
   int clear_status = 0;
 
+  /* This is the main thread still going through the shell, or, no
+     watchpoint has been set yet.  */
+  if (lwp->arch_private == NULL)
+    return;
+
   if (lwp->arch_private->debug_registers_changed)
     {
       struct i386_debug_reg_state *state = i386_debug_reg_state ();
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 1cbfc44..2b1ecb7 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1151,7 +1151,7 @@ add_lwp (ptid_t ptid)
   lp->next = lwp_list;
   lwp_list = lp;
 
-  if (linux_nat_new_thread != NULL)
+  if (num_lwps (GET_PID (ptid)) > 1 && linux_nat_new_thread != NULL)
     linux_nat_new_thread (lp);
 
   return lp;

Re: [patch] s390*: watchpoints regression [repost]

[Jan Kratochvil]

On Sat, 17 Dec 2011 21:12:57 +0100, Pedro Alves wrote:
> On Saturday 17 December 2011 19:56:32, Pedro Alves wrote:
> > Hmm.  I'm starting to think that the easiest is jut to
> > go back at not calling new_thread for the first thread.
> > Let me give that a try.
> 
> Seems to work fine.  This should obsolete the s390 patch.

I can confirm it fixes both the x86* DR set for wrapper and the s390
regression.


> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1151,7 +1151,7 @@ add_lwp (ptid_t ptid)
>    lp->next = lwp_list;
>    lwp_list = lp;
>  
> -  if (linux_nat_new_thread != NULL)
> +  if (num_lwps (GET_PID (ptid)) > 1 && linux_nat_new_thread != NULL)
>      linux_nat_new_thread (lp);
>  
>    return lp;

After this regression churn and all the discussion around I believe this line
is worth some comment, like I wrote in the previous s390* patch.  That
num_lwps there is very magic.


Thanks,
Jan

Re: [patch] s390*: watchpoints regression [repost]

[Joel Brobecker]

> +  /* This is the main thread still going through the shell, or, no
> +     watchpoint has been set yet.  */
> +  if (lwp->arch_private == NULL)
> +    return;

Just a really minor nitpick: Would you consider putting the comment
inside the if, instead of just before? I think it'd be slightly clearer
that way.

-- 
Joel

Re: [patch] s390*: watchpoints regression [repost]

[Jan Kratochvil]

On Sun, 18 Dec 2011 07:21:27 +0100, Joel Brobecker wrote:
> > +  /* This is the main thread still going through the shell, or, no
> > +     watchpoint has been set yet.  */
> > +  if (lwp->arch_private == NULL)
> > +    return;
> 
> Just a really minor nitpick: Would you consider putting the comment
> inside the if, instead of just before? I think it'd be slightly clearer
> that way.

FYI I am aware of this rule and I try to follow it but it seems unnatural to
me.  It then requires new { brackets } and when reading the code I would more
expect to see why the conditional happens, not why the return happens.

But no problem continuing to follow this rule.


Thanks,
Jan

Re: [patch] s390*: watchpoints regression [repost]

[Joel Brobecker]

> FYI I am aware of this rule and I try to follow it but it seems
> unnatural to me.  It then requires new { brackets } and when reading
> the code I would more expect to see why the conditional happens, not
> why the return happens.

Interesting, the opposite feels completely unnatural to me: The comment
only applies if the condition is true, whereas in the case I was quoting,
it looks like it applies no matter what.

Regarding the extra curly braces, I think it's OK to leave them out,
like so:

    if ([...])
      /* This is a comment that ...  */
      return;

If we prefer keeping the comment before the condition, then I would
suggest we rewrite the comment to say so.  Something like this:

    /* If this is an old thread, or there is something else
       special to it, then it's ok to do nothing.  */
    if ([...])
      return;

-- 
Joel

Code formatting [Re: [patch] s390*: watchpoints regression [repost]]

[Jan Kratochvil]

On Sun, 18 Dec 2011 11:46:34 +0100, Joel Brobecker wrote:
> Regarding the extra curly braces, I think it's OK to leave them out,
> like so:
> 

On Sun, 18 Dec 2011 12:37:59 +0100, Eli Zaretskii wrote:
> > It then requires new { brackets }
> 
> I don't think you need braces.  The compiler certainly doesn't.

While this is bikeshedding in its purest form I can jump in.


>     if ([...])
>       /* This is a comment that ...  */
>       return;

This is a bug from the first sight as there are two C statements attached to
an `if' conditional.  Two statements always need a block.  This is a bug.

I really do not have time to interrupt myself each time, several times
a minute, looking at the code starting examining what those two statements
semantically are, and therefore if they really require a block or not.


Thanks,
Jan

Re: Code formatting [Re: [patch] s390*: watchpoints regression [repost]]

[Mark Kettenis]

> Date: Sun, 18 Dec 2011 12:59:31 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Sun, 18 Dec 2011 11:46:34 +0100, Joel Brobecker wrote:
> > Regarding the extra curly braces, I think it's OK to leave them out,
> > like so:
> > 
> 
> On Sun, 18 Dec 2011 12:37:59 +0100, Eli Zaretskii wrote:
> > > It then requires new { brackets }
> > 
> > I don't think you need braces.  The compiler certainly doesn't.
> 
> While this is bikeshedding in its purest form I can jump in.
> 
> 
> >     if ([...])
> >       /* This is a comment that ...  */
> >       return;
> 
> This is a bug from the first sight as there are two C statements attached to
> an `if' conditional.  Two statements always need a block.  This is a bug.
> 
> I really do not have time to interrupt myself each time, several times
> a minute, looking at the code starting examining what those two statements
> semantically are, and therefore if they really require a block or not.

I agree with Jan here.

Re: Code formatting [Re: [patch] s390*: watchpoints regression [repost]]

[Eli Zaretskii]

> Date: Sun, 18 Dec 2011 14:52:09 +0100 (CET)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: brobecker@adacore.com, eliz@gnu.org, gdb-patches@sourceware.org,
>         pedro@codesourcery.com
> 
> > >     if ([...])
> > >       /* This is a comment that ...  */
> > >       return;
> > 
> > This is a bug from the first sight as there are two C statements attached to
> > an `if' conditional.  Two statements always need a block.  This is a bug.
> > 
> > I really do not have time to interrupt myself each time, several times
> > a minute, looking at the code starting examining what those two statements
> > semantically are, and therefore if they really require a block or not.
> 
> I agree with Jan here.

Any reasons why no one says anything about the alternative I
suggested?  AFAIU, it is free from all the disadvantages mentioned
here.

Re: Code formatting [Re: [patch] s390*: watchpoints regression [repost]]

[Jan Kratochvil]

On Sun, 18 Dec 2011 17:52:30 +0100, Eli Zaretskii wrote:
> Any reasons why no one says anything about the alternative I
> suggested?  AFAIU, it is free from all the disadvantages mentioned
> here.

You dropped the important part about "still going through the shell", that was
the surprising fact to note there.

If you do not drop that shell part of the text the Pedro's text becomes
shorter, therefore more clear.

Also you just describe 

  /* This is the main thread still going through the shell, or, no
     watchpoint has been set yet.  */
->
  /* Nothing else to do if this is the main thread, or if no
     watchpoints have been set yet.  */

additionally the "return" clause there.  "return" does not need any comment.


Regards,
Jan

Re: Code formatting [Re: [patch] s390*: watchpoints regression [repost]]

[Eli Zaretskii]

> Date: Sun, 18 Dec 2011 18:24:18 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Mark Kettenis <mark.kettenis@xs4all.nl>, brobecker@adacore.com,
>         gdb-patches@sourceware.org, pedro@codesourcery.com
> 
> You dropped the important part about "still going through the shell", that was
> the surprising fact to note there.
> 
> If you do not drop that shell part of the text the Pedro's text becomes
> shorter, therefore more clear.

Then don't drop it.  I think I dropped it by mistake.  Anyway, it was
just an example of how to reword a comment to avoid the problem that
started this thread.

> Also you just describe 
> 
>   /* This is the main thread still going through the shell, or, no
>      watchpoint has been set yet.  */
> ->
>   /* Nothing else to do if this is the main thread, or if no
>      watchpoints have been set yet.  */
> 
> additionally the "return" clause there.  "return" does not need any comment.

"Nothing else to do" explains _why_ we return, which is the point of
the comment, isn't it?

Re: Code formatting [Re: [patch] s390*: watchpoints regression [repost]]

[Pedro Alves]

On Sunday 18 December 2011 18:02:43, Eli Zaretskii wrote:
> > Date: Sun, 18 Dec 2011 18:24:18 +0100
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > Cc: Mark Kettenis <mark.kettenis@xs4all.nl>, brobecker@adacore.com,
> >         gdb-patches@sourceware.org, pedro@codesourcery.com
> > 
> > You dropped the important part about "still going through the shell", that was
> > the surprising fact to note there.
> > 
> > If you do not drop that shell part of the text the Pedro's text becomes
> > shorter, therefore more clear.
> 
> Then don't drop it.  I think I dropped it by mistake.  Anyway, it was
> just an example of how to reword a comment to avoid the problem that
> started this thread.

Thanks.  Here's what I checked in.

2011-12-20  Pedro Alves  <alves.ped@gmail.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

        * linux-nat.c (add_lwp): Don't call linux_nat_new_thread on the
	first LWP.
	* amd64-linux-nat.c (update_debug_registers_callback): Instantiate
	`lwp->arch_private' if NULL.
	(amd64_linux_prepare_to_resume): Do nothing if `lwp->arch_private'
	is NULL.
	* i386-linux-nat.c (update_debug_registers_callback): Instantiate
	`lwp->arch_private' if NULL.
	(i386_linux_prepare_to_resume): Do nothing if `lwp->arch_private'
	is NULL.
---
 gdb/amd64-linux-nat.c |    9 +++++++++
 gdb/i386-linux-nat.c  |    9 +++++++++
 gdb/linux-nat.c       |   10 +++++++++-
 3 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
index 288160b..865b971 100644
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -343,6 +343,9 @@ amd64_linux_dr_get_status (void)
 static int
 update_debug_registers_callback (struct lwp_info *lwp, void *arg)
 {
+  if (lwp->arch_private == NULL)
+    lwp->arch_private = XCNEW (struct arch_lwp_info);
+
   /* The actual update is done later just before resuming the lwp, we
      just mark that the registers need updating.  */
   lwp->arch_private->debug_registers_changed = 1;
@@ -386,6 +389,12 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
 {
   int clear_status = 0;
 
+  /* NULL means this is the main thread still going through the shell,
+     or, no watchpoint has been set yet.  In that case, there's
+     nothing to do.  */
+  if (lwp->arch_private == NULL)
+    return;
+
   if (lwp->arch_private->debug_registers_changed)
     {
       struct i386_debug_reg_state *state = i386_debug_reg_state ();
diff --git a/gdb/i386-linux-nat.c b/gdb/i386-linux-nat.c
index 190979b..4527913 100644
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -715,6 +715,9 @@ i386_linux_dr_get_status (void)
 static int
 update_debug_registers_callback (struct lwp_info *lwp, void *arg)
 {
+  if (lwp->arch_private == NULL)
+    lwp->arch_private = XCNEW (struct arch_lwp_info);
+
   /* The actual update is done later just before resuming the lwp, we
      just mark that the registers need updating.  */
   lwp->arch_private->debug_registers_changed = 1;
@@ -758,6 +761,12 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
 {
   int clear_status = 0;
 
+  /* NULL means this is the main thread still going through the shell,
+     or, no watchpoint has been set yet.  In that case, there's
+     nothing to do.  */
+  if (lwp->arch_private == NULL)
+    return;
+
   if (lwp->arch_private->debug_registers_changed)
     {
       struct i386_debug_reg_state *state = i386_debug_reg_state ();
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 1cbfc44..e5f7c3e 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1151,7 +1151,15 @@ add_lwp (ptid_t ptid)
   lp->next = lwp_list;
   lwp_list = lp;
 
-  if (linux_nat_new_thread != NULL)
+  /* Let the arch specific bits know about this new thread.  Current
+     clients of this callback take the opportunity to install
+     watchpoints in the new thread.  Don't do this for the first
+     thread though.  If we're spawning a child ("run"), the thread
+     executes the shell wrapper first, and we shouldn't touch it until
+     it execs the program we want to debug.  For "attach", it'd be
+     okay to call the callback, but it's not necessary, because
+     watchpoints can't yet have been inserted into the inferior.  */
+  if (num_lwps (GET_PID (ptid)) > 1 && linux_nat_new_thread != NULL)
     linux_nat_new_thread (lp);
 
   return lp;

Re: [patch] s390*: watchpoints regression [repost]

[Eli Zaretskii]

> Date: Sun, 18 Dec 2011 10:58:50 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Pedro Alves <pedro@codesourcery.com>, gdb-patches@sourceware.org
> 
> On Sun, 18 Dec 2011 07:21:27 +0100, Joel Brobecker wrote:
> > > +  /* This is the main thread still going through the shell, or, no
> > > +     watchpoint has been set yet.  */
> > > +  if (lwp->arch_private == NULL)
> > > +    return;
> > 
> > Just a really minor nitpick: Would you consider putting the comment
> > inside the if, instead of just before? I think it'd be slightly clearer
> > that way.
> 
> FYI I am aware of this rule and I try to follow it but it seems unnatural to
> me.  It then requires new { brackets }

I don't think you need braces.  The compiler certainly doesn't.

An alternative is to have the comment where you put it, but rephrase
it so that it references both the if-clause and the action.  For
example:

  /* Nothing else to do if this is the main thread, or if no
     watchpoints have been set yet.  */
  if (lwp->arch_private == NULL)
    return;

Re: [patch] s390*: watchpoints regression [repost]

[Ulrich Weigand]

Pedro Alves wrote:
> 2011-12-17  Pedro Alves  <pedro@codesourcery.com>
>             Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
>         * linux-nat.c (add_lwp): Don't call linux_nat_new_thread on the
> 	first LWP.
> 	* amd64-linux-nat.c (update_debug_registers_callback): Instantiate
> 	`lwp->arch_private' if NULL.
> 	(amd64_linux_prepare_to_resume): Do nothing if `lwp->arch_private'
> 	is NULL.
> 	* i386-linux-nat.c (update_debug_registers_callback): Instantiate
> 	`lwp->arch_private' if NULL.
> 	(i386_linux_prepare_to_resume): Do nothing if `lwp->arch_private'
> 	is NULL.

I've just noticed that hardware break-/watchpoint support is currently
broken on ARM as well -- this patch fixes that regression as well.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com