Alex Williamson
2008-Mar-28 21:04 UTC
[Xen-devel] [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.
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; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Mar-28 21:47 UTC
[Xen-devel] Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Mar-28 22:37 UTC
[Xen-devel] Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Alex Williamson
2008-Mar-28 22:55 UTC
Re: [Xen-devel] Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Alex Williamson
2008-Mar-31 15:49 UTC
[Xen-devel] Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Mar-31 16:21 UTC
[Xen-devel] Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Alex Williamson
2008-Mar-31 16:28 UTC
[Xen-devel] Re: [PATCH] Re: [Xen-staging] [xen-unstable] Explicitly tag every anonymous aggregate in the public headers.
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel