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

[Pedro Alves <palves@redhat.com>]

Thanks for splitting.  It indeed makes it easier to understand
the changes.

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.

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

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.

Thanks,
Pedro Alves