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

[Sergio Durigan Junior <sergiodj@redhat.com>]

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