This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] new command to search memory


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]