Re: [GOLD] PowerPC64 TOC indirect to TOC relative code editing

[Alan Modra <amodra at gmail dot com>]

On Mon, Jan 09, 2017 at 09:29:24AM -0800, Cary Coutant wrote:
> > +  if (start == 0)
> > +    start = 1;
> > +  if (start >= shnum)
> > +    return;
> 
> Isn't (start >= shnum) a "can't happen" case? Why not just
> gold_assert(start < shnum)?

No, I don't think so.  An object with shnum 0 or 1 is a valid ELF file
as far as I known.

> > +  unsigned int i = start;
> > +  do
> >      {
> > +      const unsigned char* p = pshdrs + i * This::shdr_size;
> > ...
> > +    } while ((i = i + 1 == shnum ? 1 : i + 1) != start);
> >  }
> 
> Not a fan of that do...while loop. (Hmmm, what happens if shnum == 1?)

shnum == 1 is excluded earlier.

> At the least, the condition could use an extra pair of parens to make
> it more readable. I think I'd prefer the original for loop, running
> from 1..shnum, but add the bias and calculate p at the top of the
> loop.

Huh, I first wrote a patch that used the original for loop and decided
it was cleaner to use do {} while().  You need an extra variable too,
which probably will not be eliminated by a compiler.

  if (start == 0)
    start = 1;
  for (unsigned int i = 1; i < shnum; ++i)
    {
      unsigned int rel_shndx = i + start - 1;
      if (rel_shndx >= shnum)
	rel_shndx = i - shnum + 1;
      const unsigned char* p = pshdrs + rel_shndx * This::shdr_size;

If I keep the do..while, where did you want the parens?  Like this?

    } while ((i = (i + 1 == shnum) ? 1 : i + 1) != start);

Now that you've seen the for loop option, which do you prefer?

-- 
Alan Modra
Australia Development Lab, IBM