[PATCH] perf map: fix overlapped map handling

[PATCH] perf map: fix overlapped map handling

[Steve MacLean]

Whenever an mmap/mmap2 event occurs, the map tree must be updated to add a new
entry. If a new map overlaps a previous map, the overlapped section of the
previous map is effectively unmapped, but the non-overlapping sections are
still valid.

maps__fixup_overlappings() is responsible for creating any new map entries from
the previously overlapped map. It optionally creates a before and an after map.

When creating the after map the existing code failed to adjust the map.pgoff.
This meant the new after map would incorrectly calculate the file offset
for the ip. This results in incorrect symbol name resolution for any ip in the
after region.

Make maps__fixup_overlappings() correctly populate map.pgoff.

Add an assert that new mapping matches old mapping at the beginning of
the after map.

Signed-off-by: Steve MacLean <Steve.MacLean@Microsoft.com>
---
 tools/perf/util/map.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 5b83ed1..73870d7 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "symbol.h"
+#include <assert.h>
 #include <errno.h>
 #include <inttypes.h>
 #include <limits.h>
@@ -850,6 +851,8 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp
 			}
 
 			after->start = map->end;
+			after->pgoff = pos->map_ip(pos, map->end);
+			assert(pos->map_ip(pos, map->end) == after->map_ip(after, map->end));
 			__map_groups__insert(pos->groups, after);
 			if (verbose >= 2 && !use_browser)
 				map__fprintf(after, fp);
-- 
2.7.4

Re: [PATCH] perf map: fix overlapped map handling

[Arnaldo Carvalho de Melo]

Em Fri, Sep 20, 2019 at 07:20:18PM +0000, Steve MacLean escreveu:
> Whenever an mmap/mmap2 event occurs, the map tree must be updated to add a new
> entry. If a new map overlaps a previous map, the overlapped section of the
> previous map is effectively unmapped, but the non-overlapping sections are
> still valid.
> 
> maps__fixup_overlappings() is responsible for creating any new map entries from
> the previously overlapped map. It optionally creates a before and an after map.
> 
> When creating the after map the existing code failed to adjust the map.pgoff.
> This meant the new after map would incorrectly calculate the file offset
> for the ip. This results in incorrect symbol name resolution for any ip in the
> after region.
> 
> Make maps__fixup_overlappings() correctly populate map.pgoff.
> 
> Add an assert that new mapping matches old mapping at the beginning of
> the after map.
> 
> Signed-off-by: Steve MacLean <Steve.MacLean@Microsoft.com>
> ---
>  tools/perf/util/map.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 5b83ed1..73870d7 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "symbol.h"
> +#include <assert.h>
>  #include <errno.h>
>  #include <inttypes.h>
>  #include <limits.h>
> @@ -850,6 +851,8 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp
>  			}
>  
>  			after->start = map->end;
> +			after->pgoff = pos->map_ip(pos, map->end);

So is this equivalent to what __split_vma() does in the kernel, i.e.:

        if (new_below)
                new->vm_end = addr;
        else {
                new->vm_start = addr;
                new->vm_pgoff += ((addr - vma->vm_start) >> PAGE_SHIFT);
        }

where new->vm_pgoff starts equal to the vm_pgoff of the mmap being
split?

- Arnaldo

> +			assert(pos->map_ip(pos, map->end) == after->map_ip(after, map->end));



>  			__map_groups__insert(pos->groups, after);
>  			if (verbose >= 2 && !use_browser)
>  				map__fprintf(after, fp);
> -- 
> 2.7.4

-- 

- Arnaldo

RE: [PATCH] perf map: fix overlapped map handling

[Steve MacLean]

>>  			after->start = map->end;
>> +			after->pgoff = pos->map_ip(pos, map->end);
>
> So is this equivalent to what __split_vma() does in the kernel, i.e.:
>
>        if (new_below)
>                new->vm_end = addr;
>        else {
>                new->vm_start = addr;
>                new->vm_pgoff += ((addr - vma->vm_start) >> PAGE_SHIFT);
>        }
>
> where new->vm_pgoff starts equal to the vm_pgoff of the mmap being split?

It is roughly equivalent.  The pgoff in struct map is stored in bytes not in pages, so it doesn't include the shift.

An earlier version of this patch used:
  			after->start = map->end;
+			after->pgoff += map->end - pos->start;

Instead of the newer Functionally equivalent:
  			after->start = map->end;
+			after->pgoff = pos->map_ip(pos, map->end);

I preferred the latter form as it made more sense with the assertion that the mapping of map->end should match in pos and after.

Steve

Re: [PATCH] perf map: fix overlapped map handling

[Arnaldo Carvalho de Melo]

Em Fri, Sep 20, 2019 at 09:46:15PM +0000, Steve MacLean escreveu:
> >>  			after->start = map->end;
> >> +			after->pgoff = pos->map_ip(pos, map->end);
> >
> > So is this equivalent to what __split_vma() does in the kernel, i.e.:
> >
> >        if (new_below)
> >                new->vm_end = addr;
> >        else {
> >                new->vm_start = addr;
> >                new->vm_pgoff += ((addr - vma->vm_start) >> PAGE_SHIFT);
> >        }
> >
> > where new->vm_pgoff starts equal to the vm_pgoff of the mmap being split?
> 
> It is roughly equivalent.  The pgoff in struct map is stored in bytes not in pages, so it doesn't include the shift.
> 
> An earlier version of this patch used:
>   			after->start = map->end;
> +			after->pgoff += map->end - pos->start;
> 
> Instead of the newer Functionally equivalent:
>   			after->start = map->end;
> +			after->pgoff = pos->map_ip(pos, map->end);
> 
> I preferred the latter form as it made more sense with the assertion that the mapping of map->end should match in pos and after.

Sorry for the delay in continuing with this discussion, I was at
Plumbers in Lisbon and then some vacations, etc. Also I was hoping
someone else would jump here and provide some Reviewed-by tag, etc :-)

So, if they are equivalent then I think its better to use code that
ressembles the kernel as much as possible, so that when in doubt we can
compare the tools/perf calcs with how the kernel does it, filtering out
things like the PAGE_SHIFT, can we go that way?

Also do you have some reproducer, if you have one then we can try and
have this as a 'perf test' entry, bolting some more checks into
tools/perf/tests/perf-record.c or using it as a start for a test that
stresses this code.

This is not a prerequisite for having your fix on, but would help
checking that perf doesn't regresses in this area.
 
- Arnaldo

RE: [PATCH] perf map: fix overlapped map handling

[Steve MacLean]

>> An earlier version of this patch used:
>>   			after->start = map->end;
>> +			after->pgoff += map->end - pos->start;
>> 
>> Instead of the newer Functionally equivalent:
>>   			after->start = map->end;
>> +			after->pgoff = pos->map_ip(pos, map->end);
>> 
>> I preferred the latter form as it made more sense with the assertion that the mapping of map->end should match in pos and after.
>
> So, if they are equivalent then I think its better to use code that ressembles the kernel as much as possible, so that when in doubt we can compare the tools/perf calcs with how the kernel does it, filtering out things like the PAGE_SHIFT, can we go that way?
>
> Also do you have some reproducer, if you have one then we can try and have this as a 'perf test' entry, bolting some more checks into tools/perf/tests/perf-record.c or using it as a start for a test that stresses this code.
>
> This is not a prerequisite for having your fix on, but would help checking that perf doesn't regresses in this area.
>
> - Arnaldo

I have updated the patch to use the earlier version, which more closely matches the kernel.

I have updated the commit message to include the repro info.

I am including a few other patches I have generated while adding support for perf jitdump to coreclr.