From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2544 invoked by alias); 19 Oct 2013 04:38:33 -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 2530 invoked by uid 89); 19 Oct 2013 04:38:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.9 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 19 Oct 2013 04:38:32 +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 r9J4cTLQ018021 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 19 Oct 2013 00:38:30 -0400 Received: from psique (ovpn-113-80.phx2.redhat.com [10.3.113.80]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r9J4cQ8M001564 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Sat, 19 Oct 2013 00:38:28 -0400 From: Sergio Durigan Junior To: "Dave.Tian" Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] bug fix for gdb 16039 References: X-URL: http://www.redhat.com Date: Sat, 19 Oct 2013 04:38:00 -0000 In-Reply-To: (Dave Tian's message of "Wed, 16 Oct 2013 01:44:15 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2013-10/txt/msg00605.txt.bz2 On Wednesday, October 16 2013, Dave Tian wrote: > 1 Description: Gdb bug 16039 created by me > > Title: Gdb next" command stop working when shared library unloaded. > Root Cause: There is "libc++" static linked into the shared library, > and since gdb insert internal breakpoints on std::terminate/longjump/... > so after dlclose, the memory address is > invalid,remove_breakpoints/insert_breakpoints > failed with EIO error. > > Fix: Disable the internal breakpoints when dlclose hit. > > 2 ChangeLog: > > 2013-10-16 Tian Ye > > PR gdb/16039 > * breakpoint.c (is_removable_in_unloaded_shlib): New function. > (set_longjmp_breakpoint): Check if breakpoint's location > address is not in an unloaded shared library > (disable_breakpoints_in_unloaded_shlib): Disable the > internal breakpoints in the hook function of shared > library unload. Thanks for the patch. Just FYI: the formatting of this ChangeLog entry is wrong because it doesn't use TABs. However, the entry you posted on the previous version of this patch was right, so I think this was just a copy & paste done wrong :-). Anyway, just FYI as I said. I would also like to point that you still haven't contacted me about the copyright assignment. You need to do that paperwork before your patch can be committed (don't worry, it's just this time). Just send me an email offlist and I can send you the instructions. > 3 Patch Diffs: > > --- breakpoint.c.bak 2013-10-12 01:15:09.044081000 -0700 > +++ breakpoint.c 2013-10-15 23:10:50.241167000 -0700 > @@ -1118,6 +1118,23 @@ is_tracepoint (const struct breakpoint * > return is_tracepoint_type (b->type); > } > > +/* Return 1 if B is an internal memory breakpoint that > + * can be removed when shlib is unloaded, or 0 otherwise. */ > + > +static int > +is_removable_membreak_in_unloaded_shlib (const struct breakpoint *b) > +{ > + return (b->type == bp_longjmp > + || b->type == bp_longjmp_resume > + || b->type == bp_longjmp_call_dummy > + || b->type == bp_exception > + || b->type == bp_exception_resume > + || b->type == bp_std_terminate > + || b->type == bp_longjmp_master > + || b->type == bp_std_terminate_master > + || b->type == bp_exception_master); > +} Thanks for extending this function, but I still think it's incomplete. My rationale here is: I liked your previous idea of creating a function to identify whether a breakpoint is internal. I think it is more complete this way. However, such a function (IMO) should cover all the known possibilites of internal breakpoints. And your list is not exhaustive, for sure. So, IMO, you could take a closer at the possible internal breakpoints and properly extend this list. Also, when you do that, you could rename your function to "is_internal_breakpoint". Well, this is my opinion. I don't know what others think. But I certainly don't like this new function the way it is. The rest of the patch looks good to me, but you should also provide a testcase for it as Tom noted. Thanks a lot for doing that, -- Sergio