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

Sergio Durigan Junior
2015-03-19T23:07:01Z

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