[PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[Alex Williamson]

On Wed, 2008-03-26 at 10:16 +0000, Xen staging patchbot-unstable wrote:
> # HG changeset patch
> # User Keir Fraser <keir.fraser@citrix.com>
> # Date 1206526490 0
> # Node ID 5d25187bac941611a8a836b668a398a72df0afb0
> # Parent  966c04d42e94546287a1145c82e13073f28ef116
> Explicitly tag every anonymous aggregate in the public headers.

ia64 build fix.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
--

diff -r b6d15be09aec xen/arch/ia64/vmx/save.c
--- a/xen/arch/ia64/vmx/save.c	Wed Mar 26 14:05:36 2008 +0000
+++ b/xen/arch/ia64/vmx/save.c	Wed Mar 26 09:08:42 2008 -0600
@@ -21,6 +21,7 @@
  */
 
 #include <xen/types.h>
+#include <public/xen.h>
 #include <xen/hvm/save.h>
 
 void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
diff -r b6d15be09aec xen/arch/ia64/xen/dom_fw_common.c
--- a/xen/arch/ia64/xen/dom_fw_common.c	Wed Mar 26 14:05:36 2008 +0000
+++ b/xen/arch/ia64/xen/dom_fw_common.c	Wed Mar 26 09:08:42 2008 -0600
@@ -20,6 +20,7 @@
 #include <assert.h>
 #include <inttypes.h>
 
+#include <xen/xen.h>
 #include <xen/arch-ia64.h>
 #include <asm/bundle.h>
 
diff -r b6d15be09aec xen/arch/ia64/xen/dom_fw_domu.c
--- a/xen/arch/ia64/xen/dom_fw_domu.c	Wed Mar 26 14:05:36 2008 +0000
+++ b/xen/arch/ia64/xen/dom_fw_domu.c	Wed Mar 26 09:08:42 2008 -0600
@@ -37,6 +37,7 @@
 #include <errno.h>
 #include <inttypes.h>
 
+#include <xen/xen.h>
 #include <xen/arch-ia64.h>
 
 #include "xg_private.h"

Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[Alex Williamson]

[-- Attachment #1: Type: text/plain, Size: 1659 bytes --]

resend

On Wed, 2008-03-26 at 09:11 -0600, Alex Williamson wrote:
> On Wed, 2008-03-26 at 10:16 +0000, Xen staging patchbot-unstable wrote:
> > # HG changeset patch
> > # User Keir Fraser <keir.fraser@citrix.com>
> > # Date 1206526490 0
> > # Node ID 5d25187bac941611a8a836b668a398a72df0afb0
> > # Parent  966c04d42e94546287a1145c82e13073f28ef116
> > Explicitly tag every anonymous aggregate in the public headers.
> 
> ia64 build fix.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> --
> 
> diff -r b6d15be09aec xen/arch/ia64/vmx/save.c
> --- a/xen/arch/ia64/vmx/save.c	Wed Mar 26 14:05:36 2008 +0000
> +++ b/xen/arch/ia64/vmx/save.c	Wed Mar 26 09:08:42 2008 -0600
> @@ -21,6 +21,7 @@
>   */
>  
>  #include <xen/types.h>
> +#include <public/xen.h>
>  #include <xen/hvm/save.h>
>  
>  void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
> diff -r b6d15be09aec xen/arch/ia64/xen/dom_fw_common.c
> --- a/xen/arch/ia64/xen/dom_fw_common.c	Wed Mar 26 14:05:36 2008 +0000
> +++ b/xen/arch/ia64/xen/dom_fw_common.c	Wed Mar 26 09:08:42 2008 -0600
> @@ -20,6 +20,7 @@
>  #include <assert.h>
>  #include <inttypes.h>
>  
> +#include <xen/xen.h>
>  #include <xen/arch-ia64.h>
>  #include <asm/bundle.h>
>  
> diff -r b6d15be09aec xen/arch/ia64/xen/dom_fw_domu.c
> --- a/xen/arch/ia64/xen/dom_fw_domu.c	Wed Mar 26 14:05:36 2008 +0000
> +++ b/xen/arch/ia64/xen/dom_fw_domu.c	Wed Mar 26 09:08:42 2008 -0600
> @@ -37,6 +37,7 @@
>  #include <errno.h>
>  #include <inttypes.h>
>  
> +#include <xen/xen.h>
>  #include <xen/arch-ia64.h>
>  
>  #include "xg_private.h"
> 
-- 
Alex Williamson                             HP Open Source & Linux Org.

[-- Attachment #2: ia64_build_fix.patch --]
[-- Type: text/x-patch, Size: 1009 bytes --]

diff -r b6d15be09aec xen/arch/ia64/vmx/save.c
--- a/xen/arch/ia64/vmx/save.c	Wed Mar 26 14:05:36 2008 +0000
+++ b/xen/arch/ia64/vmx/save.c	Wed Mar 26 09:08:42 2008 -0600
@@ -21,6 +21,7 @@
  */
 
 #include <xen/types.h>
+#include <public/xen.h>
 #include <xen/hvm/save.h>
 
 void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
diff -r b6d15be09aec xen/arch/ia64/xen/dom_fw_common.c
--- a/xen/arch/ia64/xen/dom_fw_common.c	Wed Mar 26 14:05:36 2008 +0000
+++ b/xen/arch/ia64/xen/dom_fw_common.c	Wed Mar 26 09:08:42 2008 -0600
@@ -20,6 +20,7 @@
 #include <assert.h>
 #include <inttypes.h>
 
+#include <xen/xen.h>
 #include <xen/arch-ia64.h>
 #include <asm/bundle.h>
 
diff -r b6d15be09aec xen/arch/ia64/xen/dom_fw_domu.c
--- a/xen/arch/ia64/xen/dom_fw_domu.c	Wed Mar 26 14:05:36 2008 +0000
+++ b/xen/arch/ia64/xen/dom_fw_domu.c	Wed Mar 26 09:08:42 2008 -0600
@@ -37,6 +37,7 @@
 #include <errno.h>
 #include <inttypes.h>
 
+#include <xen/xen.h>
 #include <xen/arch-ia64.h>
 
 #include "xg_private.h"

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[Alex Williamson]

On Wed, 2008-03-26 at 10:16 +0000, Xen staging patchbot-unstable wrote:
> # HG changeset patch
> # User Keir Fraser <keir.fraser@citrix.com>
> # Date 1206526490 0
> # Node ID 5d25187bac941611a8a836b668a398a72df0afb0
> # Parent  966c04d42e94546287a1145c82e13073f28ef116
> Explicitly tag every anonymous aggregate in the public headers.

   My previous fix allowed this to build on ia64, but it turns out
there's still a boot issue that I don't understand.  As is, we take a
nested dtlb fault on boot, which hg bisect determines is caused by this
patch.  From a simple test program, I can verify that only the outermost
__extension__ is necessary to include code w/ -std=c99.  So embedding an
__extension__ within an __extension__ isn't necessary, but I don't know
why it actually changes the behavior of the code.  The patch below
reverts a few chucks of this cset and gets us booting again.  FWIW, I'm
using gcc-4.1.2.  Thanks,

	Alex

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

diff -r 6736c28a0d35 xen/include/public/arch-ia64.h
--- a/xen/include/public/arch-ia64.h	Fri Mar 28 17:58:36 2008 +0000
+++ b/xen/include/public/arch-ia64.h	Fri Mar 28 14:51:41 2008 -0600
@@ -182,7 +182,7 @@ struct mapped_regs {
     unsigned long  reserved4[76];
     __anonymous_union {
         unsigned long  vcr[128];
-        __anonymous_struct {
+        struct {
             unsigned long dcr;  // CR0
             unsigned long itm;
             unsigned long iva;
@@ -216,7 +216,7 @@ struct mapped_regs {
     };
     __anonymous_union {
         unsigned long  reserved5[128];
-        __anonymous_struct {
+        struct {
             unsigned long precover_ifs;
             unsigned long unat;  // not sure if this is needed until NaT arch is done
             int interrupt_collection_enabled; // virtual psr.ic
@@ -609,7 +609,7 @@ struct xen_ia64_opt_feature {
 	unsigned long cmd;		/* Which feature */
 	unsigned char on;		/* Switch feature on/off */
 	__anonymous_union {
-		__anonymous_struct {
+		struct {
 				/* The page protection bit mask of the pte.
 			 	 * This will be or'ed with the pte. */
 			unsigned long pgprot;

Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[Keir Fraser]

On 28/3/08 21:04, "Alex Williamson" <alex.williamson@hp.com> wrote:

>    My previous fix allowed this to build on ia64, but it turns out
> there's still a boot issue that I don't understand.  As is, we take a
> nested dtlb fault on boot, which hg bisect determines is caused by this
> patch.  From a simple test program, I can verify that only the outermost
> __extension__ is necessary to include code w/ -std=c99.  So embedding an
> __extension__ within an __extension__ isn't necessary, but I don't know
> why it actually changes the behavior of the code.  The patch below
> reverts a few chucks of this cset and gets us booting again.  FWIW, I'm
> using gcc-4.1.2.  Thanks,

Playing around with this a bit I can't see what changes for having nested
usage of __extension__. I'm using gcc 4.1 but on x86_64.

Could you play around with sizeof() and offsetof() and find out exactly
what's getting laid out differently? Maybe we'll have to do some of the
reversion that you suggest (or just revert the whole lot and assert __GNUC__
and !__STRICT_ANSI__ at the top of the file). But it'd be nice to know a bit
more about what's going on here.

 -- Keir

Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[Alex Williamson]

On Fri, 2008-03-28 at 21:47 +0000, Keir Fraser wrote:
> On 28/3/08 21:04, "Alex Williamson" <alex.williamson@hp.com> wrote:
> 
> >    My previous fix allowed this to build on ia64, but it turns out
> > there's still a boot issue that I don't understand.  As is, we take a
> > nested dtlb fault on boot, which hg bisect determines is caused by this
> > patch.  From a simple test program, I can verify that only the outermost
> > __extension__ is necessary to include code w/ -std=c99.  So embedding an
> > __extension__ within an __extension__ isn't necessary, but I don't know
> > why it actually changes the behavior of the code.  The patch below
> > reverts a few chucks of this cset and gets us booting again.  FWIW, I'm
> > using gcc-4.1.2.  Thanks,
> 
> Playing around with this a bit I can't see what changes for having nested
> usage of __extension__. I'm using gcc 4.1 but on x86_64.
> 
> Could you play around with sizeof() and offsetof() and find out exactly
> what's getting laid out differently? Maybe we'll have to do some of the
> reversion that you suggest (or just revert the whole lot and assert __GNUC__
> and !__STRICT_ANSI__ at the top of the file). But it'd be nice to know a bit
> more about what's going on here.

   Ok, here's what I found out:

# struct xen_ia64_opt_feature with __anonymous_struct:

sizeof(struct xen_ia64_opt_feature): 24
offsetof(struct xen_ia64_opt_feature, pgprot): 16

# without:

sizeof(struct xen_ia64_opt_feature): 32
offsetof(struct xen_ia64_opt_feature, pgprot): 16

The sizeof the struct changes, and it can't possible be 24 bytes if an 8
byte pgprot starts at offset 16 and is followed by another 8 bytes, so
the size is wrong with the __extension__ attribute.

--

# struct mapped_regs with __anonymous_struct:

sizeof(struct mapped_regs): 4096
offsetof(struct mapped_regs, dcr): 2048
offsetof(struct mapped_regs, rsv6): 2048
offsetof(struct mapped_regs, precover_ifs): 3072
offsetof(struct mapped_regs, tmp): 3072

# without:

sizeof(struct mapped_regs): 4096
offsetof(struct mapped_regs, dcr): 2048
offsetof(struct mapped_regs, rsv6): 2704
offsetof(struct mapped_regs, precover_ifs): 3072
offsetof(struct mapped_regs, tmp): 3280

This time the sizeof is correct, and the offset of the first entries in
the struct are correct, but the last entry in the struct is at the same
offset as the first, which is obviously unintended.

It seems pretty clear that in nesting __extension__ attributes in this
manner, the struct is actually ignored and the compiler is treating the
everything that was in the struct as a separate member of the union.  In
the mapped_regs example, the structure size remains correct only because
the union is full padded out using the full size arrays.  Please apply
the patch I sent previously to revert the nested __extension__
attributes.  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[Keir Fraser]

On 31/3/08 16:49, "Alex Williamson" <alex.williamson@hp.com> wrote:

> It seems pretty clear that in nesting __extension__ attributes in this
> manner, the struct is actually ignored and the compiler is treating the
> everything that was in the struct as a separate member of the union.  In
> the mapped_regs example, the structure size remains correct only because
> the union is full padded out using the full size arrays.  Please apply
> the patch I sent previously to revert the nested __extension__
> attributes.  Thanks,

__extension__ seems worryingly half-baked. We're better off asserting
!__STRICT_ANSI__ in my opinion. I don't suppose you much care either way as
long as ia64 boot works again. :-)

 -- Keir

Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[Alex Williamson]

On Mon, 2008-03-31 at 17:21 +0100, Keir Fraser wrote:
> On 31/3/08 16:49, "Alex Williamson" <alex.williamson@hp.com> wrote:
> 
> > It seems pretty clear that in nesting __extension__ attributes in this
> > manner, the struct is actually ignored and the compiler is treating the
> > everything that was in the struct as a separate member of the union.  In
> > the mapped_regs example, the structure size remains correct only because
> > the union is full padded out using the full size arrays.  Please apply
> > the patch I sent previously to revert the nested __extension__
> > attributes.  Thanks,
> 
> __extension__ seems worryingly half-baked. We're better off asserting
> !__STRICT_ANSI__ in my opinion. I don't suppose you much care either way as
> long as ia64 boot works again. :-)

   Yup ;^)  FWIW, I still haven't been able to create a simple test
program that shows this behavior.  For these tests I've had to resort to
putting printks in xen.  There must be some build option we use that
tickles this issue.  Still, not good for an option that claims to have
no side effects.  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[Keir Fraser]

On 28/3/08 21:04, "Alex Williamson" <alex.williamson@hp.com> wrote:

> @@ -609,7 +609,7 @@ struct xen_ia64_opt_feature {
> unsigned long cmd;  /* Which feature */
> unsigned char on;  /* Switch feature on/off */
> __anonymous_union {
> -  __anonymous_struct {
> +  struct {
> /* The page protection bit mask of the pte.
> * This will be or'ed with the pte. */
> unsigned long pgprot;

I have no idea if it's the cause of this particular boot breakage, but I
reckon that an anonymous union containing a single anonymous struct can be
simplified. Or perhaps, if the union is there for stylistic reasons, a
single extra field in the union would fix things?

 -- Keir

Re: Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.

[Alex Williamson]

On Fri, 2008-03-28 at 22:37 +0000, Keir Fraser wrote:
> On 28/3/08 21:04, "Alex Williamson" <alex.williamson@hp.com> wrote:
> 
> > @@ -609,7 +609,7 @@ struct xen_ia64_opt_feature {
> > unsigned long cmd;  /* Which feature */
> > unsigned char on;  /* Switch feature on/off */
> > __anonymous_union {
> > -  __anonymous_struct {
> > +  struct {
> > /* The page protection bit mask of the pte.
> > * This will be or'ed with the pte. */
> > unsigned long pgprot;
> 
> I have no idea if it's the cause of this particular boot breakage, but I
> reckon that an anonymous union containing a single anonymous struct can be
> simplified. Or perhaps, if the union is there for stylistic reasons, a
> single extra field in the union would fix things?

   I toggled this one separately exactly for that reason, but it's not
the one.  I still haven't isolated a particular reason why it fails in a
test program, but I'll keep trying.  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.