From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2386 invoked by alias); 10 Jun 2010 18:40:53 -0000 Received: (qmail 2374 invoked by uid 22791); 10 Jun 2010 18:40:51 -0000 X-SWARE-Spam-Status: No, hits=-5.1 required=5.0 tests=AWL,BAYES_05,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,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; Thu, 10 Jun 2010 18:40:46 +0000 Received: from int-mx03.intmail.prod.int.phx2.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5AIeiRh007787 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 10 Jun 2010 14:40:44 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx03.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5AIehhx005130; Thu, 10 Jun 2010 14:40:43 -0400 Received: from opsy.redhat.com (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 o5AIegKo024509; Thu, 10 Jun 2010 14:40:42 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 20D063782DC; Thu, 10 Jun 2010 12:40:41 -0600 (MDT) From: Tom Tromey To: Phil Muldoon Cc: gdb-patches ml Subject: Re: [python][patch] Inferior and Thread information support. References: <4BFA6E82.3070704@redhat.com> Reply-To: tromey@redhat.com Date: Thu, 10 Jun 2010 18:40:00 -0000 In-Reply-To: <4BFA6E82.3070704@redhat.com> (Phil Muldoon's message of "Mon, 24 May 2010 13:18:10 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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-06/txt/msg00248.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> This patch adds Python support to inferiors and threads. Sorry for the delay in this review. Phil> /* Copied from bfd_put_bits. */ Phil> -static void Phil> +void Phil> put_bits (bfd_uint64_t data, char *buf, int bits, bfd_boolean big_p) I don't think you need this change. Use store_unsigned_integer instead. I don't understand why this function exists at all. Phil> +void Phil> +allocate_pattern_buffer (char **pattern_buf, char **pattern_buf_end, Phil> + ULONGEST *pattern_buf_size) Phil> +void Phil> +increase_pattern_buffer (char **pattern_buf, char **pattern_buf_end, Phil> + ULONGEST *pattern_buf_size, int val_bytes) I think this stuff can be easily done with obstacks. I prefer we not add more growable types in situations where we can reuse the ones we already have. Phil> + /* While creating new inferior no inferior thread is available. Phil> + Therefore get_current_arch has no valid current frame (and it Phil> + would crash). */ Phil> + cleanup = ensure_python_env (target_gdbarch, current_language); You should use python_gdbarch and python_language here. This occurs a couple of times. Phil> +/* An observer callback function that is called when an inferior has Phil> + been deleted. Removes the corresponding Python object from the Phil> + inferior list, and removes the list's reference to the object. */ Phil> +static void Phil> +delete_inferior_object (struct inferior *inf) Phil> +{ Phil> + struct cleanup *cleanup; Phil> + struct inflist_entry **inf_entry, *inf_tmp; Phil> + struct threadlist_entry *th_entry, *th_tmp; Phil> + Phil> + /* Find inferior_object for the given PID. */ Phil> + for (inf_entry = &gdbpy_inferior_list; *inf_entry != NULL; Phil> + inf_entry = &(*inf_entry)->next) Phil> + if ((*inf_entry)->inf_obj->inferior->pid == inf->pid) Phil> + break; It seems strange to compare the pid fields when we could just compare the inferior objects themselves. Phil> +/* Implementation of Inferior.frames () -> (gdb.Frame, ...). Phil> + Returns a tuple of all frame objects. */ Phil> +PyObject * Phil> +thpy_frames (PyObject *self, PyObject *args) [...] Phil> + for (frame = get_current_frame (); frame; Phil> + frame = get_prev_frame (frame)) Phil> + { Phil> + frame_obj = frame_info_to_frame_object (frame); Phil> + if (frame_obj == NULL) Phil> + { I don't think this is wise. It is not uncommon for a crash to cause a thread to have thousands of frames. Hm, maybe there is no way to return a frame-in-a-thread and then be able to iterate. IOW, a gdb internals limitation. That is unfortunate (if true) but I don't think it is a reason for us to go with this API. One short-term solution would be to get rid of this method. Phil> +/* Implementation of InferiorThread.newest_frame () -> gdb.Frame. Phil> + Returns the newest frame object. */ Phil> +PyObject * Phil> +thpy_newest_frame (PyObject *self, PyObject *args) Phil> +{ Phil> + struct frame_info *frame; Phil> + PyObject *frame_obj = NULL; /* Initialize to appease gcc warning. */ Phil> + thread_object *thread_obj = (thread_object *) self; Phil> + struct cleanup *cleanup; Phil> + volatile struct gdb_exception except; Phil> + Phil> + THPY_REQUIRE_VALID (thread_obj); Phil> + Phil> + cleanup = make_cleanup_restore_current_thread (); Phil> + Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + switch_to_thread (thread_obj->thread->ptid); Phil> + Phil> + frame = get_current_frame (); Phil> + frame_obj = frame_info_to_frame_object (frame); Phil> + } Phil> + GDB_PY_HANDLE_EXCEPTION (except); I am really not sure about this. Doesn't switch_to_thread reset the frame cache? Meaning that the returned frame_obj will immediately be invalid? You would have to try this with a multi-threaded program, where you are stopped in thread A but then request a frame in thread B. Phil> +void Phil> +gdbpy_initialize_thread (void) Phil> +{ Phil> + Phil> + if (PyType_Ready (&thread_object_type) < 0) Spurious blank line. Tom