From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29369 invoked by alias); 29 Jun 2006 02:42:38 -0000 Received: (qmail 29329 invoked by uid 22791); 29 Jun 2006 02:42:38 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Thu, 29 Jun 2006 02:42:35 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1FvmU9-0008Va-Jl; Wed, 28 Jun 2006 22:42:29 -0400 Date: Thu, 29 Jun 2006 02:42:00 -0000 From: Daniel Jacobowitz To: Mark Kettenis Cc: ghost@cs.msu.su, gdb-patches@sources.redhat.com Subject: Re: [PATCH] Cleanup target memory reading Message-ID: <20060629024229.GB32465@nevyn.them.org> Mail-Followup-To: Mark Kettenis , ghost@cs.msu.su, gdb-patches@sources.redhat.com References: <200606281155.59166.ghost@cs.msu.su> <200606282130.k5SLUbwK011892@elgar.sibelius.xs4all.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200606282130.k5SLUbwK011892@elgar.sibelius.xs4all.nl> User-Agent: Mutt/1.5.11+cvs20060403 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-06/txt/msg00397.txt.bz2 On Wed, Jun 28, 2006 at 11:30:37PM +0200, Mark Kettenis wrote: > > From: Vladimir Prus > > Date: Wed, 28 Jun 2006 11:55:57 +0400 > > > > Hi, > > at the moment, pretty much every read of target memory goes via > > target_read_memory, and then via xfer_using_stratum. > > > > The only exception is the get_target_memory_unsigned function, which calls > > target_read function. It's the only use of 'target_read'. > > > > The attached patch removes target_read, and makes get_target_memory_unsigned > > use target_read_memory. > > > > OK? > > Shouldn't we "fix" target_read() such that it uses > target_read_partial() instead? /* Wrappers to perform the full transfer. */ LONGEST target_read (struct target_ops *ops, enum target_object object, const char *annex, gdb_byte *buf, ULONGEST offset, LONGEST len) { LONGEST xfered = 0; while (xfered < len) { LONGEST xfer = target_read_partial (ops, object, annex, (gdb_byte *) buf + xfered, offset + xfered, len - xfered); It already does. It's just not an operation that is often useful. We've got four, if you include my pending (pending rewrite, that is) patch: - target_read_partial. This is now an implementation detail. It reads some bytes. Who knows how many. - target_read. It reads a fixed, requested number of bytes. - xfer_using_stratum. Similar, but allow the request to be split and parts handled by different strata. This is always the right choice for memory, at least for consistency. - target_read_object_alloc, which is my current thinking of a name for what was target_read_whole in my patch. This is the basically always appropriate choice to fetch an "object" other than memory, since we don't have any way to find out what size it is otherwise, so we can't request a particular size. I don't want that last one to use the second one, because it's prone to doing this: - target_read_object_alloc - target_read 512 bytes - target_read_partial gets 500 - Second target_read_partial for 12 bytes - Another target_read And we'd much rather group them in larger batches than twelve byte reads. I think Vlad's basically garbage collecting target_read, since #3 and #4 from my list cover all the needs we've got right now. Does this seem reasonable? I think we've got too many ways to do this, and could do with fewer. I'd even suggest a pass over the other memory reading functions, pruning. For instance, the ones that take a frame. We know why they were added, and it's in theory a good plan to associate memory reads with frames, but it's not going to be completed any time in the foreseeable future. We've got lots of what the GCC folks called an incomplete transition when they were ranting about internal cleanliness. -- Daniel Jacobowitz CodeSourcery