[PATCHv3,Hurd] Add hardware watch support

  Hi Samuel!

Many thanks for persisting with this patch.  The GDB testsuite shows a
pretty good improvement!  I'll try to assess the remaining issues, but
From a functional point of view that patch is much of an improvement
already.

Unless you have push access to sourceware (do you?), I'll be happy to
push this for you once the pending review comments have been addressed.
I have just to contribute a small patch to add on top of yours (merge it
in) to make it actually build:



Grüße,
 Thomas
  

Thomas Schwinge, le Fri 12 Sep 2014 19:56:13 +0200, a écrit :
> Unless you have push access to sourceware (do you?), I'll be happy to
> push this for you once the pending review comments have been addressed.
> I have just to contribute a small patch to add on top of yours (merge it
> in) to make it actually build:
> -inf_threads (struct inf *inf, inf_threads_ftype *f)
> +inf_threads (struct inf *inf, inf_threads_ftype *f, void *arg)

Ugh?  I guess with having to deal with various trees, some building and
others not, I just manage to mangle that patch.

> Many thanks for persisting with this patch.

I have to say I'm almost about to give up with submitting it.

Samuel
  

Thomas Schwinge, le Fri 12 Sep 2014 19:56:13 +0200, a écrit :
> Unless you have push access to sourceware (do you?),

I don't.

Samuel
  

> > Many thanks for persisting with this patch.
> 
> I have to say I'm almost about to give up with submitting it.

I would be very interested in hearing your honest feedback on this.
I know it took a long time, and we're not always very responsive,
but we try our best. If we could hear what made you feel this way,
I would like to try to see if there are any ways we can improve
the situation for you.
  

Joel Brobecker, le Fri 12 Sep 2014 13:01:41 -0700, a écrit :
> > > Many thanks for persisting with this patch.
> > 
> > I have to say I'm almost about to give up with submitting it.
> 
> I would be very interested in hearing your honest feedback on this.
> I know it took a long time, and we're not always very responsive,
> but we try our best. If we could hear what made you feel this way,
> I would like to try to see if there are any ways we can improve
> the situation for you.

I'm sorry I wrote it in such a harsh way while you were all polite in
your requests. Actually it's an unfortunate combination of me also
having a pending patch to the Linux kernel which has been waiting for
several years, with only sporadic reviews, then a request for reshaping,
and ended up with a "well, the way it is done will not fly" without
very much details or discussion, and another pending patch to qemu,
which apparently nobody has the time to review, while they are really
interested in it. Seeing a submission to gdb getting stuck on missing
line breaks got me a bit on my nerves. Fortunately it wasn't only
about that, but also important changes, so I carried on, but having to
resubmit only for missing spaces or new lines would have been really
hard to me, since I don't really plan to submit many patches to gdb,
and I know I'll always make this kind of mistakes since I'm submitting
patches to a lot of various projects with very differing coding styles.

What may help in the process would be to have a script which checks for
style. The Linux kernel's checkpatch.pl is a very good approach, since
one can get to check one's own style quite thoroughly before submitting.

Samuel
  

On Friday, September 12 2014, Samuel Thibault wrote:

> What may help in the process would be to have a script which checks for
> style. The Linux kernel's checkpatch.pl is a very good approach, since
> one can get to check one's own style quite thoroughly before submitting.

This has been discussed a few times at least, but so far we haven't done
much about the subject.  I will try to dedicate some time this weekend
to see if I can come up with an initial attempt to do those checkings
automatically.

I am sorry about your difficulties (and I admit I helped when I dumbly
asked for a testcase), and I am glad that your patch made it.

Thanks,
  

> Seeing a submission to gdb getting stuck on missing line breaks got me
> a bit on my nerves. Fortunately it wasn't only about that, but also
> important changes, so I carried on, but having to resubmit only for
> missing spaces or new lines would have been really hard to me, since I
> don't really plan to submit many patches to gdb, and I know I'll
> always make this kind of mistakes since I'm submitting patches to a
> lot of various projects with very differing coding styles.

I understand the fustration, and I certainly would agree if you said
that GDB is very particular with its CS. Aiming for a consistent coding
style is important for long-term maintainability, though, which is why
we almost systematically mention violations we see. If it makes you feel
any better, we make mistakes too, sometimes, and have to re-edit as
a result.  One thing I will say, however, is that I often let go of
minor violations, and only mention them if there are other adjustments
I'd like to request in the same area. And I know others can be the same.
So we try not be be overly picky (you may not believe me on that one ;-)!).

As Sergio mentioned, we do not have a script we can use to check
coding-style violations.  We would very much like to have one,
but no one really sent anything so far. What I can do to help is
being your CS corrector - just send me your patch privately before
submitting, and I will fix all CS violations for you.

Another important part I can see contributing to the fustration
is review delays. I can see how trivial requests can be extra
fustrating if you feel like your patch is being delayed quite
a bit because each review cycle is very long. You can help with
that by pinging us. The accepted practice is to ping after a week
or two, and every week thereafter.
  

Hi Samuel!

On Fri, 12 Sep 2014 19:56:13 +0200, I wrote:
> Many thanks for persisting with this patch.  The GDB testsuite shows a
> pretty good improvement!  I'll try to assess the remaining issues

From a quick scan through the »FAIL:.*watch« matches, the following one
issue should be relevant for a lot of the regressions: given, for
example, the gdb.base/pr11022 test case, on GNU/Linux we see:

    $ gcc [...]/gdb/testsuite/gdb.base/pr11022.c -g -o pr11022
    $ ./gdb -q -ex 'watch x' -ex run pr11022 
    [...]
    Hardware watchpoint 1: x
    
    Old value = 0
    New value = 42
    main () at ../../W._C._Handy/gdb/testsuite/gdb.base/pr11022.c:28
    28          j = i;  /* expect HW watchpoint stop */

... whereas on GNU Hurd we see:

    [...]
    Program received signal SIGTRAP, Trace/breakpoint trap.
    main () at ../../../W._C._Handy/gdb/testsuite/gdb.base/pr11022.c:28
    28          j = i;  /* expect HW watchpoint stop */

You may want to prepend »-ex 'maintenance set show-debug-regs on'« to see
some debugging output (and notice that »watchpoint_hit« is missing on GNU
Hurd).

I tracked this down to gdb/nat/x86-dregs.c:x86_dr_stopped_data_address
and x86_dr_stopped_by_watchpoint not returning the information as
desired, and tracked that in turn down to our kernel interface always
returning zero for DR6, the debug status register.  See the comment in
x86_dr_stopped_data_address for how GDB is using this information.  Do
you agree that thread_get_state(i386_DEBUG_STATE) should be returning the
actual DR6, and where in GNU Mach would we need to copy the DR6 register
into the PCB?


Grüße,
 Thomas
  

Thomas Schwinge, le Tue 16 Sep 2014 00:08:01 +0200, a écrit :
> Do you agree that thread_get_state(i386_DEBUG_STATE) should be
> returning the actual DR6,

Indeed.

> and where in GNU Mach would we need to copy the DR6
> register into the PCB?

it would be user_trap(), probably, in the T_DEBUG case.

Samuel