From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5986 invoked by alias); 24 Apr 2013 15:18:51 -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 5976 invoked by uid 89); 24 Apr 2013 15:18:51 -0000 X-Spam-SWARE-Status: No, score=-8.4 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 24 Apr 2013 15:18:50 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3OFImwC016785 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 24 Apr 2013 11:18:48 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3OFIkic029120; Wed, 24 Apr 2013 11:18:47 -0400 Message-ID: <5177F7D6.6070908@redhat.com> Date: Wed, 24 Apr 2013 19:04:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: lgustavo@codesourcery.com CC: "'gdb-patches@sourceware.org'" Subject: Re: [PATCH, ppc] Fix hw *points for embedded ppc in a threaded environment References: <516EC58C.5060501@codesourcery.com> <51755821.8020907@codesourcery.com> <51755A5F.3060009@redhat.com> <51756D2B.5050204@redhat.com> <51758960.2090702@redhat.com> <51768D08.4090709@codesourcery.com> <5176B3D9.2050702@redhat.com> <5176CD03.3070800@codesourcery.com> In-Reply-To: <5176CD03.3070800@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00743.txt.bz2 On 04/23/2013 07:03 PM, Luis Machado wrote: > On 04/23/2013 06:16 PM, Pedro Alves wrote: >> On 04/23/2013 02:30 PM, Luis Machado wrote: >>> /* Copy that thread's breakpoints and watchpoints to the new thread. */ >>> for (i = 0; i < max_slots_number; i++) >>> if (hw_breaks[i].hw_break) >>> - booke_insert_point (hw_breaks[i].hw_break, tid); >>> + { >>> + /* The ppc Linux kernel causes a thread to inherit its parent >>> + thread's debug state, and that includes any hardware >>> + watchpoints or breakpoints that the parent thread may have set. >> >> ISTR there was a point when the kernel changed behavior, right? >> It'd be good to mention the kernel version here. I _think_ you wanted >> to retain compatibility with older kernels, but, alas, the comment >> doesn't mention that. > > I can't state what version of the kernel had the behavior changed. I cc-ed the kernel folks in the original mail, but we received no answers back. > >> >> (Also, I'm left wondering if we couldn't detect the need for this >> just once, and skip several ptrace calls if we find the kernel >> does this for us already.) > > We probably could do that. The question is whether it brings us a great benefit compared to what we do now. Ideally the kernel developers would fix what is broken in my opinion. One idea for less intrusive debugging would be to make it so the kernel doesn't notified of new threads/clones at all, IOW, not force parent/child clones to stop to report the child's existence to a ptracer (unless it wants to). Having the kernel auto-copy debug resources in the new child is a requirement for that, and given ptrace options and other things are copied as well, I can't really call it broken outright. I'd suggest just mixing something like Older kernels did not make new threads inherit their parent thread's debug state, so we always clear the slot and replicate the debug state ourselves, ensuring compatibility with all kernels. into the existing comment. >>> +# Run to `main' where we begin our tests. >>> +if ![runto_main] then { >>> + gdb_suppress_tests >> >> Avoid gdb_suppress_tests. Just fail/return as usual. >> > > Done. It just returns 0 now. It does not: +# Run to `main' where we begin our tests. +if ![runto_main] then { + fail "Failed to run to main" +} + >>> +# Target cannot insert hardware watchpoints. Maybe it should have reported >>> +# it does not supported them in the first place. >> >> "does not support them". What does does this "maybe" note mean though? >> A bug? Where? >> > > It means that whoever has configured the testsuite for this target did not clearly specify it does not support hardware watchpoints (in gdbserver, for example). Let's call "unsupported" or even FAIL then, so it can draw attention. We could also end up in that situation due to a bug somewhere in the test or gdb. Something like 'fail "no hardware watchpoints"'. > 2013-04-23 Luis Machado > > * ppc-linux-nat.c (ppc_linux_new_thread): Clear the new thread's > debug state prior to replicating existing hardware watchpoints or > breakpoints. > > * gdb/testsuite/gdb.threads/wp-replication.c: New file. > * gdb/testsuite/gdb.threads/wp-replication.exp: New file. Recall that these go to gdb/testsuite/ChangeLog. To indicate to the reviewer that was not a mistake, I suggest posting the entry like: 2013-04-23 Luis Machado gdb/ * ppc-linux-nat.c (ppc_linux_new_thread): Clear the new thread's debug state prior to replicating existing hardware watchpoints or breakpoints. gdb/testsuite/ * gdb.threads/wp-replication.c: New file. * gdb.threads/wp-replication.exp: New file. or: gdb/ 2013-04-23 Luis Machado * ppc-linux-nat.c (ppc_linux_new_thread): Clear the new thread's debug state prior to replicating existing hardware watchpoints or breakpoints. gdb/testsuite/ 2013-04-23 Luis Machado * gdb.threads/wp-replication.c: New file. * gdb.threads/wp-replication.exp: New file. > +void empty_cycle (void) '\n' after first void. > +for { set i 1 } { $i <= $TRIGGERS} { incr i} { -------^ -----^ Missing spaces. > +# Move the threads and hit the watchpoints > +# TRIGGERS times. Nit, that fits in a single line. Otherwise OK. Thanks, -- Pedro Alves