[PATCH 0/4] xen/arm: fixes for 64 bit

[PATCH 0/4] xen/arm: fixes for 64 bit

[Ian Campbell]

The following fix a little bit of fallout from Julien's DT series on
64-bit.

Ian.

[PATCH 1/4] xen/arm: Add emacs magic blocks to asm files

[Ian Campbell]

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

Re: [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files

[Stefano Stabellini]

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
> 

Re: [PATCH 1/4] xen/arm: Add emacs magic blocks to asm files

[Tim Deegan]

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>)

[PATCH 2/4] xen/arm64: do not clobber callee saved register in early_putch

[Ian Campbell]

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

Re: [PATCH 2/4] xen/arm64: do not clobber callee saved register in early_putch

[Stefano Stabellini]

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
> 

[PATCH 3/4] xen/arm: support "arm, armv8-timer" DTS compatibility.

[Ian Campbell]

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

Re: [PATCH 3/4] xen/arm: support "arm, armv8-timer" DTS compatibility.

[Stefano Stabellini]

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
> 

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

[Ian Campbell]

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

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

[Stefano Stabellini]

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
> 

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

[Ian Campbell]

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.

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

[Jan Beulich]

>>> 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

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

[Ian Campbell]

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.

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

[Jan Beulich]

>>> 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

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

[Jan Beulich]

>>> 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

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

[Ian Campbell]

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:

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

[Ian Campbell]

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

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

[Ian Campbell]

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.

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

[Jan Beulich]

>>> 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

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

[Ian Campbell]

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.

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

[Jan Beulich]

>>> 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

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

[Ian Campbell]

> 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.

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

[Jan Beulich]

>>> 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

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

[Ian Campbell]

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.

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

[Jan Beulich]

>>> 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);
}

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

[Ian Campbell]

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

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

[Jan Beulich]

>>> 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

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

[Ian Campbell]

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

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

[Jan Beulich]

>>> 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

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

[Ian Campbell]

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.

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

[Jan Beulich]

>>> 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

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

[Jan Beulich]

>>> 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

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

[Ian Campbell]

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.

Re: [PATCH 0/4] xen/arm: fixes for 64 bit

[Ian Campbell]

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.