[PATCH] make gcore dump read-only sections not from files

[Michael Snyder]

Roland McGrath wrote:
> After making gcore dump NT_AUXV notes, I noticed that examining the
> resultant dumps still had the problem that the vsyscall DSO page was not
> included in the dump.  I changed Linux 2.6 to show that region of memory in
> /proc/PID/maps, which is the natural way to have it show up properly for
> gdb's target_find_memory_regions.  After testing my kernel changes I tried
> gcore again and was surprised to find that it still didn't produce a dump
> actually containing the magic page's data.  I tracked this down to
> gcore_create_callback, where it avoids writing any read-only sections into
> the dump.  It's obviously desireable to omit all the text segments from
> executables and shared libraries so that core dumps are not always huge.
> But you lose information by omitting read-only pages that didn't come from
> files gdb knows to look in.  The following patch to gcore.c makes it omit a
> read-only section only if it's found in a known on-disk BFD.  
> 
> With that change alone, gcore omits only the text segment of the executable
> itself and not only (rightly) dumps the vsyscall DSO page but also
> (wrongly) dumps all the normal shared libraries' text segments.  I tracked
> this down to to_sections getting the solib sections only when the right
> argument is passed to SOLIB_ADD, and the infrun.c patch below changes that.
> I don't understand why that wasn't done already, or what effects the change
> might have; I have not noticed any problems, and gcore now omits all the
> right things and none of the wrong ones.

I'll step up, since I wrote gcore.  I like what you're doing,
but I'm uncertain about the SOLIB_ADD part.  Like you, I don't
understand why it was the way it was, nor the implications of
the change.  But I think this can be done fairly easily without
that change.

As I understand it, you want to loop thru every section of
every objfile that gdb knows as part of the process (including
the shared libraries).  If you'll look at objfile_find_memory_regions
in gcore.c, there's an example of how to do that without using
current_target.to_sections.  I don't know why current_target.
to_sections doesn't conform to what you expect, but I'd rather
understand it before changing it... and that might be more effort
than it's worth.

Does this (rewriting your main loop using ALL_OBJSECTIONS)
seem reasonable?

