This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Implement new `info core mappings' command
Hello Jan,
Thanks for the review. Comments below.
Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> On Wed, 26 Oct 2011 22:49:53 +0200, Sergio Durigan Junior wrote:
>> --- a/gdb/corefile.c
>> +++ b/gdb/corefile.c
> [...]
>> @@ -83,6 +89,186 @@ core_file_command (char *filename, int from_tty)
>> }
>>
>>
>> +/* Helper function for `print_core_map'. It is used to iterate
>> + over the corefile's sections and print proper information about
>> + memory-mappings.
>> +
>> + BFD is the bfd used to get the sections.
>> + SECT is the current section being "visited".
>> + OBJ is not used. */
>> +
>> +static void
>> +print_proc_map_iter (bfd *bfd, 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";
>
> I think they are pretty useful, for Linux kernel dumped core files with
> MMF_DUMP_ELF_HEADERS
> /usr/share/doc/kernel-doc-*/Documentation/filesystems/proc.txt
> - (bit 4) ELF header pages in file-backed private memory areas (it is
> effective only if the bit 2 is cleared)
>
> Program Headers:
> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
> NOTE 0x001508 0x0000000000000000 0x0000000000000000 0x0008d0 0x000000 0
> LOAD 0x002000 0x0000000000400000 0x0000000000000000 0x001000 0x008000 R E 0x1000
> LOAD 0x003000 0x0000000000607000 0x0000000000000000 0x002000 0x002000 RW 0x1000
>
> (gdb) info files
> Local core dump file:
> 0x0000000000400000 - 0x0000000000401000 is load1a
> 0x0000000000401000 - 0x0000000000401000 is load1b
> 0x0000000000607000 - 0x0000000000609000 is load2
>
> But one does not see the ending address 0x408000 anywhere, one IMO has to use
> readelf/objdump now to get the full information.
>
>
> I think this function should not be based on sections at all, it should just
> read the segments. Linux kernel does not dump any sections. bfd creates some
> some sections from those segments (_bfd_elf_make_section_from_phdr) but they
> cannot / do not contain any additional info, those are there IMO only for
> better compatibility with sections-only consuming code.
Just to be clear, you're saying that I should actually forget about the
part of the code which checks inside (possible non-empty) sections in
the corefile, and just check immediately for segments?
>> + int proc_map_match_size = strlen (proc_map_match);
>> + /* Flag to indicate whether we have found something. */
>> + int found = 0;
>> + /* The section's size. */
>> + bfd_size_type secsize;
>> + /* 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;
>> + /* Fields to be printed for the proc map. */
>> + unsigned long start = 0, end = 0;
>> + unsigned int size = 0;
>
> On 32bit host with --enable-64-bit-bfd: sizeof (bfd_vma) > sizeof (long)
> moreover for sizeof (int) of `size'.
Ok, I'll replace that by unsigned long too, thanks.
>> + char *filename = NULL;
>> +
>> + if (strncmp (proc_map_match, sect->name, proc_map_match_size) != 0)
>> + /* This section is not useful. */
>> + return;
>> +
>> + bitness = gdbarch_addr_bit (gdbarch_from_bfd (bfd));
>> +
>> + /* Unfortunately, some sections in the corefile don't have any
>> + content inside. This is bad because we need to print, among
>> + other things, its final address in the memory (which is
>> + impossible to know if we don't have a size). That's why we
>> + first need to check if the section's got anything inside it. */
>> + secsize = bfd_section_size (bfd, sect);
>> +
>> + if (secsize == 0)
>> + {
>> + /* Ok, the section is empty. In this case, we must look inside
>> + ELF's Program Header, because (at least) there we have
>> + information about the section's size. That's what we're doing
>> + here. */
>> + Elf_Internal_Phdr *p = elf_tdata (bfd)->phdr;
>> + if (p != NULL)
>> + {
>> + int i;
>> + unsigned int n = elf_elfheader (bfd)->e_phnum;
>> + for (i = 0; i < n; i++, p++)
>> + /* For each entry in the Program Header, we have to
>> + check if the section's initial address is equal to
>> + the entry's virtual address. If it is, then we
>> + have just found the section's entry in the Program
>> + Header, and can use the entry's information to
>> + complete missing data from the section. */
>> + if (sect->vma == p->p_vaddr)
>> + {
>> + found = 1;
>> + break;
>> + }
>
> I do not understand what is a goal of this part. Isn't it related to the
> pairtally omitted segments above? But those are named "load..." so they are
> already skipped.
I'm not sure I understood what you said. The sections named "load..."
are not skipped. It's the sections *not* named "load..." which are.
>> + /* 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. */
>> + found = 0;
>> + ALL_OBJSECTIONS (objfile, s)
>> + if (obj_section_addr (s) >= start
>> + && obj_section_addr (s) <= end)
>
> I think it should ignore S which is section_is_overlay.
You mean I should check for `!section_is_overlay (s)' here?
>> +static void
>> +print_core_map (void)
>> +{
>> + const char *exe;
>> + int bitness;
>> +
>> + gdb_assert (core_bfd != NULL);
>> +
>> + bitness = gdbarch_addr_bit (gdbarch_from_bfd (core_bfd));
>> +
>> + /* Getting the executable name. */
>> + exe = bfd_core_file_failing_command (core_bfd);
>> +
>> + printf_filtered (_("exe = '%s'\n"), exe);
>
> bfd_core_file_failing_command can return NULL, NULL is not compatible with %s;
> also the bfd error may be printed in such case.
Oh, thanks for the catch, I'll update this accordingly.
>> @@ -450,6 +636,11 @@ _initialize_core (void)
>> {
>> struct cmd_list_element *c;
>>
>> + add_info ("core", info_core_cmd, _("\
>> +Show information about a corefile.\n\
>> +Specify any of the following keywords for detailed info:\n\
>> + mappings -- list of mapped memory regions."));
>
> I think it should be add_prefix_cmd so that tab completion works. "mappings
> / "all" should be commands, not parameters. "info proc" already has this bug.
Yeah, `info proc' is buggy indeed. I'll see if I send a patch fixing it
tomorrow. Thanks for the tip.
Thank you,
Sergio.