Re: [RFA] new command to search memory

Daniel Jacobowitz
2008-02-26T02:24:02Z

On Wed, Feb 13, 2008 at 06:19:15PM -0800, Doug Evans wrote:
> I didn't get a response to the non-doc portions of this patch.

Sorry for the delay on this, Doug. To be honest, I was hoping someone
else would comment on the interface. I think it's a bit confusing and
also somewhat inconsistent among GDB commands. But we do need the
feature, and you've obviously spent a while thinking about it, and
you're the one who did all the work, and the GDB CLI commands are
plenty inconsistent already. So I'll just get used to it :-)

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.

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.

What do you think of "+" instead of "@" to distinguish lengths? "find
&hello[0] +0x100".

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. */

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).

After that, we can (and should IMO) document in the manual that all
of the arguments to find are source language expressions.

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.

I don't have a ton to say about the code itself, except that you
forgot to format some of your comments as sentences. Similarly
for error messages, though I can't remember what our convention
is for that; Eli may know better.

find_command is big, and might benefit from a couple of smaller
documented functions. The list of local variables is a full page.

> +static void
> +put_bits (bfd_uint64_t data, char *buf, int bits, bfd_boolean big_p);
> +
> +static int
> +parse_search_string (char **strp, char **parsed_stringp);

Prototypes shouldn't have the function name in the first column, but
fortunately you don't even need these.

default_search_memory was similar, and some of the others in headers.

> + /* If endian is unknown use big endian.
> + ??? Is there an established convention for what to pick? */
> + bfd_boolean big_p = endian != BFD_ENDIAN_LITTLE;

GDB should handle this elsewhere; if you get BFD_ENDIAN_UNKNOWN here
we have bigger problems. So doing it this way is fine.

> + /* Don't go to the target if we don't have to.
> + This is done after checking packet->support to avoid the possibility that
> + a success for this edge case means the facility works in general. */

done before, you mean?

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.

> @@ -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.

> @@ -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.

> +/* 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).

> +
> +/* 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.

> +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.

> 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.

--
Daniel Jacobowitz
CodeSourcery