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
