From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3314 invoked by alias); 11 Aug 2010 20:21:08 -0000 Received: (qmail 3294 invoked by uid 22791); 11 Aug 2010 20:21:04 -0000 X-SWARE-Spam-Status: No, hits=-6.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_XC,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 11 Aug 2010 20:20:58 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o7BKKu6u027413 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 11 Aug 2010 16:20:56 -0400 Received: from host1.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o7BKKsH0030097 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 11 Aug 2010 16:20:56 -0400 Received: from host1.dyn.jankratochvil.net (localhost [127.0.0.1]) by host1.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id o7BKKsCc016694; Wed, 11 Aug 2010 22:20:54 +0200 Received: (from jkratoch@localhost) by host1.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id o7BKKrKi016693; Wed, 11 Aug 2010 22:20:53 +0200 Date: Wed, 11 Aug 2010 20:21:00 -0000 From: Jan Kratochvil To: gdb-patches@sourceware.org Cc: Chris Moller Subject: [patch] Fix crash on conditional watchpoints (PR 11371) Message-ID: <20100811202053.GB16057@host1.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.20 (2009-12-10) X-IsSubscribed: yes 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: 2010-08/txt/msg00158.txt.bz2 Hi, on a watchpoint with condition using inferior function call GDB crashes. == Invalid read of size 8 == at 0x63949B: bpstat_stop_status (breakpoint.c:4138) == by 0x69C145: handle_inferior_event (infrun.c:3873) == by 0x6993C4: wait_for_inferior (infrun.c:2552) == by 0x698680: proceed (infrun.c:2065) == by 0x69144A: run_command_1 (infcmd.c:586) == by 0x691484: run_command (infcmd.c:596) == by 0x5F0934: do_cfunc (cli-decode.c:67) == by 0x5F39A2: cmd_func (cli-decode.c:1771) == by 0x48A81D: execute_command (top.c:422) == by 0x6AC9D4: catch_command_errors (exceptions.c:534) == by 0x4811E3: captured_main (main.c:887) == by 0x6AC931: catch_errors (exceptions.c:518) == by 0x48127E: gdb_main (main.c:919) == by 0x47FF15: main (gdb.c:34) == Address 0xcc727b0 is 16 bytes inside a block of size 184 free'd == at 0x4C25D72: free (vg_replace_malloc.c:325) == by 0x48E503: xfree (utils.c:1505) == by 0x63B9CA: free_bp_location (breakpoint.c:5402) == by 0x643B54: update_global_location_list (breakpoint.c:9428) == by 0x635AAD: insert_breakpoints (breakpoint.c:1867) == by 0x69845E: proceed (infrun.c:1985) == by 0x68F950: run_inferior_call (infcall.c:378) == by 0x6904AF: call_function_by_hand (infcall.c:804) == by 0x65BD6B: evaluate_subexp_standard (eval.c:1794) == by 0x73D1AA: evaluate_subexp_c (c-lang.c:1047) == by 0x65780A: evaluate_subexp (eval.c:76) == by 0x657A09: evaluate_expression (eval.c:167) == by 0x6382F8: breakpoint_cond_eval (breakpoint.c:3437) == by 0x6AC931: catch_errors (exceptions.c:518) == by 0x63902B: bpstat_check_breakpoint_conditions (breakpoint.c:3970) == by 0x63924B: bpstat_stop_status (breakpoint.c:4082) == by 0x69C145: handle_inferior_event (infrun.c:3873) == by 0x6993C4: wait_for_inferior (infrun.c:2552) == by 0x698680: proceed (infrun.c:2065) == by 0x69144A: run_command_1 (infcmd.c:586) == by 0x691484: run_command (infcmd.c:596) == by 0x5F0934: do_cfunc (cli-decode.c:67) == by 0x5F39A2: cmd_func (cli-decode.c:1771) == by 0x48A81D: execute_command (top.c:422) == by 0x6AC9D4: catch_command_errors (exceptions.c:534) It is due to bpstat_stop_status calling bpstat_check_breakpoint_conditions which calls update_global_location_list which rebuilds bp_location's which bpstat_stop_status does not expect. I tried first to fill in bpstat_stop_status the bpstat events already into ecs->event_thread->stop_bpstat so that free_bp_location would find it there and clear it appropriately. It does not work as in the time free_bp_location gets called ecs->event_thread->stop_bpstat is already pre-cleared and there is no way for free_bp_location to find the bpstat_what list stored only in the local variable in bpstat_stop_status. With the fix it just unfortunately does not stop when it should. This is due to all-stop mode always removing and re-creating watchpoint locations and bpstat_what() never stops on `bs->breakpoint_at == NULL'. Anyway non-stop mode should solve it as it no longer re-creates watchpoints each time. Unfortunately I could not verify it as non-stop fails for me in this case in some general way. I would turn the PR from a crashing one into a non-working one, there is a KFAIL in the testcase for it below. I believe the problem it does not stop is unrelated to this one. No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu. Thanks, Jan gdb/ 2010-08-11 Jan Kratochvil * breakpoint.c (bpstat_remove_bp_location): New forward declaration. (bpstat_pending, bpstat_pending_link): New variables. (bpstat_alloc): Remove parameter bs_link_pointer. Use bpstat_pending_link instead. (bpstat_stop_status_cleanup): New function. (bpstat_stop_status): Remove variable bs_link. New variable save_bpstat_pending_link. New variable back_to, wrap the function by it. Move the bp_hardware_watchpoint type check into the initial loop statement. Adjust the calls of bpstat_alloc, initialize bs_head explicitly there. Add extra check for NULL bs->breakpoint_at. (free_bp_location): Check also bpstat_pending. gdb/testsuite/ 2010-08-11 Jan Kratochvil * gdb.base/watch-cond-by-func.exp: New file. * gdb.base/watch-cond-by-func.c: New file. --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -207,6 +207,8 @@ static void update_global_location_list (int); static void update_global_location_list_nothrow (int); +static void bpstat_remove_bp_location (bpstat bps, struct bp_location *loc); + static int bpstat_remove_bp_location_callback (struct thread_info *th, void *data); @@ -3440,17 +3442,21 @@ breakpoint_cond_eval (void *exp) return i; } +/* List of bpstat's to be checked for stale information. */ + +static bpstat bpstat_pending, *bpstat_pending_link = &bpstat_pending; + /* Allocate a new bpstat. Link it to the FIFO list by BS_LINK_POINTER. */ static bpstat -bpstat_alloc (const struct bp_location *bl, bpstat **bs_link_pointer) +bpstat_alloc (const struct bp_location *bl) { bpstat bs; bs = (bpstat) xmalloc (sizeof (*bs)); bs->next = NULL; - **bs_link_pointer = bs; - *bs_link_pointer = &bs->next; + *bpstat_pending_link = bs; + bpstat_pending_link = &bs->next; bs->breakpoint_at = bl; /* If the condition is false, etc., don't do the commands. */ bs->commands = NULL; @@ -4002,6 +4008,17 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid) } } +/* Cleanup helper for bpstat_stop_status. */ + +static void +bpstat_stop_status_cleanup (void *arg) +{ + bpstat *link = arg; + + gdb_assert (*bpstat_pending_link == NULL); + bpstat_clear (link); + bpstat_pending_link = link; +} /* Get a bpstat associated with having just stopped at address BP_ADDR in thread PTID. @@ -4028,11 +4045,15 @@ bpstat_stop_status (struct address_space *aspace, struct bp_location *bl; struct bp_location *loc; /* First item of allocated bpstat's. */ - bpstat bs_head = NULL, *bs_link = &bs_head; + bpstat bs_head = NULL; /* Pointer to the last thing in the chain currently. */ bpstat bs; + bpstat *save_bpstat_pending_link = bpstat_pending_link; int ix; int need_remove_insert; + struct cleanup *back_to; + + back_to = make_cleanup (bpstat_stop_status_cleanup, bpstat_pending_link); /* ALL_BP_LOCATIONS iteration would break across update_global_location_list possibly executed by @@ -4043,16 +4064,15 @@ bpstat_stop_status (struct address_space *aspace, if (!breakpoint_enabled (b) && b->enable_state != bp_permanent) continue; - for (bl = b->loc; bl != NULL; bl = bl->next) + for (bl = b->loc; bl != NULL; + /* For hardware watchpoints, we look only at the first location. + The watchpoint_check function will work on the entire + expression, not the individual locations. For read watchpoints, + the watchpoints_triggered function has checked all locations + already. Also *BL can become no longer valid during the + bpstat_check_breakpoint_conditions call so be careful. */ + bl = b->type == bp_hardware_watchpoint ? NULL : bl->next) { - /* For hardware watchpoints, we look only at the first location. - The watchpoint_check function will work on the entire expression, - not the individual locations. For read watchpoints, the - watchpoints_triggered function has checked all locations - already. */ - if (b->type == bp_hardware_watchpoint && bl != b->loc) - break; - if (bl->shlib_disabled) continue; @@ -4061,7 +4081,9 @@ bpstat_stop_status (struct address_space *aspace, /* Come here if it's a watchpoint, or if the break address matches */ - bs = bpstat_alloc (bl, &bs_link); /* Alloc a bpstat to explain stop */ + bs = bpstat_alloc (bl); /* Alloc a bpstat to explain stop */ + if (bs_head == NULL) + bs_head = bs; /* Assume we stop. Should we find watchpoint that is not actually triggered, or if condition of breakpoint is false, we'll reset @@ -4119,7 +4141,10 @@ bpstat_stop_status (struct address_space *aspace, if (breakpoint_address_match (loc->pspace->aspace, loc->address, aspace, bp_addr)) { - bs = bpstat_alloc (loc, &bs_link); + bs = bpstat_alloc (loc); + if (bs_head == NULL) + bs_head = bs; + /* For hits of moribund locations, we should just proceed. */ bs->stop = 0; bs->print = 0; @@ -4135,6 +4160,7 @@ bpstat_stop_status (struct address_space *aspace, if (! bpstat_causes_stop (bs_head)) for (bs = bs_head; bs != NULL; bs = bs->next) if (!bs->stop + && bs->breakpoint_at && bs->breakpoint_at->owner && is_hardware_watchpoint (bs->breakpoint_at->owner)) { @@ -4148,6 +4174,13 @@ bpstat_stop_status (struct address_space *aspace, if (need_remove_insert) update_global_location_list (1); + /* Break the chain and make BS_HEAD independent. */ + discard_cleanups (back_to); + gdb_assert (*bpstat_pending_link == NULL); + bpstat_pending_link = save_bpstat_pending_link; + gdb_assert (*bpstat_pending_link == bs_head); + *bpstat_pending_link = NULL; + return bs_head; } @@ -5394,6 +5427,8 @@ static void free_bp_location (struct bp_location *loc) iterate_over_threads (bpstat_remove_bp_location_callback, loc); + bpstat_remove_bp_location (bpstat_pending, loc); + if (loc->cond) xfree (loc->cond); --- /dev/null +++ b/gdb/testsuite/gdb.base/watch-cond-by-func.c @@ -0,0 +1,33 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2010 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +volatile int var; + +int +return_1 (void) +{ + return 1; +} + +int +main(void) +{ + var++; + var++; /* watchpoint-stop */ + + return 0; /* break-at-exit */ +} --- /dev/null +++ b/gdb/testsuite/gdb.base/watch-cond-by-func.exp @@ -0,0 +1,43 @@ +# Copyright 2010 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +set testfile "watch-cond-by-func" +set srcfile ${testfile}.c +set binfile ${objdir}/${subdir}/${testfile} + +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } { + untested ${testfile}.exp + return -1 +} + +if ![runto_main] then { + fail "Can't run to main" + return +} + +gdb_test "watch var if return_1 ()" "atchpoint .*: var" + +gdb_breakpoint [gdb_get_line_number "break-at-exit"] + +set test "continue" +gdb_test_multiple $test $test { + -re "atchpoint \[0-9\]+: var\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*watchpoint-stop.*.*\r\n$gdb_prompt $" { + pass $test + } + -re "break-at-exit.*\r\n$gdb_prompt $" { + # It should have stopped. + kfail breakpoint/11371 $test + } +}