Re: [PATCH] Make elf section sorting more deterministic

[Mark Wielaard <mark at klomp dot org>]

On Fri, Apr 28, 2017 at 12:35:26PM +0200, Ulf Hermann wrote:
> On 04/27/2017 09:41 PM, Mark Wielaard wrote:
> > On Thu, Apr 20, 2017 at 04:54:26PM +0200, Ulf Hermann wrote:
> >> At least one test (dwfl-addr-sect) depends on the order of elf sections
> >> with equal addresses. This is not guaranteed by the code. Compare also
> >> by end address and name to tell entries apart.
> > 
> > O, interesting find. If the start addresses match the order depends on
> > the specific qsort algorithm. So you need a real tie breaker.
> > 
> > I think it is simpler and more predictable if we just take the section
> > number into account. It seem to have the added benefit that it provide
> > the same ordering as before with the glibc qsort, so no testcases need
> > to be adjusted. Does the following work for you?
> > 
> > diff --git a/libdwfl/derelocate.c b/libdwfl/derelocate.c
> > index 439a24e..0d10672 100644
> > --- a/libdwfl/derelocate.c
> > +++ b/libdwfl/derelocate.c
> > @@ -63,7 +63,10 @@ compare_secrefs (const void *a, const void *b)
> >    if ((*p1)->start > (*p2)->start)
> >      return 1;
> >  
> > -  return 0;
> > +  /* Same start address, then just compare which section came first.  */
> > +  size_t n1 = elf_ndxscn ((*p1)->scn);
> > +  size_t n2 = elf_ndxscn ((*p2)->scn);
> > +  return n1 - n2;
> 
> I would inline the whole thing to
> 
> return elf_ndxscn (p1->scn) - elf_ndxscn (p2->scn);
> 
> There is no point in forcing the compiler to keep the intermediate
> numbers as (signed) size_t.

OK.

> Also, I would still keep the check for p1->end and p2->end before this.
> If we have a section of size 0, and another one of size > 0 starting at
> the same place, we want them to be sorted by end address. The zero-sized
> section should be squeezed in before the one that actually has a size,
> not after it.

Aha. I see that we treat the end special in find_section ()

          /* Consider the limit of a section to be inside it, unless it's
             inside the next one.  A section limit address can appear in
             line records.  */
          if (*addr == sections->refs[idx].end
              && idx + 1 < sections->count
              && *addr == sections->refs[idx + 1].start)
            ++idx;

So we do indeed want the zero sized section to be the first.

How about the attached?

Cheers,

Mark

Attachment: 0001-Make-elf-section-sorting-more-deterministic.patch
Description: Text document