Re: [PATCH] libdw: Search for the last matching address with dwarf_getsrc_die.

[Mark Wielaard <mjw at redhat dot com>]

On Fri, Dec 26, 2014 at 02:12:14PM -0700, Josh Stone wrote:
> >    /* The lines are sorted by address, so we can use binary search.  */
> > -  size_t l = 0, u = nlines;
> > +  size_t l = 0, u = nlines - 1;
> >    while (l < u)
> 
> What if nlines == 0?

I wanted to say that cannot happen. If nlines == 0 then cu->lines would
be NULL. But looking at the code there is a theoretical possibility
that isn't the case. It seems unlikely to happen in practice, but we
should guard against it indeed.

> > +      size_t idx = u - (u -l) / 2;
> 
> [nit] missing a space: "(u - l)"

Fixed.
 
> > +  /* This is guaranteed for us by libdw read_srclines.  */
> >    if (nlines > 0)
> >      assert (lines->info[nlines - 1].end_sequence);
> 
> In case your answer above was that nlines is never 0, then this "if" is
> redundant.

I moved the if up before the while loop.

> > +  /* The last line which is less than or equal to addr is what we want,
> > +     except with an end_sequence which can only be strictly equal.  */
> > +  Dwarf_Line *line = &lines->info[l];
> > +  if (line->addr == addr || (! line->end_sequence && line->addr < addr))
> > +    return &lines->info[l];
> 
> BTW, it seems like the end ought to be exclusive, i.e. [start,end).  I
> only wrote my commit this way because some existing tests depended on
> end-inclusive behavior, and I forgot to argue it. :)  Thoughts?

I think you are right and those tests, addrscopes and funcscopes, are
wrong. They use dwfl_module_getsrc to find the line associated with start
and end of the scope. But for the end they use the value of the high_pc
attribute. The high_pc attributes indicates the first address beyond
the current scope of the associated DIE. So the tests should use
highpc - 1 as end of scope.

I created a patch to change dwfl_module_getsrc to not match against
a line with end_sequence set, changed the tests to use highpc -1.
And adjusted this patch for dwarf_getsrc_die. to match the new
behavior. Do those changes look correct to you?

Thanks,

Mark