From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9512 invoked by alias); 14 Jun 2010 12:42:55 -0000 Received: (qmail 9500 invoked by uid 22791); 14 Jun 2010 12:42:53 -0000 X-SWARE-Spam-Status: No, hits=-5.2 required=5.0 tests=AWL,BAYES_00,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; Mon, 14 Jun 2010 12:42:45 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5ECghx2007848 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 14 Jun 2010 08:42:43 -0400 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5ECggnw002584; Mon, 14 Jun 2010 08:42:42 -0400 Message-ID: <4C1623C1.6090205@redhat.com> Date: Mon, 14 Jun 2010 12:42:00 -0000 From: Phil Muldoon User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100430 Fedora/3.0.4-2.fc12 Lightning/1.0b2pre Thunderbird/3.0.4 MIME-Version: 1.0 To: tromey@redhat.com CC: gdb-patches ml Subject: Re: [python][patch] Inferior and Thread information support. References: <4BFA6E82.3070704@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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-06/txt/msg00306.txt.bz2 On 06/10/2010 07:40 PM, Tom Tromey wrote: >>>>>> "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. Me either ... > > 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. I've no complaint to using obstacks. This function basically wraps/tidies the existing code that was just coded directly in a loop in parse_find_args. That code just realloc'd by a factor of two whenever the buffer was too small. This code is exactly the same, except it has been squirrelled away in a function. So we are not introducing or adding any more growable types in this patch, just moving the code bits that already existed into function. I'm not adverse to changing that code to use obstacks, that being said! > > 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. Ok, missed this! > > 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. Do you mean using the Python object's cmp inbuilt method here? > > 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. Ok. > > 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. I'll investigate this further. I had concerns about this (I think we chatted a little about this on irc way back when). I'll write an inferior test and see. Maybe Pedro or someone else in that area knows a little more. Cheers Phil