Re: [PATCH 1/4] Improve identification of memory mappings

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, March 20 2015, Pedro Alves wrote:

> On 03/19/2015 11:06 PM, Sergio Durigan Junior wrote:
>
>> However, IMHO gcore_create_callback still has some problems.  For
>> example, this heuristic used to determine whether a mapping should be
>> dumped or not:
>> 
>>   if (write == 0 && modified == 0 && !solib_keep_data_in_core (vaddr, size))
>>     {
>>       /* See if this region of memory lies inside a known file on disk.
>> 	 If so, we can avoid copying its contents by clearing SEC_LOAD.  */
>>       struct objfile *objfile;
>>       struct obj_section *objsec;
>> 
>>       ALL_OBJSECTIONS (objfile, objsec)
>> 	{
>> 	  bfd *abfd = objfile->obfd;
>> 	  asection *asec = objsec->the_bfd_section;
>> 	  bfd_vma align = (bfd_vma) 1 << bfd_get_section_alignment (abfd,
>> 								    asec);
>> 	  bfd_vma start = obj_section_addr (objsec) & -align;
>> 	  bfd_vma end = (obj_section_endaddr (objsec) + 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).
>> 
>> 	     This BFD was synthesized from reading target memory,
>> 	     we don't want to omit that.  */
>> 	  if (objfile->separate_debug_objfile_backlink == NULL
>> 	      && ((vaddr >= start && vaddr + size <= end)
>> 	          || (start >= vaddr && end <= vaddr + size))
>> 	      && !(bfd_get_file_flags (abfd) & BFD_IN_MEMORY))
>> 	    {
>> 	      flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS);
>> 	      goto keep;	/* Break out of two nested for loops.  */
>> 	    }
>> 	}
>> 
>>     keep:;
>>     }
>> 
>> will not be used by any code, because everyone will be passing
>> 'modified' as 1 with my following patch (the only code that could pass
>> 'modified' as zero was linux_find_memory_regions_full, which I will
>> patch to only pass 1 as well).
>
> Alright.  Good that that now became clear.
>
> I found the initial submission for that, btw:
>
>   https://sourceware.org/ml/gdb-patches/2003-10/msg00164.html

Thanks for doing that.

> I wonder whether it'd be worth to keep that somehow, for the fallback
> cases when /proc//smaps or some other /proc file you're relying
> on for file-backed read-only region identification is missing
> (because old kernel, or even /proc not mounted).  Maybe not.

I will not touch this code in this series, of course, but I also think
that this part should be used more often.  For example, there are cases
when we are not sure whether the mapping needs to be dumped or not
(e.g., see gdb/gnu-nat.c), and currently we are just passing 'modified =
1', which makes GDB dump those pages.  I guess this is a situation where
this code could help.

Anyway, something that I can look into later.

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/