> 
> I was motivated to discover this by wanting gcore to produce complete
> results for the vsyscall page so unwinding works when examining the dump.
> This is in part a generalized solution motivated by avoiding a weird
> special case kludge for the vsyscall page.  But I think it's also
> desireable for other cases.  Take for example this program:
> 
> 	#include <sys/mman.h>
> 	#include <string.h>
> 	#include <unistd.h>
> 
> 	int main ()
> 	{
> 	  const size_t pagesz = sysconf (_SC_PAGE_SIZE);
> 	  void *page = mmap (0, pagesz, PROT_READ|PROT_WRITE,
> 			     MAP_ANON|MAP_PRIVATE, -1, 0);
> 	  memset (page, 0x17, pagesz);
> 	  mprotect (page, pagesz, PROT_READ);
> 	  printf ("pid %d address %p size %x\n", getpid (),
> 		  page, (unsigned int) pagesz);
> 	  abort ();
> 	}
> 
> Without this change, a gcore made with the process stopped at the abort or
> printf calls after mprotect will not include the `page' page--when
> examining the dump, there will be no way to know it contains 0x17.  This is
> obviously a contrived and useless example.  But such cases do exist in the
> real world, e.g. garbage collectors that temporarily mprotect some pages to
> read-only--in a core dump of such a process, knowing the contents of these
> pages could be important.  With my change, that page appears in the dump.
> 
> Note that the existing gcore behavior is consistent with what the Linux
> kernel does in making dumps: it omits all read-only pages.  However, I
> think that's arguably a bug because of cases like the one I've just cited.
> I am submitting patches to make Linux 2.6 omit only the read-only pages
> that are backed by mapped files, and include all anonymous pages in the dump.
> 
> Note that this patch makes gcore dump more regions than the Linux kernel
> does even with my change to its behavior.  In particular, read-only mmap'd
> portions of files are dumped by gcore but not by the kernel.  This is a
> real common issue in practice, as your average GNU/Linux process nowadays
> has the large locale-archive file mapped in, and some processes may be
> mapping huge files in read-only.  An alternative change would be to change
> the to_find_memory_regions callback interface to add a flag argument saying
> whether the memory region came from a file.  Then gcore_create_callback
> could simply test !write && !anonymous and be wholly consistent with the
> kernel core dumping (assuming my change to it), and the infrun.c change is
> not required (though I'm still curious about why it's not that way already).
> 
> Comments?
> 
> 
> Thanks,
> Roland
> 
> 
> 
> 2003-10-07  Roland McGrath  <roland@redhat.com>
> 
> 	* gcore.c (gcore_create_callback): Omit read-only sections only if
> 	they correspond to a known disk file.
> 	* infrun.c (handle_inferior_event): Pass &current_target to SOLIB_ADD.
> 
> Index: gcore.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gcore.c,v
> retrieving revision 1.12
> diff -p -u -r1.12 gcore.c
> --- gcore.c	21 Sep 2003 01:26:44 -0000	1.12
> +++ gcore.c	7 Oct 2003 07:40:11 -0000
> @@ -343,10 +343,38 @@ gcore_create_callback (CORE_ADDR vaddr, 
>  
>    if (write == 0)
>      {
> +      /* See if this region of memory lies inside a known file on disk.  */
> +      struct section_table *secp;
> +      for (secp = current_target.to_sections;
> +	   secp < current_target.to_sections_end;
> +	   secp++)
> +	{
> +	  asection *const asec = secp->the_bfd_section;
> +	  bfd_vma align = (bfd_vma) 1 << bfd_get_section_alignment (secp->bfd,
> +								    asec);
> +	  bfd_vma start = secp->addr & -align;
> +	  bfd_vma end = (secp->endaddr + align - 1) & -align;
> +	  /* Match if either the entire memory region lies inside the
> +	     section (i.e. a mapping covering some pages of a large
> +	     segment) or the entire section lies inside the memory region
> +	     (i.e. a mapping covering multiple small sections).  */
> +	  if ((vaddr >= start && vaddr + size <= end)
> +	      || (start >= vaddr && end <= vaddr + size))
> +	    {
> +	      if (bfd_get_file_flags (secp->bfd) & BFD_IN_MEMORY)
> +		/* This BFD was synthesized from reading target memory,
> +		   so we don't want to omit that.  */
> +		continue;
> +	      break;
> +	    }
> +	}
> +
>        flags |= SEC_READONLY;
> -      /* Mark readonly sections as zero-sized, such that we can avoid
> -         copying their contents.  */
> -      size = 0;
> +
> +      if (secp < current_target.to_sections_end)
> +	/* Mark readonly sections as zero-sized, such that we can avoid
> +	   copying their contents.  */
> +	size = 0;
>      }
>  
>    if (exec)
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.115
> diff -b -p -u -r1.115 infrun.c
> --- infrun.c	2 Oct 2003 20:28:29 -0000	1.115
> +++ infrun.c	7 Oct 2003 07:40:23 -0000
> @@ -1366,7 +1366,7 @@ handle_inferior_event (struct execution_
>  	     terminal for any messages produced by
>  	     breakpoint_re_set.  */
>  	  target_terminal_ours_for_output ();
> -	  SOLIB_ADD (NULL, 0, NULL, auto_solib_add);
> +	  SOLIB_ADD (NULL, 0, &current_target, auto_solib_add);
>  	  target_terminal_inferior ();
>  
>  	  /* Reinsert breakpoints and continue.  */
> @@ -2189,7 +2189,7 @@ process_event_stop_test:
>  	     terminal for any messages produced by
>  	     breakpoint_re_set.  */
>  	  target_terminal_ours_for_output ();
> -	  SOLIB_ADD (NULL, 0, NULL, auto_solib_add);
> +	  SOLIB_ADD (NULL, 0, &current_target, auto_solib_add);
>  	  target_terminal_inferior ();
>  
>  	  /* Try to reenable shared library breakpoints, additional
>