From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. Date: Tue, 14 May 2013 10:32:55 +0100 Message-ID: <1368523975.14264.22.camel@zakaz.uk.xensource.com> References: <1368462160.537.125.camel@zakaz.uk.xensource.com> <1368462182-10317-4-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: Keir Fraser , Ian Jackson , "Tim (Xen.org)" , "xen-devel@lists.xen.org" , Julien Grall , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 > > Acked-by: Stefano Stabellini 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.