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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-05-13 at 17:48 +0100, Stefano Stabellini wrote:
> On Mon, 13 May 2013, Ian Campbell wrote:
> > This bit me midway through a bisect and the platform array shouldn't be
> > empty anymore but lets be safe.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

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.

The proposed fix
    for ( platform = _splatform; platform < _eplatform; platform++ )
is also not correct because due to the same optimisation it always does
at least one iteration.

I expect (although I have not tried) that the compiler would also
optimise an if (_splatform == _eplatform) away too.

We use similar constructs elsewhere (just cherry picking some likely
candidates from a grep for []):
xen/include/asm-x86/percpu.h:extern char __per_cpu_start[], 
__per_cpu_data_end[];
xen/include/asm/uaccess.h:extern struct exception_table_entry 
__start___ex_table[];
xen/include/asm/uaccess.h:extern struct exception_table_entry 
__stop___ex_table[];
xen/include/asm/uaccess.h:extern struct exception_table_entry 
__start___pre_ex_table[];
xen/include/asm/uaccess.h:extern struct exception_table_entry 
__stop___pre_ex_table[];
xen/include/asm-x86/config.h:extern char trampoline_start[], trampoline_end[];
xen/include/xen/kernel.h:extern char _start[], _end[];
xen/include/xen/kernel.h:extern char _stext[], _etext[];
xen/include/xen/kernel.h:extern const char _srodata[], _erodata[];
xen/include/xen/kernel.h:extern char _sinittext[], _einittext[];
xen/arch/x86/setup.c:extern char __init_begin[], __init_end[], __bss_start[];
xen/arch/x86/tboot.c:extern char __init_begin[], __bss_start[];
xen/common/lib.c:extern const ctor_func_t __ctors_start[], __ctors_end[];

I suspect that the code is actually OK (if not strictly
legal/well-defined) if there are actually entries in each table, since
then the two arrays are distinct like the compiler assumes and we
guarantee that the end array is after the start array via the linker
script.

Or maybe we are sitting on a ticking time bomb, waiting for the gcc guys
to get more maliciou^Wclever but at least it is the same bomb as Linux
(which uses this construct extensively too).

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.

Ian J also suggested __attribute__(may_alias) which must be applied to
the struct, or making __foo_start a real variable defined in an ASM
file.

I expect that none of these arrays will ever be empty in the natural
course of things and so the code works. I was just unlucky to trip over
the point in a bisection where some infrastructure had been introduced
but not yet used, but it is now too late to fix that. Probably the right
thing to do is not fret about this for 4.3.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel