* [PATCH 0/4] xen/arm: fixes for 64 bit @ 2013-05-13 16:22 Ian Campbell 2013-05-13 16:22 ` [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files Ian Campbell ` (4 more replies) 0 siblings, 5 replies; 34+ messages in thread From: Ian Campbell @ 2013-05-13 16:22 UTC (permalink / raw) To: xen-devel; +Cc: Julien Grall, Stefano Stabellini, Tim Deegan The following fix a little bit of fallout from Julien's DT series on 64-bit. Ian. ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files 2013-05-13 16:22 [PATCH 0/4] xen/arm: fixes for 64 bit Ian Campbell @ 2013-05-13 16:22 ` Ian Campbell 2013-05-13 16:44 ` Stefano Stabellini 2013-05-13 16:23 ` [PATCH 2/4] xen/arm64: do not clobber callee saved register in early_putch Ian Campbell ` (3 subsequent siblings) 4 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-13 16:22 UTC (permalink / raw) To: xen-devel; +Cc: julien.grall, tim, Ian Campbell, stefano.stabellini Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- xen/arch/arm/arm32/debug.S | 7 +++++++ xen/arch/arm/arm64/debug.S | 7 +++++++ xen/arch/arm/arm64/head.S | 7 +++++++ xen/arch/arm/arm64/mode_switch.S | 7 +++++++ 4 files changed, 28 insertions(+), 0 deletions(-) diff --git a/xen/arch/arm/arm32/debug.S b/xen/arch/arm/arm32/debug.S index 09e5cae..1bfbfc1 100644 --- a/xen/arch/arm/arm32/debug.S +++ b/xen/arch/arm/arm32/debug.S @@ -31,3 +31,10 @@ early_putch: early_uart_ready r1, r2 early_uart_transmit r1, r0 mov pc, lr + +/* + * Local variables: + * mode: ASM + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/arm64/debug.S b/xen/arch/arm/arm64/debug.S index 48a6567..f0c7761 100644 --- a/xen/arch/arm/arm64/debug.S +++ b/xen/arch/arm/arm64/debug.S @@ -31,3 +31,10 @@ early_putch: early_uart_ready x23, 1 early_uart_transmit x23, w0 ret + +/* + * Local variables: + * mode: ASM + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 79a8da9..8955946 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -384,3 +384,10 @@ puts: putn: ret #endif /* EARLY_PRINTK */ + +/* + * Local variables: + * mode: ASM + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/arm64/mode_switch.S b/xen/arch/arm/arm64/mode_switch.S index d115706..4125ac4 100644 --- a/xen/arch/arm/arm64/mode_switch.S +++ b/xen/arch/arm/arm64/mode_switch.S @@ -82,3 +82,10 @@ enter_el2_mode: mov x1, #0x3c9 // EL2_SP1 | D | A | I | F msr spsr_el3, x1 eret + +/* + * Local variables: + * mode: ASM + * indent-tabs-mode: nil + * End: + */ -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files 2013-05-13 16:22 ` [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files Ian Campbell @ 2013-05-13 16:44 ` Stefano Stabellini 2013-05-13 17:35 ` Tim Deegan 0 siblings, 1 reply; 34+ messages in thread From: Stefano Stabellini @ 2013-05-13 16:44 UTC (permalink / raw) To: Ian Campbell; +Cc: Julien Grall, Stefano Stabellini, Tim (Xen.org), xen-devel On Mon, 13 May 2013, Ian Campbell wrote: > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> I won't ack anything emacs related as a matter of principle ;-) > xen/arch/arm/arm32/debug.S | 7 +++++++ > xen/arch/arm/arm64/debug.S | 7 +++++++ > xen/arch/arm/arm64/head.S | 7 +++++++ > xen/arch/arm/arm64/mode_switch.S | 7 +++++++ > 4 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/xen/arch/arm/arm32/debug.S b/xen/arch/arm/arm32/debug.S > index 09e5cae..1bfbfc1 100644 > --- a/xen/arch/arm/arm32/debug.S > +++ b/xen/arch/arm/arm32/debug.S > @@ -31,3 +31,10 @@ early_putch: > early_uart_ready r1, r2 > early_uart_transmit r1, r0 > mov pc, lr > + > +/* > + * Local variables: > + * mode: ASM > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/arch/arm/arm64/debug.S b/xen/arch/arm/arm64/debug.S > index 48a6567..f0c7761 100644 > --- a/xen/arch/arm/arm64/debug.S > +++ b/xen/arch/arm/arm64/debug.S > @@ -31,3 +31,10 @@ early_putch: > early_uart_ready x23, 1 > early_uart_transmit x23, w0 > ret > + > +/* > + * Local variables: > + * mode: ASM > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 79a8da9..8955946 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -384,3 +384,10 @@ puts: > putn: ret > > #endif /* EARLY_PRINTK */ > + > +/* > + * Local variables: > + * mode: ASM > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/arch/arm/arm64/mode_switch.S b/xen/arch/arm/arm64/mode_switch.S > index d115706..4125ac4 100644 > --- a/xen/arch/arm/arm64/mode_switch.S > +++ b/xen/arch/arm/arm64/mode_switch.S > @@ -82,3 +82,10 @@ enter_el2_mode: > mov x1, #0x3c9 // EL2_SP1 | D | A | I | F > msr spsr_el3, x1 > eret > + > +/* > + * Local variables: > + * mode: ASM > + * indent-tabs-mode: nil > + * End: > + */ > -- > 1.7.2.5 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files 2013-05-13 16:44 ` Stefano Stabellini @ 2013-05-13 17:35 ` Tim Deegan 0 siblings, 0 replies; 34+ messages in thread From: Tim Deegan @ 2013-05-13 17:35 UTC (permalink / raw) To: Stefano Stabellini; +Cc: Julien Grall, Ian Campbell, xen-devel At 17:44 +0100 on 13 May (1368467079), Stefano Stabellini wrote: > On Mon, 13 May 2013, Ian Campbell wrote: > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > I won't ack anything emacs related as a matter of principle ;-) Just wait till you need an ack for a vi-related change. :) (Acked-by: Tim Deegan <tim@xen.org>) ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/4] xen/arm64: do not clobber callee saved register in early_putch 2013-05-13 16:22 [PATCH 0/4] xen/arm: fixes for 64 bit Ian Campbell 2013-05-13 16:22 ` [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files Ian Campbell @ 2013-05-13 16:23 ` Ian Campbell 2013-05-13 16:53 ` Stefano Stabellini 2013-05-13 16:23 ` [PATCH 3/4] xen/arm: support "arm, armv8-timer" DTS compatibility Ian Campbell ` (2 subsequent siblings) 4 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-13 16:23 UTC (permalink / raw) To: xen-devel; +Cc: julien.grall, tim, Ian Campbell, stefano.stabellini x23 is callee saved in the ARM 64 bit calling convention. Use x15 instead which is a temporary register which need not be preserved. Fixes a random crash during boot. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- xen/arch/arm/arm64/debug.S | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/arch/arm/arm64/debug.S b/xen/arch/arm/arm64/debug.S index f0c7761..38b7c74 100644 --- a/xen/arch/arm/arm64/debug.S +++ b/xen/arch/arm/arm64/debug.S @@ -27,9 +27,9 @@ /* Print a character on the UART - this function is called by C * x0: character to print */ early_putch: - ldr x23, =FIXMAP_ADDR(FIXMAP_CONSOLE) - early_uart_ready x23, 1 - early_uart_transmit x23, w0 + ldr x15, =FIXMAP_ADDR(FIXMAP_CONSOLE) + early_uart_ready x15, 1 + early_uart_transmit x15, w0 ret /* -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 2/4] xen/arm64: do not clobber callee saved register in early_putch 2013-05-13 16:23 ` [PATCH 2/4] xen/arm64: do not clobber callee saved register in early_putch Ian Campbell @ 2013-05-13 16:53 ` Stefano Stabellini 0 siblings, 0 replies; 34+ messages in thread From: Stefano Stabellini @ 2013-05-13 16:53 UTC (permalink / raw) To: Ian Campbell; +Cc: Julien Grall, Stefano Stabellini, Tim (Xen.org), xen-devel On Mon, 13 May 2013, Ian Campbell wrote: > x23 is callee saved in the ARM 64 bit calling convention. Use x15 instead > which is a temporary register which need not be preserved. > > Fixes a random crash during boot. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > xen/arch/arm/arm64/debug.S | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/arm64/debug.S b/xen/arch/arm/arm64/debug.S > index f0c7761..38b7c74 100644 > --- a/xen/arch/arm/arm64/debug.S > +++ b/xen/arch/arm/arm64/debug.S > @@ -27,9 +27,9 @@ > /* Print a character on the UART - this function is called by C > * x0: character to print */ > early_putch: > - ldr x23, =FIXMAP_ADDR(FIXMAP_CONSOLE) > - early_uart_ready x23, 1 > - early_uart_transmit x23, w0 > + ldr x15, =FIXMAP_ADDR(FIXMAP_CONSOLE) > + early_uart_ready x15, 1 > + early_uart_transmit x15, w0 > ret > > /* > -- > 1.7.2.5 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/4] xen/arm: support "arm, armv8-timer" DTS compatibility. 2013-05-13 16:22 [PATCH 0/4] xen/arm: fixes for 64 bit Ian Campbell 2013-05-13 16:22 ` [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files Ian Campbell 2013-05-13 16:23 ` [PATCH 2/4] xen/arm64: do not clobber callee saved register in early_putch Ian Campbell @ 2013-05-13 16:23 ` Ian Campbell 2013-05-13 16:46 ` Stefano Stabellini 2013-05-13 16:23 ` [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs Ian Campbell 2013-05-14 9:36 ` [PATCH 0/4] xen/arm: fixes for 64 bit Ian Campbell 4 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-13 16:23 UTC (permalink / raw) To: xen-devel; +Cc: julien.grall, tim, Ian Campbell, stefano.stabellini On v8 the compatibility name differs but the node is otherwise specified the same. See linux/Documentation/devicetree/bindings/arm/arch_timer.txt Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- xen/arch/arm/time.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index db849cf..4ed7882 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -107,6 +107,8 @@ int __init init_xen_time(void) dev = dt_find_compatible_node(NULL, NULL, "arm,armv7-timer"); if ( !dev ) + dev = dt_find_compatible_node(NULL, NULL, "arm,armv8-timer"); + if ( !dev ) panic("Unable to find a compatible timer in the device tree\n"); dt_device_set_used_by(dev, DOMID_XEN); -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 3/4] xen/arm: support "arm, armv8-timer" DTS compatibility. 2013-05-13 16:23 ` [PATCH 3/4] xen/arm: support "arm, armv8-timer" DTS compatibility Ian Campbell @ 2013-05-13 16:46 ` Stefano Stabellini 0 siblings, 0 replies; 34+ messages in thread From: Stefano Stabellini @ 2013-05-13 16:46 UTC (permalink / raw) To: Ian Campbell; +Cc: Julien Grall, Stefano Stabellini, Tim (Xen.org), xen-devel On Mon, 13 May 2013, Ian Campbell wrote: > On v8 the compatibility name differs but the node is otherwise specified the > same. See linux/Documentation/devicetree/bindings/arm/arch_timer.txt > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > xen/arch/arm/time.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c > index db849cf..4ed7882 100644 > --- a/xen/arch/arm/time.c > +++ b/xen/arch/arm/time.c > @@ -107,6 +107,8 @@ int __init init_xen_time(void) > > dev = dt_find_compatible_node(NULL, NULL, "arm,armv7-timer"); > if ( !dev ) > + dev = dt_find_compatible_node(NULL, NULL, "arm,armv8-timer"); > + if ( !dev ) > panic("Unable to find a compatible timer in the device tree\n"); > > dt_device_set_used_by(dev, DOMID_XEN); > -- > 1.7.2.5 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-13 16:22 [PATCH 0/4] xen/arm: fixes for 64 bit Ian Campbell ` (2 preceding siblings ...) 2013-05-13 16:23 ` [PATCH 3/4] xen/arm: support "arm, armv8-timer" DTS compatibility Ian Campbell @ 2013-05-13 16:23 ` Ian Campbell 2013-05-13 16:48 ` Stefano Stabellini 2013-05-14 9:36 ` [PATCH 0/4] xen/arm: fixes for 64 bit Ian Campbell 4 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-13 16:23 UTC (permalink / raw) To: xen-devel; +Cc: julien.grall, tim, Ian Campbell, stefano.stabellini 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@citrix.com> --- xen/arch/arm/platform.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c index afda302..1335110 100644 --- a/xen/arch/arm/platform.c +++ b/xen/arch/arm/platform.c @@ -50,7 +50,7 @@ static void dump_platform_table(void) printk("Available platform support:\n"); - for ( p = _splatform; p != _eplatform; p++ ) + for ( p = _splatform; p < _eplatform; p++ ) printk(" - %s\n", p->name); } @@ -61,7 +61,7 @@ int __init platform_init(void) ASSERT(platform == NULL); /* Looking for the platform description */ - for ( platform = _splatform; platform != _eplatform; platform++ ) + for ( platform = _splatform; platform < _eplatform; platform++ ) { if ( platform_is_compatible(platform) ) break; -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-13 16:23 ` [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs Ian Campbell @ 2013-05-13 16:48 ` Stefano Stabellini 2013-05-14 9:32 ` Ian Campbell 0 siblings, 1 reply; 34+ messages in thread From: Stefano Stabellini @ 2013-05-13 16:48 UTC (permalink / raw) To: Ian Campbell; +Cc: Julien Grall, Stefano Stabellini, Tim (Xen.org), xen-devel 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@citrix.com> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > xen/arch/arm/platform.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c > index afda302..1335110 100644 > --- a/xen/arch/arm/platform.c > +++ b/xen/arch/arm/platform.c > @@ -50,7 +50,7 @@ static void dump_platform_table(void) > > printk("Available platform support:\n"); > > - for ( p = _splatform; p != _eplatform; p++ ) > + for ( p = _splatform; p < _eplatform; p++ ) > printk(" - %s\n", p->name); > } > > @@ -61,7 +61,7 @@ int __init platform_init(void) > ASSERT(platform == NULL); > > /* Looking for the platform description */ > - for ( platform = _splatform; platform != _eplatform; platform++ ) > + for ( platform = _splatform; platform < _eplatform; platform++ ) > { > if ( platform_is_compatible(platform) ) > break; > -- > 1.7.2.5 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-13 16:48 ` Stefano Stabellini @ 2013-05-14 9:32 ` Ian Campbell 2013-05-14 9:48 ` Jan Beulich 0 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-14 9:32 UTC (permalink / raw) To: Stefano Stabellini Cc: Keir Fraser, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall, Jan Beulich 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@citrix.com> > > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 9:32 ` Ian Campbell @ 2013-05-14 9:48 ` Jan Beulich 2013-05-14 10:21 ` Ian Campbell 0 siblings, 1 reply; 34+ messages in thread From: Jan Beulich @ 2013-05-14 9:48 UTC (permalink / raw) To: Ian Campbell, Stefano Stabellini Cc: Julien Grall, Keir Fraser, Tim (Xen.org), Ian Jackson, xen-devel >>> 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? 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. Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 9:48 ` Jan Beulich @ 2013-05-14 10:21 ` Ian Campbell 2013-05-14 14:29 ` Jan Beulich 0 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-14 10:21 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 10:21 ` Ian Campbell @ 2013-05-14 14:29 ` Jan Beulich 2013-05-14 14:37 ` Jan Beulich 2013-05-14 14:54 ` Ian Campbell 0 siblings, 2 replies; 34+ messages in thread From: Jan Beulich @ 2013-05-14 14:29 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 14.05.13 at 12:21, Ian Campbell <Ian.Campbell@citrix.com> wrote: > 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... Just compiled (-O2) this extern unsigned _s[], _e[]; void u(unsigned*); void test(void) { unsigned*p; for(p = _s; p != _e; ++p) u(p); } with gcc 4.7.3 for ix86, x86-64, and arm32, and none of them produced an infinite loop. And as said before, I'd be surprised a non-buggy gcc would, considering that with empty structures (which gcc allows) you can have zero-sized objects, and hence two named objects at the same address. And similarly, they support attributes to create symbol aliases, and for those it would again be necessary to account for the addresses of two distinct symbols to actually resolve to the same address. What compiler version and switches were you using for the case where you spotted bad code to be generated? Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 14:29 ` Jan Beulich @ 2013-05-14 14:37 ` Jan Beulich 2013-05-14 14:54 ` Ian Campbell 1 sibling, 0 replies; 34+ messages in thread From: Jan Beulich @ 2013-05-14 14:37 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 14.05.13 at 16:29, "Jan Beulich" <JBeulich@suse.com> wrote: >>>> On 14.05.13 at 12:21, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> 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... > > Just compiled (-O2) this > > extern unsigned _s[], _e[]; > > void u(unsigned*); > > void test(void) { > unsigned*p; > > for(p = _s; p != _e; ++p) > u(p); > } > > with gcc 4.7.3 for ix86, x86-64, and arm32, and none of them > produced an infinite loop. And as said before, I'd be surprised a > non-buggy gcc would, considering that with empty structures > (which gcc allows) you can have zero-sized objects, and hence > two named objects at the same address. And similarly, they > support attributes to create symbol aliases, and for those it > would again be necessary to account for the addresses of two > distinct symbols to actually resolve to the same address. Actually, the part about structures without members was wrong - gcc allocates a byte for each instance nevertheless (albeit it also only allocates a single byte for an array of such structures, i.e. the individual array members are indistinguishable from their addresses). Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 14:29 ` Jan Beulich 2013-05-14 14:37 ` Jan Beulich @ 2013-05-14 14:54 ` Ian Campbell 2013-05-14 14:56 ` Ian Campbell 2013-05-14 15:11 ` Jan Beulich 1 sibling, 2 replies; 34+ messages in thread From: Ian Campbell @ 2013-05-14 14:54 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall On Tue, 2013-05-14 at 15:29 +0100, Jan Beulich wrote: > >>> On 14.05.13 at 12:21, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > 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... > > Just compiled (-O2) this > > extern unsigned _s[], _e[]; > > void u(unsigned*); > > void test(void) { > unsigned*p; > > for(p = _s; p != _e; ++p) > u(p); > } > > with gcc 4.7.3 for ix86, x86-64, and arm32, and none of them > produced an infinite loop. And as said before, I'd be surprised a > non-buggy gcc would, considering that with empty structures > (which gcc allows) you can have zero-sized objects, and hence > two named objects at the same address. And similarly, they > support attributes to create symbol aliases, and for those it > would again be necessary to account for the addresses of two > distinct symbols to actually resolve to the same address. > > What compiler version and switches were you using for the case > where you spotted bad code to be generated? gcc-linaro-aarch64-linux-gnu-4.7-2013.01-20130125_linux doing a standard arm64 build of Xen. With the attached patch with the original != check in the for loop: (XEN) Platforms: 0000000000265478-0000000000265478 (XEN) Considering platform 0000000000265478 = PL011 UART (XEN) Considering platform 00000000002654b8 = <NULL> (XEN) Considering platform 00000000002654f8 = <NULL> (XEN) Considering platform 0000000000265538 = <NULL> (XEN) Considering platform 0000000000265578 = <NULL> (XEN) Considering platform 00000000002655b8 = <NULL> (XEN) Considering platform 00000000002655f8 = <NULL> If I use < in the for loop instead I get: (XEN) Platforms: 0000000000265478-0000000000265478 (XEN) Finished with platform 0000000000265478 = PL011 UART (XEN) WARNING: Unrecognized/unsupported device tree compatible list (so it seems I was wrong about getting a spurious check of zeroeth element in this case) I also just tried the same with arm32 and got: (XEN) Platforms: 0025fdc8-0025fdc8 (XEN) Considering platform 0025fdc8 = PL011 UART (XEN) Considering platform 0025fde8 = <NULL> (XEN) Considering platform 0025fe08 = <NULL> (XEN) Considering platform 0025fe28 = <NULL> (XEN) Considering platform 0025fe48 = <NULL> or: (XEN) Platforms: 0025fdc8-0025fdc8 (XEN) Finished with platform 0025fdc8 = PL011 UART (XEN) WARNING: Unrecognized/unsupported device tree compatible list The test patch, mostly concerned with removing platforms to force an empty list: diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c index 1335110..f12947e 100644 --- a/xen/arch/arm/platform.c +++ b/xen/arch/arm/platform.c @@ -60,13 +60,18 @@ int __init platform_init(void) ASSERT(platform == NULL); + printk("Platforms: %p-%p\n", _splatform, _eplatform); + /* Looking for the platform description */ - for ( platform = _splatform; platform < _eplatform; platform++ ) + for ( platform = _splatform; platform != _eplatform; platform++ ) { + printk(" Considering platform %p = %s\n", platform, platform->name); if ( platform_is_compatible(platform) ) break; } + printk("Finished with platform %p = %s\n", platform, platform->name); + /* We don't have specific operations for this platform */ if ( platform == _eplatform ) { diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile index ff2b65b..358ec70 100644 --- a/xen/arch/arm/platforms/Makefile +++ b/xen/arch/arm/platforms/Makefile @@ -1,2 +1,2 @@ obj-y += vexpress.o -obj-y += exynos5.o +#obj-y += exynos5.o diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c index 8fc30c4..f689d7a 100644 --- a/xen/arch/arm/platforms/vexpress.c +++ b/xen/arch/arm/platforms/vexpress.c @@ -92,6 +92,7 @@ out: return ret; } +#if 0 /* * TODO: Get base address from the device tree * See arm,vexpress-reset node @@ -129,6 +130,7 @@ PLATFORM_START(vexpress, "VERSATILE EXPRESS") .compatible = vexpress_dt_compat, .reset = vexpress_reset, PLATFORM_END +#endif /* * Local variables: ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 14:54 ` Ian Campbell @ 2013-05-14 14:56 ` Ian Campbell 2013-05-14 15:07 ` Ian Campbell 2013-05-14 15:11 ` Jan Beulich 1 sibling, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-14 14:56 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall On Tue, 2013-05-14 at 15:54 +0100, Ian Campbell wrote: > > What compiler version and switches were you using for the case > > where you spotted bad code to be generated? > > gcc-linaro-aarch64-linux-gnu-4.7-2013.01-20130125_linux doing a standard > arm64 build of Xen. Flags: aarch64-linux-gnu-gcc -O1 -fno-omit-frame-pointer -g -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -fno-builtin -fno-common -Wredundant-decls -iwithprefix include -Werror -Wno-pointer-arith -pipe -I/local/scratch/ianc/devel/arm/xen.git/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -mcpu=generic -DGCC_HAS_VISIBILITY_ATTRIBUTE -fno-optimize-sibling-calls -DEARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" -g -D__XEN__ -include /local/scratch/ianc/devel/arm/xen.git/xen/include/xen/config.h -DVERBOSE -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER -MMD -MF .platform.o.d -c platform.c -o platform.o Interestingly that includes -fno-strict-aliasing, which comes from the top-level Config.mk ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 14:56 ` Ian Campbell @ 2013-05-14 15:07 ` Ian Campbell 2013-05-14 15:46 ` Jan Beulich 0 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-14 15:07 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Tim (Xen.org), Ian Jackson, xen-devel, Julien Grall On Tue, 2013-05-14 at 15:56 +0100, Ian Campbell wrote: > On Tue, 2013-05-14 at 15:54 +0100, Ian Campbell wrote: > > > What compiler version and switches were you using for the case > > > where you spotted bad code to be generated? > > > > gcc-linaro-aarch64-linux-gnu-4.7-2013.01-20130125_linux doing a standard > > arm64 build of Xen. > > Flags: > > aarch64-linux-gnu-gcc -O1 -fno-omit-frame-pointer -g > -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes > -Wdeclaration-after-statement -Wno-unused-but-set-variable > -Wno-unused-local-typedefs -fno-builtin -fno-common -Wredundant-decls > -iwithprefix include -Werror -Wno-pointer-arith -pipe > -I/local/scratch/ianc/devel/arm/xen.git/xen/include -fno-stack-protector > -fno-exceptions -Wnested-externs -mcpu=generic > -DGCC_HAS_VISIBILITY_ATTRIBUTE -fno-optimize-sibling-calls > -DEARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" -g -D__XEN__ > -include /local/scratch/ianc/devel/arm/xen.git/xen/include/xen/config.h > -DVERBOSE -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER -MMD > -MF .platform.o.d -c platform.c -o platform.o > > Interestingly that includes -fno-strict-aliasing, which comes from the > top-level Config.mk Compiling your test snippet with that I get: 0000000000000000 <test>: 0: a9be7bfd stp x29, x30, [sp,#-32]! 4: 910003fd mov x29, sp 8: a90153f3 stp x19, x20, [sp,#16] c: 90000013 adrp x19, 0 <_s> 10: 91000273 add x19, x19, #0x0 14: 90000014 adrp x20, 0 <_e> 18: 91000294 add x20, x20, #0x0 1c: aa1303e0 mov x0, x19 20: 94000000 bl 0 <u> 24: 91001273 add x19, x19, #0x4 28: eb14027f cmp x19, x20 2c: 54ffff81 b.ne 1c <test+0x1c> 30: a94153f3 ldp x19, x20, [sp,#16] 34: a8c27bfd ldp x29, x30, [sp],#32 38: d65f03c0 ret You can see it has pushed the branch at 0x2c to after the first iteration... But if I remove only -DGCC_HAS_VISIBILITY_ATTRIBUTE the issue goes away. Which makes no sense because the test code has no use of that, but Aha: -include /local/scratch/ianc/devel/arm/xen.git/xen/include/xen/config.h Which eventually gets us, from compiler.h: #ifdef GCC_HAS_VISIBILITY_ATTRIBUTE /* Results in more efficient PIC code (no indirections through GOT or PLT). */ #pragma GCC visibility push(hidden) #endif I've not looked up the precise effect of that pragma yet... Ian. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 15:07 ` Ian Campbell @ 2013-05-14 15:46 ` Jan Beulich 2013-05-14 16:41 ` Ian Campbell 0 siblings, 1 reply; 34+ messages in thread From: Jan Beulich @ 2013-05-14 15:46 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 14.05.13 at 17:07, Ian Campbell <Ian.Campbell@citrix.com> wrote: > But if I remove only -DGCC_HAS_VISIBILITY_ATTRIBUTE the issue goes away. > > Which makes no sense because the test code has no use of that, but Aha: > -include > /local/scratch/ianc/devel/arm/xen.git/xen/include/xen/config.h > > Which eventually gets us, from compiler.h: > #ifdef GCC_HAS_VISIBILITY_ATTRIBUTE > /* Results in more efficient PIC code (no indirections through GOT or PLT). > */ > #pragma GCC visibility push(hidden) > #endif That's a very interesting (to me at least) side effect of applying hidden visibility to symbols. Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 15:46 ` Jan Beulich @ 2013-05-14 16:41 ` Ian Campbell 2013-05-15 7:12 ` Jan Beulich 0 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-14 16:41 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall On Tue, 2013-05-14 at 16:46 +0100, Jan Beulich wrote: > >>> On 14.05.13 at 17:07, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > But if I remove only -DGCC_HAS_VISIBILITY_ATTRIBUTE the issue goes away. > > > > Which makes no sense because the test code has no use of that, but Aha: > > -include > > /local/scratch/ianc/devel/arm/xen.git/xen/include/xen/config.h > > > > Which eventually gets us, from compiler.h: > > #ifdef GCC_HAS_VISIBILITY_ATTRIBUTE > > /* Results in more efficient PIC code (no indirections through GOT or PLT). > > */ > > #pragma GCC visibility push(hidden) > > #endif > > That's a very interesting (to me at least) side effect of applying > hidden visibility to symbols. Indeed, me neither. I pruned all the other options and it is exactly this pragma plus -O* which causes this behaviour, even on x86. Ian. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 16:41 ` Ian Campbell @ 2013-05-15 7:12 ` Jan Beulich 2013-05-15 9:47 ` Ian Campbell 0 siblings, 1 reply; 34+ messages in thread From: Jan Beulich @ 2013-05-15 7:12 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 14.05.13 at 18:41, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Tue, 2013-05-14 at 16:46 +0100, Jan Beulich wrote: >> >>> On 14.05.13 at 17:07, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > But if I remove only -DGCC_HAS_VISIBILITY_ATTRIBUTE the issue goes away. >> > >> > Which makes no sense because the test code has no use of that, but Aha: >> > -include >> > /local/scratch/ianc/devel/arm/xen.git/xen/include/xen/config.h >> > >> > Which eventually gets us, from compiler.h: >> > #ifdef GCC_HAS_VISIBILITY_ATTRIBUTE >> > /* Results in more efficient PIC code (no indirections through GOT or PLT). > >> > */ >> > #pragma GCC visibility push(hidden) >> > #endif >> >> That's a very interesting (to me at least) side effect of applying >> hidden visibility to symbols. > > Indeed, me neither. > > I pruned all the other options and it is exactly this pragma plus -O* > which causes this behaviour, even on x86. Yes, I had double checked this too, by selectively attaching __attribute__((__visibility__())) to either or both of the involved symbols. Once present on both, any visibility other than "default" has this effect. As Linux doesn't play with symbol visibility, we're really alone having this problem, and hence indeed need to go through and audit all places where the potential for empty ranges exists. But you said "<" instead of "!=" didn't have any surprising side effects, so quite likely we won't need much code to change. Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-15 7:12 ` Jan Beulich @ 2013-05-15 9:47 ` Ian Campbell 2013-05-15 12:19 ` Jan Beulich 0 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-15 9:47 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall > As Linux doesn't play with symbol visibility, we're really alone > having this problem, and hence indeed need to go through and > audit all places where the potential for empty ranges exists. The comment says "Results in more efficient PIC code (no indirections through GOT or PLT).", how much PIC code is there in the hypervisor anyway? > But you said "<" instead of "!=" didn't have any surprising side > effects, so quite likely we won't need much code to change. "<" fails because it does process the (non-existent) first entry in the array. This happened to be "safe" in the case I saw but it wouldn't be in general. Ian. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-15 9:47 ` Ian Campbell @ 2013-05-15 12:19 ` Jan Beulich 2013-05-15 13:47 ` Ian Campbell 0 siblings, 1 reply; 34+ messages in thread From: Jan Beulich @ 2013-05-15 12:19 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 15.05.13 at 11:47, Ian Campbell <ian.campbell@citrix.com> wrote: >> As Linux doesn't play with symbol visibility, we're really alone >> having this problem, and hence indeed need to go through and >> audit all places where the potential for empty ranges exists. > > The comment says "Results in more efficient PIC code (no indirections > through GOT or PLT).", how much PIC code is there in the hypervisor > anyway? All x86-64 code has been built with -fpic from the beginning. That likely was the easiest (and possibly the only available) solution to not being able to use -mcmodel=kernel. >> But you said "<" instead of "!=" didn't have any surprising side >> effects, so quite likely we won't need much code to change. > > "<" fails because it does process the (non-existent) first entry in the > array. This happened to be "safe" in the case I saw but it wouldn't be > in general. Okay, I misread one of your earlier responses then. Did you do the necessary auditing already, or should I put this on my todo list? Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-15 12:19 ` Jan Beulich @ 2013-05-15 13:47 ` Ian Campbell 2013-05-16 10:17 ` Jan Beulich 0 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-15 13:47 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall On Wed, 2013-05-15 at 13:19 +0100, Jan Beulich wrote: > >>> On 15.05.13 at 11:47, Ian Campbell <ian.campbell@citrix.com> wrote: > > >> As Linux doesn't play with symbol visibility, we're really alone > >> having this problem, and hence indeed need to go through and > >> audit all places where the potential for empty ranges exists. > > > > The comment says "Results in more efficient PIC code (no indirections > > through GOT or PLT).", how much PIC code is there in the hypervisor > > anyway? > > All x86-64 code has been built with -fpic from the beginning. That > likely was the easiest (and possibly the only available) solution to > not being able to use -mcmodel=kernel. I should probably investigate not doing this for ARM, where we don't use fpic, then! > >> But you said "<" instead of "!=" didn't have any surprising side > >> effects, so quite likely we won't need much code to change. > > > > "<" fails because it does process the (non-existent) first entry in the > > array. This happened to be "safe" in the case I saw but it wouldn't be > > in general. > > Okay, I misread one of your earlier responses then. Did you do > the necessary auditing already, or should I put this on my todo > list? I haven't done an audit. I put a very quicly grepped list in a previous mail but it is surely incomplete. Ian. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-15 13:47 ` Ian Campbell @ 2013-05-16 10:17 ` Jan Beulich 2013-05-17 10:07 ` Ian Campbell 0 siblings, 1 reply; 34+ messages in thread From: Jan Beulich @ 2013-05-16 10:17 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 15.05.13 at 15:47, Ian Campbell <ian.campbell@citrix.com> wrote: > On Wed, 2013-05-15 at 13:19 +0100, Jan Beulich wrote: >> >>> On 15.05.13 at 11:47, Ian Campbell <ian.campbell@citrix.com> wrote: >> > "<" fails because it does process the (non-existent) first entry in the >> > array. This happened to be "safe" in the case I saw but it wouldn't be >> > in general. >> >> Okay, I misread one of your earlier responses then. Did you do >> the necessary auditing already, or should I put this on my todo >> list? > > I haven't done an audit. I put a very quicly grepped list in a previous > mail but it is surely incomplete. So I went through all of them - the only other ones that can be potentially empty are .ctors and .xsm_initcalls.init (I didn't check whether ARM has any guaranteed .ex_table.pre uses though). Both use "<", and the compiler translates this safely on x86. My ARM assembly skills are still lacking, but afaict the early exit being done with "popcs" / "b.cs" should be safe too, as they cover the "==" case (APSR.C being set for x <= y). Thus I wonder what code you saw being generated for the "<" case... And btw., for both 32- and 64-bit ARM, other than for x86, I see empty structure instances occupy zero bytes (and hence distinct symbols end up at the same address), so the compiler is conflicting with itself here. Jan extern unsigned __attribute__((__visibility__("hidden"))) _s[]; extern unsigned __attribute__((__visibility__("internal"))) _e[]; struct s {} s0 = {}, s1 = {}, sa[4] = {}; void u(unsigned*); void s(struct s*); void test1(void) { unsigned*p; for(p = _s; p != _e; ++p) u(p); } void test2(void) { unsigned*p; for(p = _s; p < _e; ++p) u(p); } void test3(void) { s(&s0); s(&s1); s(sa); s(sa + 3); } ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-16 10:17 ` Jan Beulich @ 2013-05-17 10:07 ` Ian Campbell 2013-05-17 10:39 ` Jan Beulich 0 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-17 10:07 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall On Thu, 2013-05-16 at 11:17 +0100, Jan Beulich wrote: > >>> On 15.05.13 at 15:47, Ian Campbell <ian.campbell@citrix.com> wrote: > > On Wed, 2013-05-15 at 13:19 +0100, Jan Beulich wrote: > >> >>> On 15.05.13 at 11:47, Ian Campbell <ian.campbell@citrix.com> wrote: > >> > "<" fails because it does process the (non-existent) first entry in the > >> > array. This happened to be "safe" in the case I saw but it wouldn't be > >> > in general. > >> > >> Okay, I misread one of your earlier responses then. Did you do > >> the necessary auditing already, or should I put this on my todo > >> list? > > > > I haven't done an audit. I put a very quicly grepped list in a previous > > mail but it is surely incomplete. > > So I went through all of them - the only other ones that can be > potentially empty are .ctors and .xsm_initcalls.init (I didn't check > whether ARM has any guaranteed .ex_table.pre uses though). On a random arm64 binary which I have here both ex_table and ex_table.pre are empty... > Both use "<", and the compiler translates this safely on x86. My > ARM assembly skills are still lacking, but afaict the early exit being > done with "popcs" / "b.cs" should be safe too, as they cover the > "==" case (APSR.C being set for x <= y). Thus I wonder what > code you saw being generated for the "<" case... 00000000 <test>: 0: e92d4038 push {r3, r4, r5, lr} 4: e59f4020 ldr r4, [pc, #32] ; 2c <test+0x2c> 8: e59f5020 ldr r5, [pc, #32] ; 30 <test+0x30> c: e1540005 cmp r4, r5 10: 28bd8038 popcs {r3, r4, r5, pc} 14: e1a00004 mov r0, r4 18: e2844004 add r4, r4, #4 1c: ebfffffe bl 0 <u> 20: e1540005 cmp r4, r5 24: 3afffffa bcc 14 <test+0x14> 28: e8bd8038 pop {r3, r4, r5, pc} So indeed I think you are correct that the popcs will do the right thing, I obviously missed the update of PC via that instruction when I looked at this before. > And btw., for both 32- and 64-bit ARM, other than for x86, I see > empty structure instances occupy zero bytes (and hence distinct > symbols end up at the same address), so the compiler is conflicting > with itself here. I imagine this is as much to do with the architecture ABI as the compiler. Ian ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-17 10:07 ` Ian Campbell @ 2013-05-17 10:39 ` Jan Beulich 2013-05-17 14:34 ` Ian Campbell 0 siblings, 1 reply; 34+ messages in thread From: Jan Beulich @ 2013-05-17 10:39 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 17.05.13 at 12:07, Ian Campbell <ian.campbell@citrix.com> wrote: > On Thu, 2013-05-16 at 11:17 +0100, Jan Beulich wrote: >> >>> On 15.05.13 at 15:47, Ian Campbell <ian.campbell@citrix.com> wrote: >> > On Wed, 2013-05-15 at 13:19 +0100, Jan Beulich wrote: >> >> >>> On 15.05.13 at 11:47, Ian Campbell <ian.campbell@citrix.com> wrote: >> >> > "<" fails because it does process the (non-existent) first entry in the >> >> > array. This happened to be "safe" in the case I saw but it wouldn't be >> >> > in general. >> >> >> >> Okay, I misread one of your earlier responses then. Did you do >> >> the necessary auditing already, or should I put this on my todo >> >> list? >> > >> > I haven't done an audit. I put a very quicly grepped list in a previous >> > mail but it is surely incomplete. >> >> So I went through all of them - the only other ones that can be >> potentially empty are .ctors and .xsm_initcalls.init (I didn't check >> whether ARM has any guaranteed .ex_table.pre uses though). > > On a random arm64 binary which I have here both ex_table and > ex_table.pre are empty... Indeed. Yet arch/arm/ also has no reference to __{start,stop}__{,_pre}_ex_table, so this is no problem. Out of curiosity - there's nothing you ever do in ARM on behalf of the user than can fault? >> Both use "<", and the compiler translates this safely on x86. My >> ARM assembly skills are still lacking, but afaict the early exit being >> done with "popcs" / "b.cs" should be safe too, as they cover the >> "==" case (APSR.C being set for x <= y). Thus I wonder what >> code you saw being generated for the "<" case... > > 00000000 <test>: > 0: e92d4038 push {r3, r4, r5, lr} > 4: e59f4020 ldr r4, [pc, #32] ; 2c <test+0x2c> > 8: e59f5020 ldr r5, [pc, #32] ; 30 <test+0x30> > c: e1540005 cmp r4, r5 > 10: 28bd8038 popcs {r3, r4, r5, pc} > 14: e1a00004 mov r0, r4 > 18: e2844004 add r4, r4, #4 > 1c: ebfffffe bl 0 <u> > 20: e1540005 cmp r4, r5 > 24: 3afffffa bcc 14 <test+0x14> > 28: e8bd8038 pop {r3, r4, r5, pc} > > So indeed I think you are correct that the popcs will do the right > thing, I obviously missed the update of PC via that instruction when I > looked at this before. But you still said that in practice you observed one unwanted iteration of such loops - how does that fit together? >> And btw., for both 32- and 64-bit ARM, other than for x86, I see >> empty structure instances occupy zero bytes (and hence distinct >> symbols end up at the same address), so the compiler is conflicting >> with itself here. > > I imagine this is as much to do with the architecture ABI as the > compiler. So do I, but this doesn't make this any less of a compiler bug. Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-17 10:39 ` Jan Beulich @ 2013-05-17 14:34 ` Ian Campbell 2013-05-17 15:34 ` Jan Beulich 0 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-17 14:34 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall On Fri, 2013-05-17 at 11:39 +0100, Jan Beulich wrote: > >>> On 17.05.13 at 12:07, Ian Campbell <ian.campbell@citrix.com> wrote: > > On Thu, 2013-05-16 at 11:17 +0100, Jan Beulich wrote: > >> >>> On 15.05.13 at 15:47, Ian Campbell <ian.campbell@citrix.com> wrote: > >> > On Wed, 2013-05-15 at 13:19 +0100, Jan Beulich wrote: > >> >> >>> On 15.05.13 at 11:47, Ian Campbell <ian.campbell@citrix.com> wrote: > >> >> > "<" fails because it does process the (non-existent) first entry in the > >> >> > array. This happened to be "safe" in the case I saw but it wouldn't be > >> >> > in general. > >> >> > >> >> Okay, I misread one of your earlier responses then. Did you do > >> >> the necessary auditing already, or should I put this on my todo > >> >> list? > >> > > >> > I haven't done an audit. I put a very quicly grepped list in a previous > >> > mail but it is surely incomplete. > >> > >> So I went through all of them - the only other ones that can be > >> potentially empty are .ctors and .xsm_initcalls.init (I didn't check > >> whether ARM has any guaranteed .ex_table.pre uses though). > > > > On a random arm64 binary which I have here both ex_table and > > ex_table.pre are empty... > > Indeed. Yet arch/arm/ also has no reference to > __{start,stop}__{,_pre}_ex_table, so this is no problem. Out of > curiosity - there's nothing you ever do in ARM on behalf of the > user than can fault? That's right, we access guest memory with domain_map_page and never by using a guest virtual address directly (since we don't share page tables with the guest). The mechanism for translating guest virtual addresses into MFNs has an explicit error return and doesn't fault. > >> Both use "<", and the compiler translates this safely on x86. My > >> ARM assembly skills are still lacking, but afaict the early exit being > >> done with "popcs" / "b.cs" should be safe too, as they cover the > >> "==" case (APSR.C being set for x <= y). Thus I wonder what > >> code you saw being generated for the "<" case... > > > > 00000000 <test>: > > 0: e92d4038 push {r3, r4, r5, lr} > > 4: e59f4020 ldr r4, [pc, #32] ; 2c <test+0x2c> > > 8: e59f5020 ldr r5, [pc, #32] ; 30 <test+0x30> > > c: e1540005 cmp r4, r5 > > 10: 28bd8038 popcs {r3, r4, r5, pc} > > 14: e1a00004 mov r0, r4 > > 18: e2844004 add r4, r4, #4 > > 1c: ebfffffe bl 0 <u> > > 20: e1540005 cmp r4, r5 > > 24: 3afffffa bcc 14 <test+0x14> > > 28: e8bd8038 pop {r3, r4, r5, pc} > > > > So indeed I think you are correct that the popcs will do the right > > thing, I obviously missed the update of PC via that instruction when I > > looked at this before. > > But you still said that in practice you observed one unwanted > iteration of such loops - how does that fit together? You're right, I was sure I did observe that, but trying again now I don't. I think I may just have been confused earlier -- probably confusing my debug print after the loop with the one from inside. Sorry for the noise. > >> And btw., for both 32- and 64-bit ARM, other than for x86, I see > >> empty structure instances occupy zero bytes (and hence distinct > >> symbols end up at the same address), so the compiler is conflicting > >> with itself here. > > > > I imagine this is as much to do with the architecture ABI as the > > compiler. > > So do I, but this doesn't make this any less of a compiler bug. Is there a requirement (e.g. from the C standard) that an empty struct take at least one byte, or more likely I suppose something about different instances having different addresses which sort of implies it? Ian ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-17 14:34 ` Ian Campbell @ 2013-05-17 15:34 ` Jan Beulich 2013-05-17 17:03 ` Ian Campbell 0 siblings, 1 reply; 34+ messages in thread From: Jan Beulich @ 2013-05-17 15:34 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 17.05.13 at 16:34, Ian Campbell <ian.campbell@citrix.com> wrote: > On Fri, 2013-05-17 at 11:39 +0100, Jan Beulich wrote: >> >> And btw., for both 32- and 64-bit ARM, other than for x86, I see >> >> empty structure instances occupy zero bytes (and hence distinct >> >> symbols end up at the same address), so the compiler is conflicting >> >> with itself here. >> > >> > I imagine this is as much to do with the architecture ABI as the >> > compiler. >> >> So do I, but this doesn't make this any less of a compiler bug. > > Is there a requirement (e.g. from the C standard) that an empty struct > take at least one byte, or more likely I suppose something about > different instances having different addresses which sort of implies it? Yes: "Two pointers compare equal if and only if both are null pointers, both are pointers to the same object (including a pointer to an object and a subobject at its beginning) or function, both are pointers to one past the last element of the same array object, or one is a pointer to one past the end of one array object and the other is a pointer to the start of a different array object that happens to immediately follow the first array object in the address space." Gcc of course can, as an extension, support code that doesn't match this, but then they need to not do the visibility based optimization (and I would assume that, unless the architecture ABI specifically provides for it, this should be disabled by default). Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-17 15:34 ` Jan Beulich @ 2013-05-17 17:03 ` Ian Campbell 2013-05-21 7:54 ` Jan Beulich 0 siblings, 1 reply; 34+ messages in thread From: Ian Campbell @ 2013-05-17 17:03 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall On Fri, 2013-05-17 at 16:34 +0100, Jan Beulich wrote: > >>> On 17.05.13 at 16:34, Ian Campbell <ian.campbell@citrix.com> wrote: > > On Fri, 2013-05-17 at 11:39 +0100, Jan Beulich wrote: > >> >> And btw., for both 32- and 64-bit ARM, other than for x86, I see > >> >> empty structure instances occupy zero bytes (and hence distinct > >> >> symbols end up at the same address), so the compiler is conflicting > >> >> with itself here. > >> > > >> > I imagine this is as much to do with the architecture ABI as the > >> > compiler. > >> > >> So do I, but this doesn't make this any less of a compiler bug. > > > > Is there a requirement (e.g. from the C standard) that an empty struct > > take at least one byte, or more likely I suppose something about > > different instances having different addresses which sort of implies it? > > Yes: "Two pointers compare equal if and only if both are null pointers, > both are pointers to the same object (including a pointer to an object > and a subobject at its beginning) or function, both are pointers to one > past the last element of the same array object, or one is a pointer to > one past the end of one array object and the other is a pointer to the > start of a different array object that happens to immediately follow > the first array object in the address space." Right, that rings a bell, thanks. Even on x86 I see this: struct foo { }; static struct foo a, b; int main(void) { printf("a %p %lx\n", &a, (unsigned long)&a); printf("b %p %lx\n", &b, (unsigned long)&b); printf("a and b are %s\n", &a == &b ? "equal" : "distinct"); printf("ulong a and b are %s\n", (unsigned long)&a == (unsigned long)&b ? "equal" : "distinct"); } $ gcc --version gcc (Debian 4.7.2-5) 4.7.2 $ gcc -O2 t.c $ ./a.out a 0x60094c 60094c b 0x60094c 60094c a and b are distinct ulong a and b are distinct and I see the same thing on arm32 and visibility has no affect. This seems to conform to what you quote above even though the addresses appear equal. I was a bit surprised about the casted result... (I can't quite shake the feeling I've done something stupid here...) Ian. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-17 17:03 ` Ian Campbell @ 2013-05-21 7:54 ` Jan Beulich 0 siblings, 0 replies; 34+ messages in thread From: Jan Beulich @ 2013-05-21 7:54 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 17.05.13 at 19:03, Ian Campbell <ian.campbell@citrix.com> wrote: > On Fri, 2013-05-17 at 16:34 +0100, Jan Beulich wrote: >> >>> On 17.05.13 at 16:34, Ian Campbell <ian.campbell@citrix.com> wrote: >> > Is there a requirement (e.g. from the C standard) that an empty struct >> > take at least one byte, or more likely I suppose something about >> > different instances having different addresses which sort of implies it? >> >> Yes: "Two pointers compare equal if and only if both are null pointers, >> both are pointers to the same object (including a pointer to an object >> and a subobject at its beginning) or function, both are pointers to one >> past the last element of the same array object, or one is a pointer to >> one past the end of one array object and the other is a pointer to the >> start of a different array object that happens to immediately follow >> the first array object in the address space." > > Right, that rings a bell, thanks. > > Even on x86 I see this: > > struct foo { }; > static struct foo a, b; > int main(void) > { > printf("a %p %lx\n", &a, (unsigned long)&a); > printf("b %p %lx\n", &b, (unsigned long)&b); > printf("a and b are %s\n", &a == &b ? "equal" : "distinct"); > printf("ulong a and b are %s\n", (unsigned long)&a == (unsigned long)&b ? "equal" : "distinct"); > } > > $ gcc --version > gcc (Debian 4.7.2-5) 4.7.2 > $ gcc -O2 t.c > $ ./a.out > a 0x60094c 60094c > b 0x60094c 60094c > a and b are distinct > ulong a and b are distinct > > and I see the same thing on arm32 and visibility has no affect. > > This seems to conform to what you quote above even though the addresses > appear equal. This is overly simplified (as the compiler eliminates the comparisons). Try this #include <stdio.h> struct foo { }; extern struct foo a, b; int main(void) { printf("a %p %lx\n", &a, (unsigned long)&a); printf("b %p %lx\n", &b, (unsigned long)&b); printf("a and b are %s\n", &a == &b ? "equal" : "distinct"); printf("ulong a and b are %s\n", (unsigned long)&a == (unsigned long)&b ? "equal" : "distinct"); return 0; } once with this as second source file struct foo { }; struct foo a = { }, b = { }; and a second time with another second source file struct foo { }; struct foo a, b; The initializers already make a difference (also on x86). This is yet another bug then (apparently architecture independent). If you then take the earlier of the "second" source files and build it for ARM32, you'll see different behavior from x86 (identical to that with the latter "second" source file). And if you then change the declarations in the first source file to extern struct foo __attribute__((__visibility__("hidden"))) a; extern struct foo __attribute__((__visibility__("hidden"))) b; and compare the disassembly here with the original one, you'll see that the compiler eliminates the pointer comparison, but retains the comparison of the casts to unsigned long (the effect isn't visible in the output of the executable). Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 14:54 ` Ian Campbell 2013-05-14 14:56 ` Ian Campbell @ 2013-05-14 15:11 ` Jan Beulich 2013-05-14 15:14 ` Ian Campbell 1 sibling, 1 reply; 34+ messages in thread From: Jan Beulich @ 2013-05-14 15:11 UTC (permalink / raw) To: Ian Campbell Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall >>> On 14.05.13 at 16:54, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Tue, 2013-05-14 at 15:29 +0100, Jan Beulich wrote: >> What compiler version and switches were you using for the case >> where you spotted bad code to be generated? > > gcc-linaro-aarch64-linux-gnu-4.7-2013.01-20130125_linux doing a standard > arm64 build of Xen. Odd - I'm using mostly unmodified 4.7.3 from gcc.gnu.org, so I'm wondering if that Linaro build has meaningful extra patches and/or lacks important bug fixes. The reason I'm trying to get to the bottom of this is because, as you noted, we might be on the edge of running into more issues if at least some not know broken gcc versions behave like this. Could you try compiling the example code I sent earlier with the two arm compilers you use, to see if that trivial piece of code also exposes that behavior (or whether this is tied to something else)? Jan ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs. 2013-05-14 15:11 ` Jan Beulich @ 2013-05-14 15:14 ` Ian Campbell 0 siblings, 0 replies; 34+ messages in thread From: Ian Campbell @ 2013-05-14 15:14 UTC (permalink / raw) To: Jan Beulich Cc: Keir (Xen.org), Stefano Stabellini, Ian Jackson, Tim (Xen.org), xen-devel, Julien Grall On Tue, 2013-05-14 at 16:11 +0100, Jan Beulich wrote: > >>> On 14.05.13 at 16:54, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Tue, 2013-05-14 at 15:29 +0100, Jan Beulich wrote: > >> What compiler version and switches were you using for the case > >> where you spotted bad code to be generated? > > > > gcc-linaro-aarch64-linux-gnu-4.7-2013.01-20130125_linux doing a standard > > arm64 build of Xen. > > Odd - I'm using mostly unmodified 4.7.3 from gcc.gnu.org, so I'm > wondering if that Linaro build has meaningful extra patches and/or > lacks important bug fixes. > > The reason I'm trying to get to the bottom of this is because, as > you noted, we might be on the edge of running into more issues > if at least some not know broken gcc versions behave like this. > > Could you try compiling the example code I sent earlier with the > two arm compilers you use, to see if that trivial piece of code also > exposes that behavior (or whether this is tied to something else)? Looks like our mails crossed in the ether, I just posted the arm64 results... Didn't bother with arm32. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/4] xen/arm: fixes for 64 bit 2013-05-13 16:22 [PATCH 0/4] xen/arm: fixes for 64 bit Ian Campbell ` (3 preceding siblings ...) 2013-05-13 16:23 ` [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs Ian Campbell @ 2013-05-14 9:36 ` Ian Campbell 4 siblings, 0 replies; 34+ messages in thread From: Ian Campbell @ 2013-05-14 9:36 UTC (permalink / raw) To: xen-devel; +Cc: Julien Grall, Tim (Xen.org), Stefano Stabellini On Mon, 2013-05-13 at 17:22 +0100, Ian Campbell wrote: > The following fix a little bit of fallout from Julien's DT series on > 64-bit. I applied 1-3. As you'll have seen in my other mail it turns out #4 is a bit more complex than I first thought... Ian. ^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2013-05-21 7:54 UTC | newest] Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-05-13 16:22 [PATCH 0/4] xen/arm: fixes for 64 bit Ian Campbell 2013-05-13 16:22 ` [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files Ian Campbell 2013-05-13 16:44 ` Stefano Stabellini 2013-05-13 17:35 ` Tim Deegan 2013-05-13 16:23 ` [PATCH 2/4] xen/arm64: do not clobber callee saved register in early_putch Ian Campbell 2013-05-13 16:53 ` Stefano Stabellini 2013-05-13 16:23 ` [PATCH 3/4] xen/arm: support "arm, armv8-timer" DTS compatibility Ian Campbell 2013-05-13 16:46 ` Stefano Stabellini 2013-05-13 16:23 ` [PATCH 4/4] xen/arm: correctly handle an empty array of platform descs Ian Campbell 2013-05-13 16:48 ` Stefano Stabellini 2013-05-14 9:32 ` Ian Campbell 2013-05-14 9:48 ` Jan Beulich 2013-05-14 10:21 ` Ian Campbell 2013-05-14 14:29 ` Jan Beulich 2013-05-14 14:37 ` Jan Beulich 2013-05-14 14:54 ` Ian Campbell 2013-05-14 14:56 ` Ian Campbell 2013-05-14 15:07 ` Ian Campbell 2013-05-14 15:46 ` Jan Beulich 2013-05-14 16:41 ` Ian Campbell 2013-05-15 7:12 ` Jan Beulich 2013-05-15 9:47 ` Ian Campbell 2013-05-15 12:19 ` Jan Beulich 2013-05-15 13:47 ` Ian Campbell 2013-05-16 10:17 ` Jan Beulich 2013-05-17 10:07 ` Ian Campbell 2013-05-17 10:39 ` Jan Beulich 2013-05-17 14:34 ` Ian Campbell 2013-05-17 15:34 ` Jan Beulich 2013-05-17 17:03 ` Ian Campbell 2013-05-21 7:54 ` Jan Beulich 2013-05-14 15:11 ` Jan Beulich 2013-05-14 15:14 ` Ian Campbell 2013-05-14 9:36 ` [PATCH 0/4] xen/arm: fixes for 64 bit Ian Campbell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.