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

Jan Kratochvil
2011-11-04T16:27:16Z

Hi Sergio,

On Thu, 03 Nov 2011 21:00:39 +0100, Sergio Durigan Junior wrote:
> gdb/ChangeLog
>
> 2011-11-03 Sergio Durigan Junior <sergiodj@redhat.com>
>
> Implement `info core mappings'.
> * NEWS: Mention new `info core' command, along with its new
> subcommands `mappings', `exe' and `all'.
> * corefile.c: Include a bunch of header files needed to implement
> the `info core mappings'.
> (info_core_what): New enum.
> (info_core_print_core_exe): New function.
> (info_core_print_proc_map_non_elf): Likewise.
> (info_core_print_core_map_elf): Likewise.
> (info_core_print_core_map): Likewise.
> (info_core_cmd_1): Likewise.
> (info_core_cmd): Likewise.
> (info_core_cmd_mappings): Likewise.
> (info_core_cmd_exe): Likewise.
> (info_core_cmd_all): Likewise.
> (_initialize_core): Add new `info core' command, along with its new
> subcommands `mappings', `exe' and `all'.
>
>
> gdb/doc/ChangeLog
>
> 2011-11-03 Sergio Durigan Junior <sergiodj@redhat.com>
>
> Implement `info core mappings'.
> * gdb.texinfo: Add documentation for `info core', and to its new
> subcommands `mappings', `exe' and `all'.
>
>
> gdb/testsuite/ChangeLog
>
> 2011-11-03 Sergio Durigan Junior <sergiodj@redhat.com>
>
> Implement `info core mappings'.
> * gdb.base/corefile.exp: Add test for `info core', and for its new
> subcommands `mappings', `exe' and `all'.
>
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 1713049..df9e573 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,20 @@
>
> *** Changes since GDB 7.3.1
>
> +* GDB has a new `info core' command, which can be used to display information
> + from a corefile. Its subcommands are:
> +
> + ** `info core mappings': display information about memory mappings
> + from the corefile.
> +
> + ** `info core exe': display the executable filename that generated
> + the corefile.
> +
> + ** `info core all': display all of the above.

It has been approved by Eli but FYI I would do: s/: /: /

> +
> +It displays the memory
> + regions in a corefile, similar to `info proc mappings' command.
> +
> * GDB now allows you to skip uninteresting functions and files when
> stepping with the "skip function" and "skip file" commands.
>
> diff --git a/gdb/corefile.c b/gdb/corefile.c
> index ce3b755..d0897fe 100644
> --- a/gdb/corefile.c
> +++ b/gdb/corefile.c
> @@ -24,17 +24,23 @@
> #include <errno.h>
> #include <signal.h>
> #include <fcntl.h>
> +#include <ctype.h>
> #include "inferior.h"
> #include "symtab.h"
> +#include "gdbarch.h"
> +#include "arch-utils.h"
> #include "command.h"
> #include "gdbcmd.h"
> #include "bfd.h"
> +#include "elf-bfd.h"
> +#include "elf/internal.h"
> #include "target.h"
> #include "gdbcore.h"
> #include "dis-asm.h"
> #include "gdb_stat.h"
> #include "completer.h"
> #include "exceptions.h"
> +#include "objfiles.h"
>
> /* Local function declarations. */
>
> @@ -83,6 +89,280 @@ core_file_command (char *filename, int from_tty)
> }
> \f
>
> +/* Implement the `info core' command. */
> +
> +enum info_core_what
> + {
> + /* Display the default exe output. */
> + IC_MINIMAL,
> +
> + /* Display `info core mappings'. */
> + IC_MAPPINGS,
> +
> + /* Display `info core exe'. */
> + IC_EXE,
> +
> + /* Display all of the above. */
> + IC_ALL
> + };
> +
> +/* Implement `info core exe' command. */
> +
> +static void
> +info_core_print_core_exe (void)
> +{
> + const char *exe;
> +
> + /* Getting the executable name. */
> + exe = bfd_core_file_failing_command (core_bfd);
> +
> + if (exe)
> + printf_filtered (_("exe = '%s'\n"), exe);
> + else
> + warning (_("Could not obtain executable name: %s"),

Again IMO s/: /: /, two spaces are not used in GDB.

> + bfd_errmsg (bfd_get_error ()));
> +}
> +
> +/* Helper function for `info_core_print_proc_map', used for non-ELF objects.
> +
> + It is used to iterate over the corefile's BFD sections and print proper
> + information about memory-mappings.
> +
> + BFD is the bfd used to get the sections.
ABFD

> + SECT is the current section being "visited".
> + OBJ is not used. */
> +
> +static void
> +info_core_print_proc_map_non_elf (bfd *abfd, asection *sect, void *obj)
> +{
> + /* We're interested in matching sections' names beginning with
> + `load', because they are the sections containing information
> + about the process' memory regions. */
> + static const char *proc_map_match = "load";

static const char proc_map_match[] = "load";

to save one read/write pointer. :-)

