From patchwork Wed Jun 17 13:26:01 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luis Machado X-Patchwork-Id: 7213 Received: (qmail 10690 invoked by alias); 17 Jun 2015 13:26:11 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 10668 invoked by uid 89); 17 Jun 2015 13:26:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, KAM_WEIRDTRICK1, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=no version=3.3.2 X-HELO: relay1.mentorg.com 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; Wed, 17 Jun 2015 13:26:08 +0000 Received: from svr-orw-fem-05.mgc.mentorg.com ([147.34.97.43]) by relay1.mentorg.com with esmtp id 1Z5DLw-00056c-Np from Luis_Gustavo@mentor.com ; Wed, 17 Jun 2015 06:26:04 -0700 Received: from [172.30.7.9] (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.3.224.2; Wed, 17 Jun 2015 06:26:04 -0700 Message-ID: <55817569.7060704@codesourcery.com> Date: Wed, 17 Jun 2015 10:26:01 -0300 From: Luis Machado Reply-To: Luis Machado User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Pedro Alves , Subject: Re: [PATCH] Fix problems with finishing a dummy function call on simulators. References: <1433862056-18237-1-git-send-email-lgustavo@codesourcery.com> <55772797.802@redhat.com> <55805F52.20805@codesourcery.com> <55816AD5.6020605@redhat.com> In-Reply-To: <55816AD5.6020605@redhat.com> X-IsSubscribed: yes On 06/17/2015 09:40 AM, Pedro Alves wrote: > On 06/09/2015 07:22 PM, Luis Machado wrote: >> One strange side effect of this change on my local machine (x86-64) is >> that gdb.threads/attach-many-short-lived-threads.exp gives me PASS >> instead of FAIL when always-inserted mode is ON. I didn't investigate >> this further though. > > You mean you _always_ get a FAIL before your patch? This test sometimes > FAILs for an unknown reason, but it's racy -- it should be passing most of > the time. > I went back to this and yes, it is indeed racy. It just so happens that it failed both times i checked the results of an unpatched testsuite run. :-) >> Is this patch what you had in mind? > > Yep. Close, but also remove the bp_call_dummy check in > bp_loc_is_permanent, and merge in its comment, like ... > Fixed now. >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c >> index eb3df02..768ce59 100644 >> --- a/gdb/breakpoint.c >> +++ b/gdb/breakpoint.c >> @@ -7440,15 +7440,16 @@ make_breakpoint_permanent (struct breakpoint *b) >> struct bp_location *bl; >> >> /* By definition, permanent breakpoints are already present in the >> - code. Mark all locations as inserted. For now, >> - make_breakpoint_permanent is called in just one place, so it's >> - hard to say if it's reasonable to have permanent breakpoint with >> - multiple locations or not, but it's easy to implement. */ >> + code. For now, make_breakpoint_permanent is called in just one place, so >> + it's hard to say if it's reasonable to have permanent breakpoint with >> + multiple locations or not, but it's easy to implement. >> + >> + Permanent breakpoints are not marked as inserted so we allow other >> + non-permanent locations at the same address to be inserted on top >> + of it. This is required due to some targets, simulators mostly, not >> + dealing properly with hardwired breakpoints in the code. */ > > ... this: > > /* While by definition, permanent breakpoints are already present in the > code, we don't mark the location as inserted. Normally one would expect > that GDB could rely on that breakpoint instruction to stop the program, thus > removing the need to insert its own breakpoint, except that executing the > breakpoint instruction can kill the target instead of reporting a SIGTRAP. > E.g., on SPARC, when interrupts are disabled, executing the instruction > resets the CPU, so QEMU 2.0.0 for SPARC correspondingly dies > with "Trap 0x02 while interrupts disabled, Error state". Letting the > breakpoint be inserted normally results in QEMU knowing about the GDB > breakpoint, and thus trap before the breakpoint instruction is executed. > (If GDB later needs to continue execution past the permanent breakpoint, > it manually increments the PC, thus avoiding executing the breakpoint > instruction.) > >> for (bl = b->loc; bl; bl = bl->next) >> - { >> - bl->permanent = 1; >> - bl->inserted = 1; >> - } >> + bl->permanent = 1; >> } >> I've updated this now. > > Actually, make_breakpoint_permanent is dead and should be deleted. The > last remaining caller is finally gone - it was one of the old Unix ports > we removed. So the comment should be moved to add_location_to_breakpoint > instead. Ah, that explains why i couldn't find callers of that function, but the build didn't fail due to it being unused. The attached updated patch also removes this dead function. I can commit it as a separate patch if you're not removing it yourself. Thanks! 2015-06-17 Luis Machado * breakpoint.c (make_breakpoint_permanent): Remove unused function. (add_location_to_breakpoint): Don't mark permanent locations as inserted. Update and expand comment about permanent locations. (bp_loc_is_permanent): Don't return 0 for bp_call_dummy. Move comment to add_location_to_breakpoint. (update_global_location_list): Don't error out if a permanent breakpoint is not marked inserted. Don't error out if a non-permanent breakpoint location is inserted on top of a permanent breakpoint. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 657c58e..d731f1f 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -7431,26 +7431,6 @@ set_raw_breakpoint (struct gdbarch *gdbarch, return b; } - -/* Note that the breakpoint object B describes a permanent breakpoint - instruction, hard-wired into the inferior's code. */ -void -make_breakpoint_permanent (struct breakpoint *b) -{ - struct bp_location *bl; - - /* By definition, permanent breakpoints are already present in the - code. Mark all locations as inserted. For now, - make_breakpoint_permanent is called in just one place, so it's - hard to say if it's reasonable to have permanent breakpoint with - multiple locations or not, but it's easy to implement. */ - for (bl = b->loc; bl; bl = bl->next) - { - bl->permanent = 1; - bl->inserted = 1; - } -} - /* Call this routine when stepping and nexting to enable a breakpoint if we do a longjmp() or 'throw' in TP. FRAME is the frame which initiated the operation. */ @@ -8918,11 +8898,21 @@ add_location_to_breakpoint (struct breakpoint *b, set_breakpoint_location_function (loc, sal->explicit_pc || sal->explicit_line); + /* While by definition, permanent breakpoints are already present in the + code, we don't mark the location as inserted. Normally one would expect + that GDB could rely on that breakpoint instruction to stop the program, + thus removing the need to insert its own breakpoint, except that executing + the breakpoint instruction can kill the target instead of reporting a + SIGTRAP. E.g., on SPARC, when interrupts are disabled, executing the + instruction resets the CPU, so QEMU 2.0.0 for SPARC correspondingly dies + with "Trap 0x02 while interrupts disabled, Error state". Letting the + breakpoint be inserted normally results in QEMU knowing about the GDB + breakpoint, and thus trap before the breakpoint instruction is executed. + (If GDB later needs to continue execution past the permanent breakpoint, + it manually increments the PC, thus avoiding executing the breakpoint + instruction.) */ if (bp_loc_is_permanent (loc)) - { - loc->inserted = 1; - loc->permanent = 1; - } + loc->permanent = 1; return loc; } @@ -8974,20 +8964,6 @@ bp_loc_is_permanent (struct bp_location *loc) gdb_assert (loc != NULL); - /* bp_call_dummy breakpoint locations are usually memory locations - where GDB just wrote a breakpoint instruction, making it look - as if there is a permanent breakpoint at that location. Considering - it permanent makes GDB rely on that breakpoint instruction to stop - the program, thus removing the need to insert its own breakpoint - there. This is normally expected to work, except that some versions - of QEMU (Eg: QEMU 2.0.0 for SPARC) just report a fatal problem (Trap - 0x02 while interrupts disabled, Error state) instead of reporting - a SIGTRAP. QEMU should probably be fixed, but in the interest of - compatibility with versions that behave this way, we always consider - bp_call_dummy breakpoint locations as non-permanent. */ - if (loc->owner->type == bp_call_dummy) - return 0; - cleanup = save_current_space_and_thread (); switch_to_program_space_and_thread (loc->pspace); @@ -12430,12 +12406,6 @@ update_global_location_list (enum ugll_insert_mode insert_mode) continue; } - /* Permanent breakpoint should always be inserted. */ - if (loc->permanent && ! loc->inserted) - internal_error (__FILE__, __LINE__, - _("allegedly permanent breakpoint is not " - "actually inserted")); - if (b->type == bp_hardware_watchpoint) loc_first_p = &wp_loc_first; else if (b->type == bp_read_watchpoint) @@ -12471,12 +12441,6 @@ update_global_location_list (enum ugll_insert_mode insert_mode) /* Clear the condition modification flag. */ loc->condition_changed = condition_unchanged; - - if (loc->inserted && !loc->permanent - && (*loc_first_p)->permanent) - internal_error (__FILE__, __LINE__, - _("another breakpoint was inserted on top of " - "a permanent breakpoint")); } if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ())