Re: PING: [PATCH] Fix a bug of addrmap

[Joel Brobecker <brobecker@adacore.com>]

> Joel> Did you familiarize yourself with the code that you'd say that
> Joel> the patch sounds good to you?
> 
> I looked at it again today.

Me too (finally!!! woohoo!)

> I re-read the contract for this function, and it seems pretty clear
> that the current behavior is intended:
> 
> /* In the mutable address map MAP, associate the addresses from START
>    to END_INCLUSIVE that are currently associated with NULL with OBJ
>    instead.  Addresses mapped to an object other than NULL are left
>    unchanged.
> 
> In particular, "currently associated with NULL" is the key.
> 
> My understanding now is that this data structure is designed to be
> built bottom-up.

Right, and the comment explains why just below. Also, I found this,
which I think confirms your understanding:

/* Record that the range of addresses from START to END_INCLUSIVE
   (inclusive, like it says) belongs to BLOCK.  BLOCK's start and end
   addresses must be set already.  You must apply this function to all
   BLOCK's children before applying it to BLOCK.

(buildsym.c:record_block_range)

> So, I think that this is a bug in the caller, and
> that the test I wrote is actually not correct.

It very much looks like it, yes. But it also looks like it wasn't
designed to handle the case of overlapping or nested sym files
address ranges :-(.

I've been thinking about this for the last hour now, and I'm not sure
how to store the information in the addrmap in a way that would solve
the problem.

I don't think that the solution provided here is satisfactory, because
it leads to different solutions depending on the order the psymtabs
are read. Consider for instance:

  file a.c is [0x10 .. 0x30]
  file b.c is [0x20 .. 0x40]

If a.c is processed first, then we get an addrmap that says:
  [0x10 .. 0x20[ = a.c
  [0x20 .. 0x30] = b.c

On the other hand, if b.c is processed first, we get:
  [0x10 .. 0x20[ = a.c
  [0x20 .. 0x40] = b.c

(I might have screwed up the inclusion/exclusion for the bounds,
it's getting really late here). I suspect the above would happen
if we had added a function inside foo.c also pushed to the .text.init
section.

Before using addrmaps, I think we were getting it right by scanning all
psymtabs and trying to get the "best" one, with some heuristic as to what
the best one was, but that was causing us other source of problems.
But this isn't possible in this case since we don't have enough info
in the objfile addrmap.

With all this being said, I think this is as far as I am willing to go
by myself for now. I don't have the feeling that the described problem
is problematic enough to mandate a non-trivial effort. Perhaps I'm
wrong? In any case, it seems easy enough to work around the problem
in the particular example, by putting the routine forced to a different
section into a different file.

> BTW, addrmap is nice code.  It has all the hallmarks of the Blandy
> style.

Very nice indeed...

-- 
Joel