From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32596 invoked by alias); 17 Nov 2011 17:48:50 -0000 Received: (qmail 32547 invoked by uid 22791); 17 Nov 2011 17:48:44 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SINGLE_HEADER_1K,SPF_HELO_PASS,TW_BP 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; Thu, 17 Nov 2011 17:48:28 +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.14.4/8.14.4) with ESMTP id pAHHmQ5n020510 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 17 Nov 2011 12:48:26 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id pAHHmQ5X029509; Thu, 17 Nov 2011 12:48:26 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id pAHHmOQ6023754; Thu, 17 Nov 2011 12:48:25 -0500 From: Tom Tromey To: Kevin Pouget Cc: gdb-patches@sourceware.org, pmuldoon@redhat.com Subject: Re: [RFC] Python Finish Breakpoints References: Date: Thu, 17 Nov 2011 17:48:00 -0000 In-Reply-To: (Kevin Pouget's message of "Wed, 9 Nov 2011 15:09:32 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.90 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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: 2011-11/txt/msg00477.txt.bz2 >>>>> "Kevin" == Kevin Pouget writes: Kevin> py-breakpoint.c::gdbpy_should_stop is executed _before_ we have the Kevin> chance to save the stop_registers, used to compute the return value Kevin> (in infrun.c::normal_stop). Kevin> (the problem existed since the beginning, but I never faced it before) Kevin> I've updated the function infcmd.c::get_return_value to take this Kevin> situation into account, using the current registers if the Kevin> 'stop_registers' are not set, based on what is done in Kevin> infrun.c::normal_stop: This approach seems reasonable to me but I am not sure whether or not it is really ok. Most times I've tried to change infrun, I've been burned. Kevin> I think that I handle that in the following lines: Kevin> + if (except.reason < 0 Kevin> + || !self_bpfinish->return_type || !self_bpfinish->function_type) The problem is that Python errors are sticky. So, if one occurs one must either pass it upward (having the current function fail), or clear it somehow. It isn't ok to ignore them, call other Python functions, and then later check. Kevin> I think I wrote a word about that in the previous mail, anyway, my Kevin> feeling was that I don't want to abort the FinishBreakpoint creation Kevin> just because of a return value not computable, so I currently nullify Kevin> these fields and silently disable return value computation. I can't Kevin> see any straightforward way to notify Python about that, Kevin> warning/exceptions won't suite ... otherwise, I could expose the Kevin> return_type to the Python interface, this way, one could check that Kevin> it's not None and now if GDB will/might be able to compute the return Kevin> value when the FinishBP is hit Ok, this makes sense to me. Tom> bpfinishpy_detect_out_scope_cb still acquires the GIL (via Tom> ensure_python_env), but should not. Kevin> I'm not exactly sure what was you concern here, as far as I Kevin> understand, the GIL was acquired in bpfinishpy_handle_stop, so it Kevin> should have no effect in detect_out_scope_cb. So I've removed it from Kevin> detect_out_scope_cb as it was useless. I think it is inefficient to recursively acquire the GIL. Kevin> +@defun FinishBreakpoint.__init__ (@r{[}frame@r{]} @r{[}, internal@r{]}) Kevin> +Create a finish breakpoint at the return address of the @code{gdb.Frame} Kevin> +object @var{frame} (or @code{gdb.selected_frame()} is not provided). The I think it should read something like: Create a finish breakpoint at the return address of the @code{gdb.Frame} object @var{frame}. If @var{frame} is not provided, this defaults to the newest frame. I think it is better to default to the newest frame, because the selected frame is more of a user-interface thing, and I think code wanting this should explicitly use it. Kevin> +type is @code{VOID} I think just @code{void} is better. Kevin> + /* If stop_registers where not saved, use the current registers. */ s/where/were/ Kevin> + if (cleanup) Kevin> + do_cleanups (cleanup); This is a gdb anti-pattern. A call to make_cleanup can return NULL in some scenarios. It is better to install a null cleanup and then unconditionally call do_cleanups. Kevin> + /* Default to gdb.selected_frame if necessary. */ Kevin> + if (!frame_obj) Kevin> + frame_obj = gdbpy_selected_frame (NULL, NULL); gdbpy_newest_frame I don't think there is a decref for the frame_obj along this path. Kevin> + PyErr_SetString (PyExc_ValueError, Kevin> + _("\"FinishBreakpoint\" not meaningful in the outermost frame.")); Indentation. Kevin> + PyErr_SetString (PyExc_ValueError, Kevin> + _("\"FinishBreakpoint\" cannot be set on a dummy frame.")); Indentation. Kevin> + PyErr_SetString (PyExc_ValueError, Kevin> + _("Invalid ID for the `frame' object.")); Indentation. Kevin> + /* Remember only non-VOID return types. */ Kevin> + if (TYPE_CODE (ret_type) != TYPE_CODE_VOID) Kevin> + { Kevin> + self_bpfinish->return_type = type_to_type_object (ret_type); Kevin> + self_bpfinish->function_type = Kevin> + type_to_type_object (SYMBOL_TYPE (function)); As discussed above, you need to check for errors immediately after each call, and DTRT. You can ignore them with PyErr_Clear. Kevin> +static void Kevin> +bpfinishpy_out_of_scope (struct finish_breakpoint_object *bpfinish_obj) Kevin> +{ [...] Kevin> + TRY_CATCH (except, RETURN_MASK_ALL) Kevin> + { Kevin> + delete_breakpoint (bpfinish_obj->py_bp.bp); Kevin> + } Kevin> + if (except.reason < 0) Kevin> + { Kevin> + gdbpy_convert_exception (except); Kevin> + gdbpy_print_stack (); Kevin> + } I probably asked you to add this, but if bpfinishpy_out_of_scope can only be called in one spot: Kevin> + TRY_CATCH (except, RETURN_MASK_ALL) Kevin> + { Kevin> + if (b->pspace == current_inferior ()->pspace Kevin> + && (!target_has_registers Kevin> + || frame_find_by_id (b->frame_id) == NULL)) Kevin> + { Kevin> + bpfinishpy_out_of_scope (finish_bp); Kevin> + } Kevin> + } Kevin> + if (except.reason < 0) Kevin> + { Kevin> + gdbpy_convert_exception (except); Kevin> + gdbpy_print_stack (); Kevin> + } ... then the TRY_CATCH in bpfinishpy_out_of_scope is not needed. Kevin> + struct cleanup *cleanup = ensure_python_env (target_gdbarch, Kevin> + current_language); Indentation. Tom