> + int proc_map_match_size = strlen (proc_map_match);
> + /* The section's size. */
> + bfd_size_type size;
> + /* We have to know the bitness of this architecture. */
> + int bitness;
> + /* We'll use these later. They are basically used for iterating
> + over every objfile in the system so that we can find needed
> + information about the memory region being examinated. */
> + struct obj_section *s = NULL;
> + struct objfile *objfile = NULL;

Redundant two initializations.

> + /* Fields to be printed for the proc map. */
> + bfd_vma start;
> + bfd_vma end = 0;
> + char *filename = NULL;
> +
> + if (strncmp (proc_map_match, sect->name, proc_map_match_size) != 0)
> + /* This section is not useful. */
> + return;
> +
> + /* Bitness is important for formatting the text to output. */
> + bitness = gdbarch_addr_bit (gdbarch_from_bfd (abfd));
> +
> + /* Retrieving the section size. */
> + size = bfd_section_size (bfd, sect);
> +
> + start = sect->vma;
> + if (size)
> + end = sect->vma + size;

I think for zero-sized segments END should be equal START.

> +
> + /* Now begins a new part of the work. We still don't have complete
> + information about the memory region. For example, we still need
> + to know the filename which is represented by the region. Such
> + info can be gathered from the objfile's data structure, and for
> + that we must iterate over all the objsections and check if the
> + objsection's initial address is inside the section we have at hand.
> + If it is, then we can use this specific objsection to obtain the
> + missing data. */
> + ALL_OBJSECTIONS (objfile, s)
> + if (!section_is_overlay (s) && obj_section_addr (s) >= start
> + && obj_section_addr (s) <= end)

Separate debug info check like for ELF.

The full coverage as suggested for ELF is not possible foe non-ELF so I do not
know what to do better than what you do.

> + {
> + filename = s->objfile->name;
> + break;
> + }
> +
> + if (bitness == 32)
> + printf_filtered ("\t%#10lx %#10lx %#10x %7s\n",

Why %7s and not %s? The filename is almost always longer than 7 anyway.

> + start,
> + end,
> + (int) size,
> + filename ? filename : "");
> + else
> + printf_filtered (" %#18lx %#18lx %#10x %7s\n",
> + start,
> + end,
> + (int) size,
> + filename ? filename : "");

