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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 31 Oct 2011 04:16:51 +0100, Sergio Durigan Junior wrote:
> > 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?

Yes.  Otherwise you need the tricks combining it with segments you do below
anyway, moreover you can read in sections in core file generated by gcore
which are completely useless,


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

I meant you should use bfd_vma.  Even `unsigned long' is too short.  You would
need to use `unsigned long long' but that has compatibility issues already
resolved if you just use `bfd_vma'.


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

Sorry, OK.  But still I do not understand why to look at sections of core file
at all.  Another question is to handle non-ELF core files but that could be
a completely separate code path.


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

Yes.


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

Or it should be a single command using add_setshow_enum_cmd, not sure which
approach is better.  Still I think the separate commands are better as they
can have each specific help text.


Thanks,
Jan