From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6530 invoked by alias); 16 Apr 2014 03:22:45 -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 6499 invoked by uid 89); 16 Apr 2014 03:22:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00 autolearn=ham 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, 16 Apr 2014 03:22:42 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1WaGQo-00070t-H7 from Yao_Qi@mentor.com ; Tue, 15 Apr 2014 20:22:38 -0700 Received: from SVR-ORW-FEM-03.mgc.mentorg.com ([147.34.97.39]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Tue, 15 Apr 2014 20:22:36 -0700 Received: from qiyao.dyndns.org (147.34.91.1) by svr-orw-fem-03.mgc.mentorg.com (147.34.97.39) with Microsoft SMTP Server id 14.2.247.3; Tue, 15 Apr 2014 20:22:37 -0700 Message-ID: <534DF6EC.2040000@codesourcery.com> Date: Wed, 16 Apr 2014 03:22:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Pedro Alves , CC: Nicolas Blanc Subject: Re: [PATCH] Stale breakpoint instructions, spurious SIGTRAPS. References: <1397585886-29261-1-git-send-email-palves@redhat.com> In-Reply-To: <1397585886-29261-1-git-send-email-palves@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2014-04/txt/msg00310.txt.bz2 On 04/16/2014 02:18 AM, Pedro Alves wrote: > ppc_linux_memory_remove_breakpoint > does this unconditionally for all memory breakpoints, and questions > whether memory_remove_breakpoint should be changed to do this for all > breakpoints. Possibly yes, though I'm not certain, hence this > baby-steps patch. > > Comments? As it is said in the comments of ppc_linux_memory_remove_breakpoint, "more traffic is generated for the remote targets". I'd like not to "validate memory before writing shadow contents back" in memory_remove_breakpoint unless we have more evidence that it is useful to more targets or scenarios. This patch looks good to me, some comments below. > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index f777a4a..8c14bb1 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -2648,7 +2648,9 @@ insert_bp_location (struct bp_location *bl, > target doesn't define error codes), so we must treat > generic errors as memory errors. */ > if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR) > - && solib_name_from_address (bl->pspace, bl->address)) > + && (solib_name_from_address (bl->pspace, bl->address) > + || userloaded_objfile_contains_address_p (bl->pspace, > + bl->address))) > { > /* See also: disable_breakpoints_in_shlibs. */ > bl->shlib_disabled = 1; > @@ -3778,7 +3780,27 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is) > || !(section_is_overlay (bl->section))) > { > /* No overlay handling: just remove the breakpoint. */ > - val = bl->owner->ops->remove_location (bl); > + > + /* If we're trying to uninsert a memory breakpoint that we > + know is set in a dynamic object that is marked > + shlib_disabled, then either the dynamic object was > + removed with remove-symbol-file or with > + "nosharedlibrary". In the former case, we don't know > + whether another dynamic object might have loaded over the > + breakpoint's address -- the user might well let us know > + about it next with add-symbol-file (the whole point of > + OBJF_USERLOADED is letting the user manually maintain a > + list of dynamically loaded objects). Check if the > + breakpoint is still inserted in memory, to avoid > + overwriting wrong code with stale saved shadow contents. > + HW breakpoints are independent of poking at memory and > + must always be removed. */ > + if (bl->shlib_disabled > + && bl->target_info.shadow_len != 0 Does this condition mean "BL is software breakpoint location"? If so, we can comment this explicitly and move comment "HW breakpoints are ..." to this line above. Otherwise, people (at least I) may ask why don't check "bl->loc_type == bp_loc_software_breakpoint). > + && !memory_validate_breakpoint (bl->gdbarch, &bl->target_info)) > + val = 0; > + else > + val = bl->owner->ops->remove_location (bl); > } > else > { > @@ -3823,10 +3845,15 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is) > } > } > > - /* In some cases, we might not be able to remove a breakpoint > - in a shared library that has already been removed, but we > - have not yet processed the shlib unload event. */ > - if (val && solib_name_from_address (bl->pspace, bl->address)) > + /* In some cases, we might not be able to remove a breakpoint in > + a shared library that has already been removed, but we have > + not yet processed the shlib unload event. Similarly for an > + unloaded add-symbol-file object - the user might not yet have > + had the chance to remove-symbol-file it. */ > + if (val && (bl->shlib_disabled > + || solib_name_from_address (bl->pspace, bl->address) > + || userloaded_objfile_contains_address_p (bl->pspace, > + bl->address))) > val = 0; I can understand why userloaded_objfile_contains_address_p is added here, but check to "bl->shlib_disabled" isn't explained in the comments or changelog entry. > diff --git a/gdb/mem-break.c b/gdb/mem-break.c > index 1a057df..5371bd0 100644 > --- a/gdb/mem-break.c > +++ b/gdb/mem-break.c > @@ -89,3 +89,36 @@ memory_remove_breakpoint (struct target_ops *ops, struct gdbarch *gdbarch, > { > return gdbarch_memory_remove_breakpoint (gdbarch, bp_tgt); > } > + > +int > +memory_validate_breakpoint (struct gdbarch *gdbarch, > + struct bp_target_info *bp_tgt) > +{ > + CORE_ADDR addr = bp_tgt->placed_address; > + const gdb_byte *bp; > + int val; > + int bplen; > + gdb_byte old_contents[BREAKPOINT_MAX]; > + struct cleanup *cleanup; > + int ret; > + > + /* Determine appropriate breakpoint contents and size for this > + address. */ > + bp = gdbarch_breakpoint_from_pc (gdbarch, &addr, &bplen); > + > + if (bp == NULL > + || bp_tgt->placed_size != bplen) Nit: they may fit in one line? > + > + # Now delete the breakpoint from GDB's tables, to make sure > + # GDB doesn't reinsert it, masking the bug (with the bug, on > + # re-insert, GDB would fill the shadow buffer with a > + # breakpoint instruction). > + set test "delete" > + gdb_test_multiple $test $test { > + -re ".*y or n. $" { > + send_gdb "y\n" > + exp_continue > + } > + -re "$gdb_prompt $" { > + pass $test > + } > + } Use delete_breakpoints? or something like gdb_test "$test" "" "$test" "Delete all breakpoints.*y or n.*$" "y" > + > + # Re-add symbols back. > + set test "file \$binfile" > + gdb_test_multiple "file $binfile" $test { > + -re "Are you sure you want to change the file. .*y or n. $" { > + send_gdb "y\n" > + exp_continue > + } > + -re "Reading symbols from.*done.*$gdb_prompt $" { > + pass $test > + } > + } > + > + # Run to another function now. With the bug, GDB would trip > + # on a spurious trap at foo. > + gdb_test "b bar" ".*reakpoint .* at .*" > + gdb_test "continue" "Breakpoint .*, bar .*" > + } > +} > + > +# While it doesn't trigger the original bug this is a regression test > +# for, test with breakpoint always-inserted off for extra coverage. > +foreach always_inserted { "off" "on" } { > + foreach break_command { "break" "hbreak" } { > + test_break $always_inserted $break_command We need to check skip_hw_breakpoint_tests and don't pass "hbreak" to test_break if skip_hw_breakpoint_tests returns true. -- Yao (齐尧)