From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29503 invoked by alias); 7 Jul 2010 16:30:07 -0000 Received: (qmail 29490 invoked by uid 22791); 7 Jul 2010 16:30:04 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 07 Jul 2010 16:30:00 +0000 Received: (qmail 6461 invoked from network); 7 Jul 2010 16:29:58 -0000 Received: from unknown (HELO caradoc.them.org) (dan@127.0.0.2) by mail.codesourcery.com with ESMTPA; 7 Jul 2010 16:29:58 -0000 Date: Wed, 07 Jul 2010 16:30:00 -0000 From: Daniel Jacobowitz To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: Better MI memory commands Message-ID: <20100707162952.GA6530@caradoc.them.org> Mail-Followup-To: Vladimir Prus , gdb-patches@sources.redhat.com References: <201006251232.55281.vladimir@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201006251232.55281.vladimir@codesourcery.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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-07/txt/msg00114.txt.bz2 I have no further comments on the documentation, but Eli had several. On Fri, Jun 25, 2010 at 12:32:55PM +0400, Vladimir Prus wrote: > - The current output of -data-read-memory is totally insane. This patch > introduces -data-read-memory-raw that clearly reports what regions were > read, and shows the data as hex string. I've also added -data-write-memory-raw > which, contrary to -data-write-memory can actually write *memory*, as opposed to > a single byte. I think that "raw" is not very expressive - it is too much like "new" in that you don't know later what is newer than what. The main change to -data-read-memory is to return a string of byte data, instead of the word-oriented table. So how about -data-read-bytes? > + if (argc != 2) > + error ("Usage: ADDR LENGTH."); Missing [-o BYTE-OFFSET] in the message. > + /* Loop invariant is that the [current_begin, current_end) was previously > + found to be not readable as a whole. > + > + Note loop condition -- if the range has 1 byte, we can't divide the range > + so there's no point trying further. */ > + while (current_end - current_begin > 1) > + { > + ULONGEST first_half_begin, first_half_end; > + ULONGEST second_half_begin, second_half_end; > + LONGEST xfer; > + > + ULONGEST middle = current_begin + (current_end - current_begin)/2; > + if (forward) > + { > + first_half_begin = current_begin; > + first_half_end = middle; > + second_half_begin = middle; > + second_half_end = current_end; > + } > + else > + { > + first_half_begin = middle; > + first_half_end = current_end; > + second_half_begin = current_begin; > + second_half_end = middle; > + } > + > + xfer = target_read (ops, TARGET_OBJECT_MEMORY, NULL, > + buf + (first_half_begin - begin), > + first_half_begin, > + first_half_end - first_half_begin); > + > + if (xfer == first_half_end - first_half_begin) > + { > + /* This half reads up fine. So, the error must be in the other half. */ > + current_begin = second_half_begin; > + current_end = second_half_end; > + } > + else > + { > + current_begin = first_half_begin; > + current_end = first_half_end; > + } > + } If we take the first branch of this if statement, then we've read the "fine" half of the search space into BUF. We search the "bad" half for the error. If we take the second branch of the if statement, then we narrow the search space to the first half. But we didn't read the second half. Won't we return uninitialized data in the buffer? I think I see now. The assumption is a single valid/invalid transition in the entire range. So we assume that in the else block, the whole second half won't be readable. Right? If so, this is OK, but I'd appreciate a comment to that effect. > - if (xfer == 0) > - return xfered; > - if (xfer < 0) > - { > - remaining = half; > - } > - else > - { > - /* We have successfully read the first half. So, the > - error must be in the second half. Adjust start and > - remaining to point at the second half. */ > - xfered += xfer; > - start += xfer; > - remaining -= xfer; > - } > - half = remaining/2; > + xfree (buffer); > + xfered += to_read; > } Why do we skip to_read bytes if we succeed at reading zero bytes? For that matter, what does a return value of zero mean? It seems like this would mean the same as -1. > - > - return xfered; > + else if (xfer < 0) > + { > + /* Got an error reading full chunk. See if maybe we can read > + some subrange. */ > + xfree (buffer); > + read_whatever_is_readable (ops, offset + xfered, offset + xfered + to_read, &result); > + xfered += to_read; > + } > + else > + { > + struct memory_read_result r; > + r.data = buffer; > + r.begin = offset + xfered; > + r.end = r.begin + xfer; > + VEC_safe_push (memory_read_result_s, result, &r); > + xfered += xfer; > + } A single consecutive block of memory may appear as more than one memory_read_result (and thus more than one MI record). Is this what we want? If so, please mention it in the documentation. > diff --git a/gdb/target.h b/gdb/target.h > index 870a1eb..66e1166 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > +extern VEC(memory_read_result_s)* > +read_memory_robust (struct target_ops *ops, ULONGEST offset, LONGEST len); Formatting: function name doesn't go in column 1 for the prototype. -- Daniel Jacobowitz CodeSourcery