mmap fixes

[Corinna Vinschen]

On Dec 19 09:59, Ken Brown wrote:
> Hi Corinna,
> 
> On 12/19/2024 5:17 AM, Corinna Vinschen wrote:
> > Fixes: c68de3a262fe5 ("* mmap.cc (class mmap_record): Declare new map_pages method with address parameter.")
> 
> Again thanks.  Some time you'll have to tell me how to find those commits.

I'm using `git blame <filename>' and check relevant commit IDs backwards
in time until it fits.  When I find a commit 123456789 which changes the
lines but is not actually the one introducing the problem, next I use

  git blame <filename> 123456789^  <--Note the caret

For files changing their name, you have to change the filename on
the commandline, too.  For instance

  git blame mm/mmap.cc

but

  git blame mmap.cc 123456789^

if the commit is before the filename change.

> > > +	  if (u_addr > (caddr_t) addr || u_len < len
> > >   	      || !rec->compatible_flags (flags))
> > 
> > While this is strictly correct, I wonder if this shouldn't be
> > 
> >    if (u_addr > (caddr_t) addr || u_addr + u_len < (caddr_t) addr + len ...
> > 
> > for plain readability.  The problem is, you can't see what match()
> > really returns, an intersection or the entire free region.  That's
> > what I stumbled over in the cygwin ML.
> > 
> > This way, the code immediately tells the reader that we want to make
> > sure that [addr,addr+len] is a region completely inside the region
> > [u_addr,u_addr+u_len], without needing to know what exactly match()
> > returns.  And it would still be correct, even if we redefine match().
> > 
> > What do you think?
> I agree.  I'll make that change and push.  At some point it might be helpful
> to add a reference parameter "contains" to match(), so that match() can
> return the information about whether the mmap_record region contains
> [addr,addr+len].  That way the relevant tests can be done right where the
> reader can see what's going on.  But I'm not going to try to do that
> immediately.

Sounds like a good extension.  It would be great if we could handle
partial page intersections at one point, too.


Corinna