Message ID | 1466439050-11330-1-git-send-email-tom@tromey.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 115620 invoked by alias); 20 Jun 2016 16:11:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 113777 invoked by uid 89); 20 Jun 2016 16:11:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.7 required=5.0 tests=AWL, BAYES_00, FSL_HELO_HOME, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=no version=3.3.2 spammy=H*Ad:U*tom, Block, das X-HELO: gproxy8-pub.mail.unifiedlayer.com Received: from gproxy8-pub.mail.unifiedlayer.com (HELO gproxy8-pub.mail.unifiedlayer.com) (67.222.33.93) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with SMTP; Mon, 20 Jun 2016 16:11:02 +0000 Received: (qmail 24538 invoked by uid 0); 20 Jun 2016 16:11:00 -0000 Received: from unknown (HELO cmgw3) (10.0.90.84) by gproxy8.mail.unifiedlayer.com with SMTP; 20 Jun 2016 16:11:00 -0000 Received: from box522.bluehost.com ([74.220.219.122]) by cmgw3 with id 94Av1t0062f2jeq014Aybs; Mon, 20 Jun 2016 10:11:00 -0600 X-Authority-Analysis: v=2.1 cv=KpLehwmN c=1 sm=1 tr=0 a=GsOEXm/OWkKvwdLVJsfwcA==:117 a=GsOEXm/OWkKvwdLVJsfwcA==:17 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=PnD2wP_eR3oA:10 a=7XZj0uCbPdcA:10 a=pD_ry4oyNxEA:10 a=zstS-IiYAAAA:8 a=pGLkceISAAAA:8 a=vAjCVHPuAAAA:8 a=90XrTFU3PPuDedHrzfMA:9 a=4G6NA9xxw8l3yy4pmD5M:22 a=6kGIvZw6iX1k4Y-7sg4_:22 a=U9FomqI9nG4SDPFMYEq8:22 Received: from [75.171.172.174] (port=52328 helo=pokyo.Home) by box522.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.86_2) (envelope-from <tom@tromey.com>) id 1bF1mp-0006Oo-03; Mon, 20 Jun 2016 10:10:55 -0600 From: Tom Tromey <tom@tromey.com> To: gdb-patches@sourceware.org Cc: Tom Tromey <tom@tromey.com> Subject: [RFA] PR python/18565 - make Frame.function work for inline frames Date: Mon, 20 Jun 2016 10:10:50 -0600 Message-Id: <1466439050-11330-1-git-send-email-tom@tromey.com> X-Identified-User: {36111:box522.bluehost.com:elynrobi:tromey.com} {sentby:smtp auth 75.171.172.174 authed with tom+tromey.com} X-Exim-ID: 1bF1mp-0006Oo-03 X-Source-Sender: (pokyo.Home) [75.171.172.174]:52328 X-Source-Auth: tom+tromey.com X-Email-Count: 0 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTIyLmJsdWVob3N0LmNvbQ== |
Commit Message
Tom Tromey
June 20, 2016, 4:10 p.m. UTC
PR python/18565 notes that calling frame filters don't work properly for inlined functions. This happens because Frame.function on an inline frame will yield the wrong result. This patch changes this code to use find_frame_funname instead, which handles inline frames properly. Built and regtested on x86-64 Fedora 23. 2016-06-20 Tom Tromey <tom@tromey.com> PR python/18565: * python/py-frame.c (frapy_function): Use find_frame_funname. 2016-06-20 Tom Tromey <tom@tromey.com> PR python/18565: * gdb.python/py-frame-inline.exp: Add Frame.function test. --- gdb/ChangeLog | 5 +++++ gdb/python/py-frame.c | 6 +++++- gdb/testsuite/ChangeLog | 5 +++++ gdb/testsuite/gdb.python/py-frame-inline.exp | 4 ++++ 4 files changed, 19 insertions(+), 1 deletion(-)
Comments
Tom Tromey <tom@tromey.com> writes: > TRY > { > + char *funname; > + enum language funlang; > + > FRAPY_REQUIRE_VALID (self, frame); > > - sym = find_pc_function (get_frame_address_in_block (frame)); > + find_frame_funname (frame, &funname, &funlang, &sym); > + xfree (funname); > } > CATCH (except, RETURN_MASK_ALL) > { Call xfree in CATCH block? Otherwise, patch is good to me.
>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes: Yao> Tom Tromey <tom@tromey.com> writes: >> TRY >> { >> + char *funname; >> + enum language funlang; >> + >> FRAPY_REQUIRE_VALID (self, frame); >> >> - sym = find_pc_function (get_frame_address_in_block (frame)); >> + find_frame_funname (frame, &funname, &funlang, &sym); >> + xfree (funname); >> } >> CATCH (except, RETURN_MASK_ALL) >> { Yao> Call xfree in CATCH block? Otherwise, patch is good to me. I looked at this. I think it's probably better as-is. My reasoning is that "funname" is initialized by the call to find_frame_funname and isn't otherwise used. So, putting the free where it appears now means that there is no gap between initialization and free. On the other hand if it is moved into the catch, then there have to be two frees, one in the catch and one afterward. So if it's ok with you, I'm inclined to leave it as-is. Another option would be to change find_frame_funname to allow a NULL argument here. Then this free would not be needed at all. Let me know what you think. Tom
Sorry, I missed this mail, On Wed, Jun 22, 2016 at 7:42 PM, Tom Tromey <tom@tromey.com> wrote: >>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes: > > Yao> Tom Tromey <tom@tromey.com> writes: >>> TRY >>> { >>> + char *funname; >>> + enum language funlang; >>> + >>> FRAPY_REQUIRE_VALID (self, frame); >>> >>> - sym = find_pc_function (get_frame_address_in_block (frame)); >>> + find_frame_funname (frame, &funname, &funlang, &sym); >>> + xfree (funname); >>> } >>> CATCH (except, RETURN_MASK_ALL) >>> { > > Yao> Call xfree in CATCH block? Otherwise, patch is good to me. > > I looked at this. I think it's probably better as-is. > My reasoning is that "funname" is initialized by the call to > find_frame_funname and isn't otherwise used. So, putting the free where > it appears now means that there is no gap between initialization and > free. > The reason I suggested that way is that the exception may be thrown out in find_frame_funname after the memory is allocated for funname, so we need xfree in CATCH, and also need xfree afterwards.
On 07/25/2016 11:23 AM, Yao Qi wrote: > Sorry, I missed this mail, > > On Wed, Jun 22, 2016 at 7:42 PM, Tom Tromey <tom@tromey.com> wrote: >>>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes: >> >> Yao> Tom Tromey <tom@tromey.com> writes: >>>> TRY >>>> { >>>> + char *funname; >>>> + enum language funlang; >>>> + >>>> FRAPY_REQUIRE_VALID (self, frame); >>>> >>>> - sym = find_pc_function (get_frame_address_in_block (frame)); >>>> + find_frame_funname (frame, &funname, &funlang, &sym); >>>> + xfree (funname); >>>> } >>>> CATCH (except, RETURN_MASK_ALL) >>>> { >> >> Yao> Call xfree in CATCH block? Otherwise, patch is good to me. >> >> I looked at this. I think it's probably better as-is. >> My reasoning is that "funname" is initialized by the call to >> find_frame_funname and isn't otherwise used. So, putting the free where >> it appears now means that there is no gap between initialization and >> free. >> > > The reason I suggested that way is that the exception may be thrown out in > find_frame_funname after the memory is allocated for funname, so we need > xfree in CATCH, and also need xfree afterwards. I disagree. In general, I think that up until the called function does a normal return, the memory for output parameters is owned by the called function. A normal return then transfers ownership of the output parameters' memory to the caller. So I think that it's find_frame_funname that should be responsible for making sure that memory for output parameters is cleaned up on exception, or be written in a way that never throws after the memory allocation, which it may be already, but I haven't checked in detail. Thanks, Pedro Alves
Yao> The reason I suggested that way is that the exception may be thrown Yao> out in find_frame_funname after the memory is allocated for Yao> funname, so we need xfree in CATCH, and also need xfree afterwards. Pedro> I disagree. In general, I think that up until the called function does a normal Pedro> return, the memory for output parameters is owned by the called function. Pedro> A normal return then transfers ownership of the output parameters' memory Pedro> to the caller. Pedro> So I think that it's find_frame_funname that should be responsible for making Pedro> sure that memory for output parameters is cleaned up on exception, or be Pedro> written in a way that never throws after the memory allocation, which it may be Pedro> already, but I haven't checked in detail. I agree with this. I've just looked into the function and its callers. Most cases in find_frame_funname clearly set *funname in a spot where an exception cannot occur. The one iffy case is: *funname = xstrdup (SYMBOL_PRINT_NAME (func)); [...] if (*funlang == language_cplus) { /* It seems appropriate to use SYMBOL_PRINT_NAME() here, to display the demangled name that we already have stored in the symbol table, but we stored a version with DMGL_PARAMS turned on, and here we don't want to display parameters. So remove the parameters. */ char *func_only = cp_remove_params (*funname); I'm not 100% sure that cp_remove_params cannot throw. However, it's simple to deal with this by adding a cleanup in find_frame_funname. I'm happy to do this if desired. Another approach might be to have a free_current_contents cleanup at the start of find_frame_funname and discard it at the exit. This would maybe make it a bit safer in the face of future changes. Alternatively, if we need a try/catch in the caller to possibly free the function name, then several other callers are incorrect (ada-lang.c and stack.c). Tom
On 07/25/2016 04:01 PM, Tom Tromey wrote: > *funname = xstrdup (SYMBOL_PRINT_NAME (func)); > [...] > if (*funlang == language_cplus) > { > /* It seems appropriate to use SYMBOL_PRINT_NAME() here, > to display the demangled name that we already have > stored in the symbol table, but we stored a version > with DMGL_PARAMS turned on, and here we don't want to > display parameters. So remove the parameters. */ > char *func_only = cp_remove_params (*funname); > > I'm not 100% sure that cp_remove_params cannot throw. > However, it's > simple to deal with this by adding a cleanup in find_frame_funname. I'm > happy to do this if desired. > > Another approach might be to have a free_current_contents cleanup at the > start of find_frame_funname and discard it at the exit. This would > maybe make it a bit safer in the face of future changes. Yet another approach would be to push the xstrdup call to after the cp_remove_params call, and remove the xfree call, something like: if (*funlang == language_cplus) { char *func_only = cp_remove_params (SYMBOL_PRINT_NAME (func)); if (func_only) *funname = func_only; else *funname = xstrdup (SYMBOL_PRINT_NAME (func)); } else *funname = xstrdup (SYMBOL_PRINT_NAME (func)); } In any case, IMO this would be the subject of a separate patch. > Alternatively, if we need a try/catch in the caller to possibly free the > function name, then several other callers are incorrect (ada-lang.c and > stack.c). Yeah. I think that if a function has such a requirement, then it needs to be clearly documented as that being part of its API contract. Otherwise, it's too easy for the called function to change in a way that makes the caller try to free a dangling output pointer. Thanks, Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> Yet another approach would be to push the xstrdup call to after the
Pedro> cp_remove_params call, and remove the xfree call, something like:
Yeah.
Pedro> In any case, IMO this would be the subject of a separate patch.
Let me recommend accepting the original patch; and I will write a new
patch to fix this.
Tom
On 07/26/2016 02:18 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > Pedro> In any case, IMO this would be the subject of a separate patch. > > Let me recommend accepting the original patch; FAOD, the original patch LGTM. I think we should put it in too. > and I will write a new patch to fix this. I'd be super fine with just forgetting about it, but if you want to, please go ahead. Thanks, Pedro Alves
On Mon, Jul 25, 2016 at 12:04 PM, Pedro Alves <palves@redhat.com> wrote: >> >> The reason I suggested that way is that the exception may be thrown out in >> find_frame_funname after the memory is allocated for funname, so we need >> xfree in CATCH, and also need xfree afterwards. > > I disagree. In general, I think that up until the called function does a normal What do you disagree on? > return, the memory for output parameters is owned by the called function. > A normal return then transfers ownership of the output parameters' memory > to the caller. Yes, so we need xfree after find_frame_funname on normal return. That is what I suggested. We need to free the memory referenced by output parameter when exception is thrown too. The point in question is that who is responsible to free the memory referenced by output parameter. In Tom's patch, they are freed in the caller in normal return, so it is reasonable to free them in the caller in exception return as well, because it is not specified that find_frame_funname frees the memory on exception. > > So I think that it's find_frame_funname that should be responsible for making > sure that memory for output parameters is cleaned up on exception, or be > written in a way that never throws after the memory allocation, which it may be > already, but I haven't checked in detail. > If you think it is find_frame_funname's responsibility to free memory on exception, that is fine. We should document this behaviour for find_frame_funname and guarantee that find_frame_funname behaves that way. However, we are not sure current find_frame_funname behaves that way, because exception may be thrown in cp_remove_params.
On Tue, Jul 26, 2016 at 12:14 PM, Pedro Alves <palves@redhat.com> wrote: > On 07/25/2016 04:01 PM, Tom Tromey wrote: > >> *funname = xstrdup (SYMBOL_PRINT_NAME (func)); >> [...] >> if (*funlang == language_cplus) >> { >> /* It seems appropriate to use SYMBOL_PRINT_NAME() here, >> to display the demangled name that we already have >> stored in the symbol table, but we stored a version >> with DMGL_PARAMS turned on, and here we don't want to >> display parameters. So remove the parameters. */ >> char *func_only = cp_remove_params (*funname); >> >> I'm not 100% sure that cp_remove_params cannot throw. >> However, it's >> simple to deal with this by adding a cleanup in find_frame_funname. I'm >> happy to do this if desired. >> >> Another approach might be to have a free_current_contents cleanup at the >> start of find_frame_funname and discard it at the exit. This would >> maybe make it a bit safer in the face of future changes. > > Yet another approach would be to push the xstrdup call to after the > cp_remove_params call, and remove the xfree call, something like: > > if (*funlang == language_cplus) > { > char *func_only = cp_remove_params (SYMBOL_PRINT_NAME (func)); > > if (func_only) > *funname = func_only; > else > *funname = xstrdup (SYMBOL_PRINT_NAME (func)); > } > else > *funname = xstrdup (SYMBOL_PRINT_NAME (func)); > } > > > In any case, IMO this would be the subject of a separate patch. This is the prerequisite of Tom's patch, no? It guarantees that *FUNNAME is not allocated if exception is thrown, otherwise *FUNAME is allocated. Tom's patch relies on this behaviour. I am OK with Tom's original patch, but we need to fix find_frame_funname first.
On 08/03/2016 09:07 AM, Yao Qi wrote: > On Mon, Jul 25, 2016 at 12:04 PM, Pedro Alves <palves@redhat.com> wrote: >>> >>> The reason I suggested that way is that the exception may be thrown out in >>> find_frame_funname after the memory is allocated for funname, so we need >>> xfree in CATCH, and also need xfree afterwards. >> >> I disagree. In general, I think that up until the called function does a normal > > What do you disagree on? That it's the caller's responsibility to free an output parameter of a called function that throws. Or more generally, that the state of an output parameter as observed in the caller is determinate when the callee throws. > >> return, the memory for output parameters is owned by the called function. >> A normal return then transfers ownership of the output parameters' memory >> to the caller. > > Yes, so we need xfree after find_frame_funname on normal return. That's what Tromey's patch does. > That is what I suggested. You suggested to free it _also_ when the exception is thrown. That's where my disagreement lies. > > We need to free the memory referenced by output parameter when exception > is thrown too. This. > The point in question is that who is responsible to free the > memory referenced by output parameter. Right. > In Tom's patch, they are freed in > the caller in normal return, so it is reasonable to free them in the caller in > exception return as well, because it is not specified that find_frame_funname > frees the memory on exception. I don't think it needs to be explicitly specified, because I think it should be the behavior or any function that has output parameters. It's unsafe otherwise, because when an exception is thrown from inside a callee, the caller has no idea whether the output parameter has been definitely assigned to. - the callee might throw an exception before the output parameter pointer is ever written to. - the output parameter pointer may have been initialized but now be dangling at the point the exception is thrown inside callee - the callee freed it before throwing. So the exception path (usually the cleanup) in the caller could try to use a dangling pointer (or even a partially constructed object). Basically, this, where foo returns through an output param: extern void foo (char **ret); char *ret; old_chain = make_cleanup (xfree, ret); foo (&ret); do_cleanups (old_chain); ... is as broken as this obviously broken one, which is the exact same except that it returns through normal return: extern char *foo (void); char *ret; old_chain = make_cleanup (xfree, ret); ret = foo (); do_cleanups (old_chain); > >> >> So I think that it's find_frame_funname that should be responsible for making >> sure that memory for output parameters is cleaned up on exception, or be >> written in a way that never throws after the memory allocation, which it may be >> already, but I haven't checked in detail. >> > > If you think it is find_frame_funname's responsibility to free memory on > exception, that is fine. We should document this behaviour for > find_frame_funname and guarantee that find_frame_funname behaves > that way. However, we are not sure current find_frame_funname behaves that > way, because exception may be thrown in cp_remove_params. IMO that becomes an unrelated, preexisting problem. I don't think we should require that all the functions (and their callees, transitively) called by all patches are first inspected for leaks and fixed. Thanks, Pedro Alves
On Wed, Aug 3, 2016 at 12:35 PM, Pedro Alves <palves@redhat.com> wrote: > > IMO that becomes an unrelated, preexisting problem. I don't think we should > require that all the functions (and their callees, transitively) called by all > patches are first inspected for leaks and fixed. I don't intend to inspect and fix all leaks too. I am OK with Tom's patch.
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index faa9e9f..8c327c4 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2016-06-20 Tom Tromey <tom@tromey.com> + + PR python/18565: + * python/py-frame.c (frapy_function): Use find_frame_funname. + 2016-06-17 Yan-Ting Lin <currygt52@gmail.com> * Makefile.in (ALL_TARGET_OBS): Add nds32-tdep.o. diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c index 98a7d7b..6bdac08 100644 --- a/gdb/python/py-frame.c +++ b/gdb/python/py-frame.c @@ -340,9 +340,13 @@ frapy_function (PyObject *self, PyObject *args) TRY { + char *funname; + enum language funlang; + FRAPY_REQUIRE_VALID (self, frame); - sym = find_pc_function (get_frame_address_in_block (frame)); + find_frame_funname (frame, &funname, &funlang, &sym); + xfree (funname); } CATCH (except, RETURN_MASK_ALL) { diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index e5f1c54..785b2cb 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2016-06-20 Tom Tromey <tom@tromey.com> + + PR python/18565: + * gdb.python/py-frame-inline.exp: Add Frame.function test. + 2016-06-17 Sanjoy Das <sanjoy@playingwithpointers.com> * gdb.base/jit-reader.exp: New file. diff --git a/gdb/testsuite/gdb.python/py-frame-inline.exp b/gdb/testsuite/gdb.python/py-frame-inline.exp index 6306c8e..1372ee0 100644 --- a/gdb/testsuite/gdb.python/py-frame-inline.exp +++ b/gdb/testsuite/gdb.python/py-frame-inline.exp @@ -51,3 +51,7 @@ gdb_test_no_output "set backtrace limit 1" gdb_continue_to_breakpoint "Block break here." gdb_test "python print (gdb.newest_frame())" ".*" + +# Regression test to verify that Frame.function works properly for +# inline frames. +gdb_test "python print (gdb.newest_frame().function())" "f"