Re: [PATCH] Implement new `info core mappings' command

Eli Zaretskii
2011-10-27T07:04:28Z

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Wed, 26 Oct 2011 19:07:45 -0200
>
> *** Changes since GDB 7.3.1
>
> +* GDB has a new `info core mappings' command. It displays the memory
> + regions in a corefile, similar to `info proc mappings' command.

This is okay.

> + while (argv != NULL && *argv != NULL)
> + {
> + if (strncmp (argv[0], "mappings", strlen (argv[0])) == 0)
> + {
> + mappings_f = 1;
> + }
> + else if (strncmp (argv[0], "all", strlen (argv[0])) == 0)
> + {
> + all = 1;
> + }
> + argv++;
> + }

What is this "all" stuff about?

> +@cindex core dump file

This index entry is too general, it sounds like this section describes
everything about core dump files that GDB supports. Better qualify
it, e.g.

@cindex core dump file, list mapped memory

> +@cindex memory address space mappings

Likewise; you need to qualify this so it's clear it only talks about
memory mappings in the context of core file debugging.

> +@item info core
> +@item info core mappings

You cannot have 2 @item's in a row. All but the first one need to be
@itemx.

> +Report the memory address space ranges accessible in the core file.

I would lose the "space" part.

I think a short example of output would be good here.

> One thing I am not sure is where to put the entry for this command on
> the documentation. I decided to put it below `info proc', but I'd be
> glad if you could give your opinions.

I don't think it's good to put it in the same section as "info proc",
because we say at the beginning of the section

Many versions of SVR4 and compatible systems provide a facility called
@samp{/proc} that can be used to examine the image of a running
process using file-system subroutines.

But this new command has nothing to do with /proc, and does not need
/proc support to work, right?

If /proc is indeed irrelevant, then I'd prefer a separate @subsection
alongside this one. You'd need to add some short explanation of the
background and use case(s) for this command, but having that is a good
idea anyway: as written now, this command lands on the reader out of
the blue, more or less.

Thanks.