Re: [PATCH 1/2] Port gdbserver to GNU/Hurd

[Thomas Schwinge]

Hi!

Just a very quick one; short on time.

On Thu, 05 Sep 2013 20:29:43 +0100, Pedro Alves <palves@redhat.com> wrote:
> On 09/05/2013 11:53 AM, Yue Lu wrote:
> > This is the my new patch.

You've received quite some positive feedback, good!  :-)

> Thanks.  Follows a few comments, by no means an in depth review.

Thanks for those reviews -- helps a lot.

> We'll probably need to iterate a few times.  I'm counting on
> Thomas and others to help as well!

Yes, but unfortunately not immediatelly.  Anyway, the more the code
converges towards using the present gnu-nat.c code on the one hand (which
I'll try to help with) as well as the present gdbserver code on the other
hand (which I don't have experience with, but Pedro has already pointed
out several items), the easier it will be to review.  And, we've made
quite a step forward with this revision of the patch, as far as I can
tell!

> It'd be very useful if you send and maintain along
> with the patch the rationale for all design divergences between
> the ports you found necessary.  E.g., it seems that gdbserver's
> task/proc bookkeeping is different from gdb's.  Why?

Yes, it'd be useful to have some source code comments next to (some of)
the »#ifdef GDBSERVER« (as well as elsewhere, of course).

> > --- a/gdb/config/i386/i386gnu.mh
> > +++ b/gdb/config/i386/i386gnu.mh
> > @@ -1,4 +1,5 @@
> >  # Host: Intel 386 running the GNU Hurd
> > +ifndef GDBSERVER
> >  NATDEPFILES= i386gnu-nat.o gnu-nat.o core-regset.o fork-child.o \
> >       notify_S.o process_reply_S.o msg_reply_S.o \
> >       msg_U.o exc_request_U.o exc_request_S.o
> > @@ -12,6 +13,10 @@ XM_CLIBS = -lshouldbeinlibc
> >  # Use our own user stubs for the msg rpcs, so we can make them time out, in
> >  # case the program is fucked, or we guess the wrong signal thread.
> >  msg-MIGUFLAGS = -D'MSG_IMPORTS=waittime 1000;'
> > +else
> > +NATDEPFILES= notify_S.o process_reply_S.o msg_reply_S.o \
> > +      exc_request_U.o exc_request_S.o
> > +endif

Pedro, is the idea that this file should also be shared with gdbserver?
Well, probably yes (for the moment), and the idea really is to move the
shared bits into common/, as with the other shared code.  So, I guess
this is fine for the moment.

> > --- a/gdb/gnu-nat.c
> > +++ b/gdb/gnu-nat.c
> > @@ -1,6 +1,7 @@
> >  /* Interface GDB to the GNU Hurd.
> >     Copyright (C) 1992-2013 Free Software Foundation, Inc.
> > 
> > +
> >     This file is part of GDB.
> 
> Please go through the patch and remove spurious whitespace
> changes line these.

Yes, this is generally good advise -- I (try to...) re-read every patch
I'm about to send by email or directly commit, and avoid any such
"spurious" changes.  GDB (as other GNU projects, too) tend to set a high
standard on code quality, which is good, and this also includes such
discipline when doing changes to the code.

> > +#ifdef GDBSERVER
> > +/* this should move into gnu-i386-low.c ?*/
> 
> I guess that means i386gnu-nat.c now, but it sounds like
> it, yes.
> 
> > +/* Defined in auto-generated file i386.c.  */
> > +extern void init_registers_i386 (void);
> > +extern const struct target_desc *tdesc_i386;

Yes, something with a i386 tag in it can't be in the generic gnu-nat.c
file.  (Not that there'd be any other GNU Hurd ports available, but you
get the idea.)

> (correct me if
> I'm wrong here), the Hurd's threads are kernel threads

Correct.

> so it'd
> be better to just make the GDB side use the lwp field too.
> It's really a simple and mechanic change.  Nothing in GDB core
> actually cares which field is used.  So in this case, it'd be
> better if you send a preparatory patch

Based on the current upstream master branch.

> that does that change
> (and nothing else)

Confirm that you haven't caused any regressions by running the GDB
testsuite (natively) without and then with your change (don't forget to
apply the testsuite patch I gave you earlier, to avoid the testsuite
hanging (known Hurd issue)), and diff the *.sum files to see that there
are no (major) differences (post them with your patch).

> and then rebase the port on top of that
> patch.

Please tell if you need help with how to use Git to rebase your current
patch onto the upstream master branch.  Also, did you figure out your
earlier question about how to merge several Git commits into one?

> >  /* Attach to process PID, then initialize for debugging it
> >     and wait for the trace-trap that results from attaching.  */
> > +#ifndef GDBSERVER
> >  static void
> >  gnu_attach (struct target_ops *ops, char *args, int from_tty)
> >  {
> > @@ -2207,8 +2347,14 @@ gnu_attach (struct target_ops *ops, char *args,
> > int from_tty)
> >    renumber_threads (0); /* Give our threads reasonable names.  */
> >  #endif
> >  }
> > +#else
> > +static int
> > +gnu_attach (unsigned long pid)
> > +{
> > +  return -1; //not support now
> 
> Doesn't look like this should be hard to get going.

With the native GDB port, there is a known issue when attaching to a
running processes (specifically if that process is currently doing a
kernel call (Mach RPC)), so I suggested to not spend time on this
functionality for the moment, and we can fix (native GDB) and implement
(gdbserver) that it later.

> >  /* Take a program previously attached to and detaches it.
> >     The program resumes execution and will no longer stop
> >     on signals, etc.  We'd better not have left any breakpoints
> > @@ -2216,6 +2362,7 @@ gnu_attach (struct target_ops *ops, char *args,
> > int from_tty)
> >     to work, it may be necessary for the process to have been
> >     previously attached.  It *might* work if the program was
> >     started via fork.  */
> > +#ifndef GDBSERVER
> >  static void
> >  gnu_detach (struct target_ops *ops, char *args, int from_tty)
> >  {
> > @@ -2255,7 +2402,25 @@ gnu_stop (ptid_t ptid)
> >  {
> >    error (_("to_stop target function not implemented"));
> >  }
> > +#else
> > +static int
> > +gnu_detach (int pid)
> > +{
> 
> Eheh, we have detach but not attach.  ;-)

Even if attaching doesn't, detaching from a process started with
gdbserver does work.  ;-)

> > +static void
> > +gnu_request_interrupt (void)
> > +{
> > +  printf ("gnu_request_interrupt not support!\n");
> > +  exit (-1);
> 
> That's quite limiting.  :-/  Doesn't sending a SIGINT
> work?

Please give that a try, but if it's more difficult, I again suggest we
finish that later.

> > --- a/gdb/gnu-nat.h
> > +++ b/gdb/gnu-nat.h

> > +#define THREAD_STATE_FLAVOR i386_REGS_SEGS_STATE
> > +#define THREAD_STATE_SIZE i386_THREAD_STATE_COUNT
> > +#define THREAD_STATE_SET_TRACED(state) \
> > +   ((struct i386_thread_state *) (state))->efl |= 0x100
> > +#define THREAD_STATE_CLEAR_TRACED(state) \
> > +   ((((struct i386_thread_state *) (state))->efl &= ~0x100), 1)

That's again for a i386 file.

Further review to come later.


Now, please try to address the comments raised (especially also Pedro's),
and don't hesitate to ask if there are any questions.


Grüße,
 Thomas

pgphYe4QNt1_4.pgp
Description: PGP signature