Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs.

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2013-05-14 at 10:48 +0100, Jan Beulich wrote:
> >>> On 14.05.13 at 11:32, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > Sadly this turns out to be a bit more complicated :-( CCing Jan and Keir
> > since this was wider implications (this construct is used elsewhere) and
> > Ian J because he pointed this out.
> > 
> > TL;DR: There is probably a problem here, but I'm not sure it is one
> > worth worrying about, at least not yet/for 4.3.
> > 
> > The problem is that the compiler is allowed to assume that:
> >         extern const struct platform_desc _splatform[], _eplatform[];
> > Defines two *distinct* arrays, which therefore can never be equal. Hence
> > it optimises 
> >     for ( platform = _splatform; platform != _eplatform; platform++ )
> > with the assumption that there must always be at least one iteration.
> 
> Did you actually observe gcc doing this?

Yes, leading to an infinite loop.

With my "fixed" version (with the <) it just did one needless iteration
of whatever happened to be at _splatform =_eplatform, which luckily was
accessible data. It happens to be the device data (which likely suffers
the same issue).

I've also semi-confirmed by staring at the disassembly...

>  I agree that the abstract
> C specification allows for such an assumption, but with the gcc
> itself providing ways to create symbols aliases I would be pretty
> surprised if it actually then produces broken code here.
> 
> > FWIW this seems to work:
> >     for ( platform = &_splatform[0]; platform < &_eplatform[0]; platform++ )
> > But I'm not sure it is strictly any less undefined than the original
> > code and it might just be obscuring things enough that today's optimiser
> > misses a trick.
> 
> This surely is as (un)defined as the original code.

As I suspected!

Ian.