Re: [RFA] new command to search memory

Doug Evans
2008-02-29T02:44:29Z

Thanks for the review.

On Mon, Feb 25, 2008 at 6:23 PM, Daniel Jacobowitz <drow@false.org> 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 (&current_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.