From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23500 invoked by alias); 29 Feb 2008 02:44:29 -0000 Received: (qmail 23491 invoked by uid 22791); 29 Feb 2008 02:44:27 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.45.13) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 29 Feb 2008 02:44:04 +0000 Received: from zps38.corp.google.com (zps38.corp.google.com [172.25.146.38]) by smtp-out.google.com with ESMTP id m1T2i2pd017188 for ; Thu, 28 Feb 2008 18:44:02 -0800 Received: from gv-out-0910.google.com (gves4.prod.google.com [10.16.170.4]) by zps38.corp.google.com with ESMTP id m1T2hvIl027097 for ; Thu, 28 Feb 2008 18:44:01 -0800 Received: by gv-out-0910.google.com with SMTP id s4so1545429gve.37 for ; Thu, 28 Feb 2008 18:44:01 -0800 (PST) Received: by 10.115.108.1 with SMTP id k1mr10617007wam.42.1204253040768; Thu, 28 Feb 2008 18:44:00 -0800 (PST) Received: by 10.115.107.7 with HTTP; Thu, 28 Feb 2008 18:44:00 -0800 (PST) Message-ID: Date: Fri, 29 Feb 2008 03:13:00 -0000 From: "Doug Evans" To: gdb-patches@sourceware.org Subject: Re: [RFA] new command to search memory In-Reply-To: <20080226022335.GB4456@caradoc.them.org> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20080214021915.F3FE51C72F0@localhost> <20080226022335.GB4456@caradoc.them.org> 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: 2008-02/txt/msg00489.txt.bz2 Thanks for the review. On Mon, Feb 25, 2008 at 6:23 PM, Daniel Jacobowitz wrote: > > The find command looks like: > > find [/[s][n]] START_ADDR, [@LEN | END_ADDR], VAL [, VAL]... > > Stopping values at a comma is not much like other GDB commands. > On the other hand, there's priort art (the printf command), and > there's parser support (parse_to_comma_and_eval, which you used > and I totally did not know was there before today), and the parsing is > reasonably intuitive. So we've got commas. There aren't many commands that take multiple expressions as arguments. One needs a useful way to separate them. > The slashed arguments work analagously to x and display, which is > nice. Should the default count should be one instead of infinity? > I suppose having it default to infinity is nice, since we don't > have to invent a syntax for infinity that way. /u or some such for "unlimited" would work I guess. I can change the default if that's preferable. > What do you think of "+" instead of "@" to distinguish lengths? "find > &hello[0] +0x100". I picked "@" because it's used, for example, in "p foo[0]@10". It's not identical, but it seemed similar enough. "+" works too. > And for the search string, I guess the main reason that you didn't > use the normal language parsers was to avoid the malloc call: > > > > + /* If we see a string, parse it ourselves rather than the normal > > + handling of downloading it to target memory. */ Ya, but for completeness sake it's not just the malloc call, it's the whole shebang. I can understand why in (gdb) p strcmp (foo, "bar") one wants to download "bar" to the target before calling strcmp, but find's needs are different. > Can we use the language parsers, for consistency with other GDB > commands, if we fix this? Which I happen to have a patch for. I'll > dust it off and post it. Then, someday, we can get wchar_t strings > here for free (I hope). Sure. > I noticed that the current command is implemented all in one function, > which goes from a CLI string to output. I'm sure we'll want a GDB/MI > command for this, so it would be helpful if it was broken out into > two functions. OTOH that's easy to do later so don't worry about it, > let's get it in the CLI first. ok. > qSearch:memory does not need to be advertised for qSupported. The > rule of thumb is that things which are used to implement a user > command don't need to be, since there's no big penalty if we try them > and are told they are not supported - we'll just try another approach > and next time we'll know. That means you need to handle > PACKET_DISABLE twice, before and after sending the packet. I found a use for the option that goes with qSupported both for testing and analysis. Maybe users would also find use for the choice, but it can be tossed. > > @@ -464,6 +475,7 @@ update_current_target (void) > > INHERIT (to_make_corefile_notes, t); > > INHERIT (to_get_thread_local_address, t); > > /* Do not inherit to_read_description. */ > > + INHERIT (to_search_memory, t); > > INHERIT (to_magic, t); > > /* Do not inherit to_memory_map. */ > > /* Do not inherit to_flash_erase. */ > > I suggest doing this as a do-not-inherit, like the other new methods. > Maybe someday we'll eliminate the older style entirely. ok. > > @@ -1723,6 +1737,139 @@ target_read_description (struct target_o > > return NULL; > > } > > > > +/* Utility to implement a basic search of memory. */ > > + > > +int > > +simple_search_memory (struct target_ops* ops, > > "struct target_ops *ops" > > > + CORE_ADDR start_addr, ULONGEST search_space_len, > > + const gdb_byte *pattern, ULONGEST pattern_len, > > + CORE_ADDR *found_addrp) > > +{ > > + /* ??? tunable? > > + NOTE: also defined in find.c testcase. */ > > +#define SEARCH_CHUNK_SIZE 16000 > > I don't think it needs to be tunable. The value doesn't matter much, > since you truncate to the size of the search space. ok. > > +/* The default implementation of to_search_memory. */ > > + > > +static int > > +default_search_memory (CORE_ADDR start_addr, ULONGEST search_space_len, > > + const gdb_byte *pattern, ULONGEST pattern_len, > > + CORE_ADDR *found_addrp) > > +{ > > + return simple_search_memory (¤t_target, start_addr, search_space_len, > > + pattern, pattern_len, found_addrp); > > +} > > Please pass target_ops to to_search_memory. That lets you combine > default_search_memory and simple_search_memory, and it also lets > targets get at their own state (e.g. if the remote target needs to > know whether it's an extended target or not). ok. > > + > > +/* Search SEARCH_SPACE_LEN bytes beginning at START_ADDR for the > > + sequence of bytes in PATTERN with length PATTERN_LEN. > > + > > + The result is 1 if found, 0 if not found, and -1 if there was an error > > + requiring halting of the search (e.g. memory read error). > > + If the pattern is found the address is recorded in FOUND_ADDRP. > > + > > + NOTE: May wish to give target ability to maintain state across successive > > + calls within one search request. Left for later. */ > > Isn't there only one call per search request now? I don't think this > is necessary. ok. > > +If the value size is not specified, it is taken from the > > +value's type. This is useful when one wants to specify the search > > +pattern as a mixture of types. > > IMO this will confuse users for constants, which have type int (or > sometimes long), so could you add a word about that? Otherwise > someone will type "find &hello[0], @100, 0x65, 0x66" and be confused > by the lack of matches. Or one could default to something else, bytes or ints or some such, and have a /t option or some such that says to use the type of the object. > > Index: gdb/gdbserver/server.c > > =================================================================== > > RCS file: /cvs/src/src/gdb/gdbserver/server.c,v > > retrieving revision 1.63 > > diff -u -p -u -p -r1.63 server.c > > --- gdb/gdbserver/server.c 30 Jan 2008 00:51:50 -0000 1.63 > > +++ gdb/gdbserver/server.c 14 Feb 2008 02:03:43 -0000 > > @@ -270,6 +270,157 @@ monitor_show_help (void) > > monitor_output (" Enable remote protocol debugging messages\n"); > > } > > > > +/* Subroutine of handle_search_memory to simplify it. */ > > +/* ??? Copied from simple_search_memory. Combine? */ > > GDB and gdbserver? No, I don't think it's a good idea any more. I > used to, but they're just not laid out right. ok.