From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2165 invoked by alias); 3 Nov 2011 19:33:32 -0000 Received: (qmail 2155 invoked by uid 22791); 3 Nov 2011 19:33:31 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate1.uk.ibm.com (HELO mtagate1.uk.ibm.com) (194.196.100.161) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 03 Nov 2011 19:33:17 +0000 Received: from d06nrmr1707.portsmouth.uk.ibm.com (d06nrmr1707.portsmouth.uk.ibm.com [9.149.39.225]) by mtagate1.uk.ibm.com (8.13.1/8.13.1) with ESMTP id pA3JXEXr008873 for ; Thu, 3 Nov 2011 19:33:14 GMT Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by d06nrmr1707.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pA3JXEB21974508 for ; Thu, 3 Nov 2011 19:33:14 GMT Received: from d06av02.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pA3JXEAb016865 for ; Thu, 3 Nov 2011 13:33:14 -0600 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id pA3JXCcs016848; Thu, 3 Nov 2011 13:33:13 -0600 Message-Id: <201111031933.pA3JXCcs016848@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 03 Nov 2011 20:33:12 +0100 Subject: Re: RFC: don't set the pspace on ordinary breakpoints To: pedro@codesourcery.com (Pedro Alves) Date: Thu, 03 Nov 2011 19:33:00 -0000 From: "Ulrich Weigand" Cc: tromey@redhat.com (Tom Tromey), gdb-patches@sourceware.org In-Reply-To: <201111031601.51221.pedro@codesourcery.com> from "Pedro Alves" at Nov 03, 2011 04:01:50 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2011-11/txt/msg00100.txt.bz2 Pedro Alves wrote: > On Thursday 27 October 2011 16:20:38, Tom Tromey wrote: > > Second, this patch removes bp_startup_disabled. Instead, I changed > > should_be_inserted to directly examine the location's pspace, and I > > changed one spot (update_global_location_list) to use should_be_inserted > > to pick this up. > > I don't think that's correct. During startup, we disable user breakpoints, > because the symbols haven't been relocated yet. But, we still need to > insert internal breakpoints set at magic addresses (not through symbols), so > that we know when the startup is done with. Ulrich? The scenario that was fixed by the disabled-during-startup feature is debugging stand-alone SPU applications on Cell/B.E. using the combined PPU/SPU debugger. A stand-alone application is an SPU architecture ELF binary; running such a binary involves binfmt_misc loader which is a PowerPC executable that loads the SPU binary up into an SPU context and runs it. >From GDB's perspective, before the program is started, the inferior is just a plain "exec" target for the SPU binary. The user will be able to set breakpoint on SPU funtions, source lines etc. Once the binary is started, however, GDB will suddenly see the PowerPC loader executable. After a while, once the loader has finished starting up the SPU context, GDB will suddenly see the SPU executable (in that newly started context) again. At that point, all breakpoints can be re-set (properly relocated to the virtual address that includes the SPU context ID). However, in the time in-between, when the loader has started up but not yet loaded the SPU executable, any attempt to place a SPU breakpoint will fail, because nothing is at those addresses. Likewise, any attempt to re-set the breakpoint will fail, because the SPU executable is not in fact visible in the inferior during this time period. Thus the idea was to simply make all such breakpoints that might have been set by the user during the "exec" target phase completely dormant until such time as the SPU context management code in solib-spu.c has detected the presence of the newly started-up SPU context. However, *system* breakpoints, in particular those needed by the shared library (and SPU context) layers, must definitely be placed as usual during that period, otherwise that detection of the new SPU context will itself fail. > On Thursday 03 November 2011 14:01:19, Tom Tromey wrote: > > >>>>> "Pedro" == Pedro Alves writes: > > > > Pedro> Thanks. IMO, the executing_startup change IMO could go in > > Pedro> separately, as soon as it is correct, it looks quite independent. > > Pedro> What prompted it, BTW? > > > > It seemed wrong to mark an entire breakpoint as "startup disabled" in > > the case where the breakpoint has locations in multiple inferiors. > > Ah, indeed. > > > static void > > bkpt_re_set (struct breakpoint *b) > > { > > - /* Do not attempt to re-set breakpoints disabled during startup. */ > > - if (b->enable_state == bp_startup_disabled) > > - return; > > - > > I'm looking at this one, and thinking that if we simply remove the > guard, we'll allow re-set to go through while we can't trust > symbols/sections in the program space, because they haven't been > relocated yet, but this is wrong (as opposed to just inneficient) > because GDB will actually read memory from the wrong addresses > (prologue skipping, breakpoint shadow, at least). > > We can't just make bp_startup_disabled be per-location, > because a re-set could find new locations, defeating the guard. Agreed, exactly. > So I think we'll maybe want this: > > static void > bkpt_re_set (struct breakpoint *b) > { > - /* Do not attempt to re-set breakpoints disabled during startup. */ > - if (b->enable_state == bp_startup_disabled) > - return; > + if (current_program_space->executing_startup) > + return 0; > > And adjust the `current_program_space' reference in the > ambiguous linespec patch to whatever will be necessary then. This assumes that bkpt_re_set is never called for the system breakpoints, which seems to be true as far as I can see ... > and: > > @@ -1578,6 +1578,9 @@ should_be_inserted (struct bp_location *bl) > if (!bl->enabled || bl->shlib_disabled || bl->duplicate) > return 0; > > + if (!bl->owner->ops->should_be_inserted (bl)) > + return 0; > > With the regular breakpoint's (or any other breakpoint that > uses linespecs) implementation of the new should_be_inserted hook > being: > > static int > bkpt_should_be_inserted (struct bp_location *bl) > { > return !bl->pspace->executing_startup; > } > > It seems to me that watchpoints shouldn't be > inserted during startup either (can't trust symbols > for the watchpoint's expression either), so maybe my > previous suggestion is indeed good enough: > > @@ -1578,6 +1578,9 @@ should_be_inserted (struct bp_location *bl) > if (!bl->enabled || bl->shlib_disabled || bl->duplicate) > return 0; > > -+ if (bl->pspace->executing_startup) > ++ if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup) > + return 0; That looks reasonable to me. > Unless we have some user breakpoint kind that would want to > be inserted in the startup phase. One such example may simply be > a breakpoint set at a specific address, to debug exactly the > startup phase. So we'd need: > > static int > bkpt_should_be_inserted (struct bp_location *bl) > { > if (!bl->pspace->executing_startup) > { > struct breakpoint *b = bl->owner; > > if (!linespec_is_address (b->addr_string)) > return 0; > > if (b->addr_string_range_end != NULL > && !linespec_is_address (b->addr_string_range_end)) > return 0; > } > > return 1; > } > > linespec_is_address would return true to linespecs of "*NUMBER" form. > (or if we have a linespec struct, we'd record address-ness there). For the SPU case, this would also cause failures, since even *NUMBER is not there while the loader is active. (Once the SPU context is back up, *NUMBER will work because the address will be re-located to the proper SPU context ID via the integer_to_address hook.) Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com