You cannot (a) assume bfd_vma is compatible with %lx, for example on i386 with
--enable-64-bit-bfd such GDB will fail to properly display i386 core files
from kdump on >=4GB machine which uses ELF64, `long' is there 32-bit.

There is BFD_VMA_FMT.

Still requesting to rather use CORE_ADDR and paddress everywhere as this is
GDB and not BFD.

> +}
> +
> +/* Helper function for `info_core_print_core_map' which handles objects
> + in the ELF format. */
> +
> +static void
> +info_core_print_core_map_elf (void)
> +{
> +#ifdef HAVE_ELF

One could also move it to elfread.c via a function pointer similar to
gnu_ifunc_fns_p but I am OK with this #ifdef.

> + int bitness;
> + int i;
> + unsigned int n_segs;
> + Elf_Internal_Phdr *p;
> +
> + /* Bitness is important when formatting the text to be printed. */
> + bitness = gdbarch_addr_bit (gdbarch_from_bfd (core_bfd));
> +
> + p = elf_tdata (core_bfd)->phdr;
> + if (!p)
> + error (_("Could not obtain mapped addresses."));
> +
> + printf_filtered (_("Mapped address spaces:\n\n"));
> + if (bitness == 32)
> + printf_filtered ("\t%10s %10s %10s %7s\n",
> + "Start Addr",
> + " End Addr",

s/Addr/Address/, there is enough space.

> + " Size", "objfile");
> + else
> + printf_filtered (" %18s %18s %10s %7s\n",
> + "Start Addr",
> + " End Addr",
> + " Size", "objfile");
> +
> + n_segs = elf_elfheader (core_bfd)->e_phnum;
> +
> + for (i = 0; i < n_segs; i++, p++)

Variable `i' is not used in this function.

> + {
> + /* These are basically used for iterating over every objfile in
> + the system so that we can find needed information about the
> + memory region being examinated. */
> + struct obj_section *s = NULL;
> + struct objfile *objfile = NULL;

These two NULL initializations are redundant.

There should be empty lines before comments, even for variable declarations,
in the whole file everywhere.

> + /* Information about the segment. */
> + bfd_vma start;
> + bfd_vma end;
> + bfd_vma size;
> + /* File associated with this memory region. */
> + char *filename = NULL;

I would initialize it by "", was your NULL intentional? (again for non-ELF)

> +
> + if (p->p_type != PT_LOAD)
> + /* We are only interested in PT_LOAD segments. */
> + continue;
> +
> + start = p->p_vaddr;
> + size = p->p_memsz;
> + end = start + size;
> +
> + ALL_OBJSECTIONS (objfile, s)
> + if (!section_is_overlay (s) && obj_section_addr (s) >= start
> + && obj_section_addr (s) <= end)

Also I think you should discard the separate debug info objfiles. I find
wrong to display:
0x37aa200000 0x37aa222000 0x22000 /usr/lib/debug/lib64/ld-2.14.90.so.debug
when it in fact means:
0x37aa200000 0x37aa222000 0x22000 /lib64/ld-2.14.90.so

Be aware even for core file loading /usr/lib/debug/lib64/ld-2.14.90.so.debug
is not enough as some parts (such as .symtab) as sometimes contained just in
/lib64/ld-2.14.90.so, also readonly sections such as .text are contained only
in /lib64/ld-2.14.90.so and not present in the core file so printing just the
.debug file is wrong.

> + {
> + filename = s->objfile->name;
> + break;
> + }

I think the logic should be opposite. Not to find any part of the core
segment from some file but ensure the whole segment is mapped from that file.

For ELF core files you should rather check also the ELF segments in
objfiles->obfd and verify it is fully covered. (I said off-list something
different, not seeing the code, sorry.) That is verify every byte between
0x37aa200000 and 0x37aa222000 belongs to any of the segments from such file.
Otherwise you may print a filename for some core file segment while only part
of that segment is from that file (it probably will not happen for normal core
files but GDB is for debugging problems so it should never give inexact
results).

> +
> + if (bitness == 32)
> + printf_filtered ("\t%#10lx %#10lx %#10x %7s\n",
> + start,
> + end,
> + (int) size,
> + filename ? filename : "");

Could you print all the info readelf -Wa prints? I would use rather readelf
to see the protection bits, also I guess the file size vs. mem size may
suggest if it is a data segment or code segment, up to you how to display it,
even some indicator "from core file" (memsize==filesize) vs. "from local file"
(filesize==0) vs. "partial" (others, e.g. filesize==0x1000) could be enough.

> + else
> + printf_filtered (" %#18lx %#18lx %#10x %7s\n",
> + start,
> + end,
> + (int) size,
> + filename ? filename : "");
> + }
> +#else
> + error (_("Your system does not support ELF format."));
> +#endif /* HAVE_ELF */
> +}
> +
> +/* Implement the `info core map' command. */
> +
> +static void
> +info_core_print_core_map (void)
> +{
> + gdb_assert (core_bfd != NULL);
> +
> + if (bfd_get_flavour (core_bfd) == bfd_target_elf_flavour)
> + info_core_print_core_map_elf ();
> + else
> + bfd_map_over_sections (core_bfd,
> + info_core_print_proc_map_non_elf,
> + NULL);
> +}
> +
> +/* Implement the `info core' command. */
> +
> +static void
> +info_core_cmd_1 (char *args, enum info_core_what what, int from_tty)
> +{
> + char **argv = NULL;
> + int mappings_f = (what == IC_MAPPINGS || what == IC_ALL);
> + int exe_f = (what == IC_MINIMAL || what == IC_EXE || what == IC_ALL);
> + struct cleanup *c = NULL;
> +
> + if (!core_bfd)
> + error (_("You are not using a corefile at the moment."));
> +

> + if (args)
> + {
> + /* Break up 'args' into an argv array. */
> + argv = gdb_buildargv (args);
> + c = make_cleanup_freeargv (argv);
> + }
> + while (argv != NULL && *argv != NULL)
> + {
> + if (strncmp (argv[0], "mappings", strlen (argv[0])) == 0)
> + {
> + mappings_f = 1;
> + }
> + argv++;
> + }

This is completely redundant block, this function should just complain if ARGS
contains anything as the "exec"/"mappings" parameters are already parsed
through info_core_cmd_mappings/info_core_cmd_exe.

> +
> + if (exe_f)
> + info_core_print_core_exe ();
> +
> + if (mappings_f)
> + info_core_print_core_map ();
> +
> + if (c)
> + do_cleanups (c);

If ARGS was NULL "c" is left NULL, which will execute all the cleanups stored
even before and unrelated to info_core_cmd_1. You must either if (args && c)
or pre-initialize C by make_cleanup (null_cleanup, NULL) or else-initialize
C the same way.

In fact I see now whole C is redundant together with that parsing block.

> +}
> +
> +/* Implement `info core' without parameters. */

Empty line.

> +static void
> +info_core_cmd (char *args, int from_tty)
> +{
> + info_core_cmd_1 (args, IC_MINIMAL, from_tty);
> +}
> +
> +/* Implement `info core mappings'. */
> +
> +static void
> +info_core_cmd_mappings (char *args, int from_tty)
> +{
> + info_core_cmd_1 (args, IC_MAPPINGS, from_tty);
> +}
> +
> +/* Implement `info core exe'. */
> +
> +static void
> +info_core_cmd_exe (char *args, int from_tty)
> +{
> + info_core_cmd_1 (args, IC_EXE, from_tty);
> +}
> +
> +/* Implement `info core all'. */
> +
> +static void
> +info_core_cmd_all (char *args, int from_tty)
> +{
> + info_core_cmd_1 (args, IC_ALL, from_tty);
> +}
> +
> /* If there are two or more functions that wish to hook into
> exec_file_command, this function will call all of the hook
> functions. */
> @@ -449,6 +729,26 @@ void
> _initialize_core (void)
> {
> struct cmd_list_element *c;
> + static struct cmd_list_element *info_core_cmdlist;
> +
> + add_prefix_cmd ("core", class_info, info_core_cmd,
> + _("\
> +Show information about a corefile.\n\
> +The command uses the corefile loaded."),
> + &info_core_cmdlist, "info core ",
> + 1/*allow-unknown*/, &infolist);
> +
> + add_cmd ("mappings", class_info, info_core_cmd_mappings, _("\
> +List of mapped memory regions."),
> + &info_core_cmdlist);
> +
> + add_cmd ("exe", class_info, info_core_cmd_exe, _("\
> +List absolute filename for executable which generated the corefile."),
> + &info_core_cmdlist);
> +
> + add_cmd ("all", class_info, info_core_cmd_all, _("\
> +List all available corefile information."),
> + &info_core_cmdlist);
>
> c = add_cmd ("core-file", class_files, core_file_command, _("\
> Use FILE as core dump for examining memory and registers.\n\
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 93450c6..e138f5a 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -17829,6 +17829,62 @@ processes and all the threads within each process.
> For QNX Neutrino only, this command displays the list of all mapinfos.
> @end table
>
> +@node Process Information from Core Dump File
> +@subsection Process Information from Core Dump File
> +@cindex examine core dump file process information
> +@cindex process info via core dump file
> +
> +If your system supports the generation of core dump files (core files), you
> +can use them to obtain information about processes. For that, you can use
> +the command @code{info core} inside @value{GDBN} to report information like
> +the memory mappings of the process when the core dump was generated.
> +@code{info core} works only on systems that support core dump files, and only
> +when you are using a core dump file inside @value{GDBN}.
> +
> +@xref{Core File Generation}, for information on how to generate core dump
> +files inside @value{GDBN}. @xref{Files}, for information on invoking
> +@value{GDBN} in the post-mortem debugging mode.
> +
> +@table @code
> +@kindex info core
> +@cindex core dump file, process information
> +@item info core
> +@itemx info core mappings
> +@cindex memory address space mappings inside a core dump file
> +Report the memory address ranges accessible in the core dump file. Assuming
> +you have a core dump file and it is loaded into @value{GDBN}, the output of
> +the command will be similar to:
> +
> +@smallexample
> +(@value{GDBP}) info core mappings
> +Mapped address spaces:
> +
> + Start Addr End Addr Size objfile
> + 0x400000 0x401000 0x1000 /tmp/a.out
> + 0x600000 0x601000 0x1000 /tmp/a.out
> +0x397de00000 0x397de1f000 0x1f000 /usr/lib/debug/lib/ld.so.debug
> +0x397e01e000 0x397e01f000 0x1000 /usr/lib/debug/lib/ld.so.debug
> +0x397e01f000 0x397e020000 0x1000 /usr/lib/debug/lib/ld.so.debug
> +0x397e020000 0x397e021000 0x1000 /usr/lib/debug/lib/ld.so.debug
> +0x397e200000 0x397e391000 0x191000 /usr/lib/debug/lib/libc.so.debug
> +0x397e591000 0x397e595000 0x4000 /usr/lib/debug/lib/libc.so.debug
> +0x397e595000 0x397e596000 0x1000 /usr/lib/debug/lib/libc.so.debug

Here remove the .debug suffixes as noted above.

> +0x397e596000 0x397e59c000 0x6000
> +@end smallexample
> +
> +@item info core exe
> +Show the filename of the process that generated this core dump file.
> +
> +@smallexample
> +(@value{GDBP}) info core exe
> +exe = '/tmp/a.out'
> +@end smallexample
> +
> +@item info core all
> +Show all the information about the core dump file described under all of
> +the above @code{info core} subcommands.
> +@end table
> +
> @node DJGPP Native
> @subsection Features for Debugging @sc{djgpp} Programs
> @cindex @sc{djgpp} debugging
> diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp
> index 5b0cdf1..ccf226f 100644
> --- a/gdb/testsuite/gdb.base/corefile.exp
> +++ b/gdb/testsuite/gdb.base/corefile.exp
> @@ -171,6 +171,18 @@ gdb_test_multiple "x/8bd buf2" "$test" {
> }
> }
>
> +# Test the `info core mappings' command.
> +set ws "\[ \t\]+"
> +set test "test info core mappings"
> +gdb_test "info core mappings" \
> + ".*Mapped address spaces:.*${hex}${ws}${hex}${ws}${hex}.*" \
^^ useless leading .*

> + $test
> +
> +# Test the `info core exe' command.
> +set test "test info core exe"
> +gdb_test "info core exe" \
> + "exe = .*"

Could you put some content there? Testing for .* is not much useful.

> +
> # test reinit_frame_cache
>
> gdb_load ${binfile}

Thanks,
Jan