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

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

On Thursday, March 19 2015, Pedro Alves wrote:

> On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote:
>> This commit implements the new 'enum memory_mapping_state', which can
>> be used to represent the different states of each memory mapping from
>> the inferior.  These states are:
>> 
>>   - MODIFIED, which means that the mapping should be dumped in
>>     corefiles
>> 
>>   - UNMODIFIED, which means that the mapping should not be dumped in
>>     corefiles (e.g., mappings that have been marked as VM_DONTDUMP), and
>> 
>>   - UNKNOWN, which means that we don't know whether the mapping should
>>     or should not be dumped.
>> 
>
> I'm sorry to push back on this, but it sounds to me that this is mixing
> up orthogonal aspects.
>
> For example, what if a mapping is indeed modified, but the tdep code
> decides it should not be dumped?  With this interface, you need to
> "lie" and pass down UNMODIFIED.
>
> And then, what if a mapping is definitely not modified, but the
> tdep code decides it should dumped (e.g., say we could tell that the
> vdso mapping really wasn't modified, but we still want to dump
> it anyhow because there's no file on the filesystem to read the
> vdso contents from later at core load time).  With this interface,
> you need to pass down either MODIFIED or UNKNOWN.
>
> So it sounds to me that instead, the arch/target code that is walking
> the memory mappings should just not call the "dump this mapping"
> callback if it decides the mapping should not be dumped.

Right, I agree there is some confusion in the terms being used here.
Thanks for giving examples that make this confusion obvious.

While I still think gcore_create_callback should probably receive more
attention, I will withdraw this patch because it doesn't really help to
fix the problem at hand, which is to make GDB obey
/proc/PID/coredump_filter.

> And if we do _that_ first, then, what other changes to
> gcore_create_callback would be required to make it do what
> we need?

If we do what you proposed, we wouldn't need to change
gcore_create_callback at all *to fix the specific problem of making GDB
obey /proc/PID/smaps*.  This is why, as I said, I am withdrawing this
patch.

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

> This may need further discussion, but in any case, note that the
> descriptions above of what each state means ...
>
>> +/* Enum used to inform the state of a memory mapping.  This is used in
>> +   functions implementing find_memory_region_ftype.  */
>> +
>> +enum memory_mapping_state
>> +  {
>> +    MEMORY_MAPPING_MODIFIED,
>> +    MEMORY_MAPPING_UNMODIFIED,
>> +    MEMORY_MAPPING_UNKNOWN_STATE,
>> +  };
>
> ... should be here in the code.

This is not needed anymore.

Thanks,

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