From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9715 invoked by alias); 5 Sep 2013 21:38:00 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 9706 invoked by uid 89); 5 Sep 2013 21:38:00 -0000 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 05 Sep 2013 21:38:00 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.7 required=5.0 tests=AWL,BAYES_00,KHOP_PGP_SIGNED,KHOP_THREADED,RDNS_NONE,SPF_HELO_FAIL autolearn=no version=3.3.2 X-HELO: relay1.mentorg.com Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1VHhFQ-00078R-2Q from Thomas_Schwinge@mentor.com ; Thu, 05 Sep 2013 14:37:52 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Thu, 5 Sep 2013 14:37:52 -0700 Received: from feldtkeller.schwinge.homeip.net (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.2.247.3; Thu, 5 Sep 2013 22:37:50 +0100 From: Thomas Schwinge To: Pedro Alves , Yue Lu CC: gdb-patches , Luis Machado , Subject: Re: [PATCH 1/2] Port gdbserver to GNU/Hurd In-Reply-To: <5228DBA7.9050408@redhat.com> References: <87txi2i6t6.fsf@kepler.schwinge.homeip.net> <5225C3C6.8090101@redhat.com> <5228DBA7.9050408@redhat.com> User-Agent: Notmuch/0.9-101-g81dad07 (http://notmuchmail.org) Emacs/23.4.1 (i486-pc-linux-gnu) Date: Thu, 05 Sep 2013 21:38:00 -0000 Message-ID: <87ob87c5lr.fsf@kepler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" X-SW-Source: 2013-09/txt/msg00189.txt.bz2 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-length: 7041 Hi! Just a very quick one; short on time. On Thu, 05 Sep 2013 20:29:43 +0100, Pedro Alves 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 =C2=BB#ifdef GDBSERVER=C2=AB (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=3D 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 =3D -lshouldbeinlibc > > # Use our own user stubs for the msg rpcs, so we can make them time ou= t, in > > # case the program is fucked, or we guess the wrong signal thread. > > msg-MIGUFLAGS =3D -D'MSG_IMPORTS=3Dwaittime 1000;' > > +else > > +NATDEPFILES=3D 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. > >=20 > > + > > This file is part of GDB. >=20 > 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 ?*/ >=20 > I guess that means i386gnu-nat.c now, but it sounds like > it, yes. >=20 > > +/* 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 >=20 > 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) > > +{ >=20 > 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); >=20 > 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 |=3D 0x100 > > +#define THREAD_STATE_CLEAR_TRACED(state) \ > > + ((((struct i386_thread_state *) (state))->efl &=3D ~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=C3=BC=C3=9Fe, Thomas --=-=-= Content-Type: application/pgp-signature Content-length: 489 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAEBAgAGBQJSKPmgAAoJEGe3hdm9kOiinRkIAL8fadxrBa1IFplPcZ9J1h/F 7dWEhn3RCSjwQDs2x9reXkAwBu5vnccYo7boqNDmjnUCRn9rreoWBuHvFIHWA3nD ywX3DNxDS9hoENrsFiAeeSxZumLXWqLKSLnmNDLoFjbDBuLtTXSlCJQZDBrKYs7P g2iTWMZeDssTyVUcq8Q08wuZY3WoI2R4WTgEUq2qozhwhsNqqb3Akeh2tr2ie8cq 2Xm2dbdq8aRxdE/XaYdBzy7sQaBri9sxVd5paoJDznFCe9uIfldXfbUxQUbF15Kd hpICN4jh5gDq1joMEvaTJOgfECDVHPZmuP6Kx5oUkVIXXx2uVwzWaz8WCVfOhqc= =NOxE -----END PGP SIGNATURE----- --=-=-=--