[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] zfs: fix compilation failure with clang due to alignment
From: |
Leif Lindholm |
Subject: |
Re: [PATCH] zfs: fix compilation failure with clang due to alignment |
Date: |
Wed, 15 Jul 2015 17:52:28 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Jul 15, 2015 at 05:47:50PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko
wrote:
> Go ahead
The below was more of an RFC than something committable - are you OK
with me splitting the types.h changes out as a separate patch?
> On 07.07.2015 19:17, Leif Lindholm wrote:
> > On Fri, Jul 03, 2015 at 10:05:47PM +0300, Andrei Borzenkov wrote:
> >> I do not claim I understand why clang complains, but this patch does
> >> fix it.
> >>
> >> fs/xfs.c:452:25: error: cast from 'struct grub_xfs_btree_node *' to
> >> 'grub_uint64_t *' (aka 'unsigned long long *') increases required
> >> alignment from 1 to 8 [-Werror,-Wcast-align]
> >> grub_uint64_t *keys = (grub_uint64_t *)(leaf + 1);
> >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> 1 error generated.
> >>
> >> ---
> >>
> >> Jan, do you have any idea what's wrong and whether this is proper fix?
> >> Or should I raise it with clang?
> >
> > Well, the problem is that struct grub_xfs_btree_node is defined with
> > GRUB_PACKED - forcing a 1-byte alignment requirement as opposed to the
> > 8-byte requirement that would naturally be enforced by the struct
> > contents. And apparently clang objects to this, whereas gcc thinks
> > everything is fine ... even though -Wcast-align is explicitly used.
> >
> > Now, grub_xfs_btree_keys() is only called by grub_xfs_read_block(),
> > where it is immediately stuffed back into another 8-byte aligned
> > pointer. And then the alignment is immediately discarded again by
> > casting it to a (char *) for an arithmetic operation.
> >
> > If the alignment is indeed not required, it may be worth explicitly
> > marking that pointer as one to a potentially unaligned location.
> > But we don't currently appear to have a GRUB_UNALIGNED macro, to match
> > the GRUB_PACKED for structs. Should we?
> >
> > If so, something like the following could be added to your patch for a
> > more complete fix:
> > --- a/grub-core/fs/xfs.c
> > +++ b/grub-core/fs/xfs.c
> > @@ -488,7 +488,7 @@ grub_xfs_read_block (grub_fshelp_node_t node,
> > grub_disk_addr_t fileblock)
> > if (node->inode.format == XFS_INODE_FORMAT_BTREE)
> > {
> > struct grub_xfs_btree_root *root;
> > - const grub_uint64_t *keys;
> > + const grub_uint64_t *keys GRUB_UNALIGNED;
> > int recoffset;
> >
> > leaf = grub_malloc (node->data->bsize);
> > diff --git a/include/grub/types.h b/include/grub/types.h
> > index e732efb..720e236 100644
> > --- a/include/grub/types.h
> > +++ b/include/grub/types.h
> > @@ -30,6 +30,8 @@
> > #define GRUB_PACKED __attribute__ ((packed))
> > #endif
> >
> > +#define GRUB_UNALIGNED __attribute__ ((aligned (1)))
> > +
> > #ifdef GRUB_BUILD
> > # define GRUB_CPU_SIZEOF_VOID_P BUILD_SIZEOF_VOID_P
> > # define GRUB_CPU_SIZEOF_LONG BUILD_SIZEOF_LONG
> >
> > /
> > Leif
> >
> >> grub-core/fs/xfs.c | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> >> index 7249291..ea8cf7e 100644
> >> --- a/grub-core/fs/xfs.c
> >> +++ b/grub-core/fs/xfs.c
> >> @@ -445,14 +445,14 @@ grub_xfs_next_de(struct grub_xfs_data *data, struct
> >> grub_xfs_dir2_entry *de)
> >> return (struct grub_xfs_dir2_entry *)(((char *)de) + ALIGN_UP(size, 8));
> >> }
> >>
> >> -static grub_uint64_t *
> >> +static void *
> >> grub_xfs_btree_keys(struct grub_xfs_data *data,
> >> struct grub_xfs_btree_node *leaf)
> >> {
> >> - grub_uint64_t *keys = (grub_uint64_t *)(leaf + 1);
> >> + char *keys = (char *)leaf + sizeof (*leaf);
> >>
> >> if (data->hascrc)
> >> - keys += 6; /* skip crc, uuid, ... */
> >> + keys += 6 * sizeof (grub_uint64_t); /* skip crc, uuid, ... */
> >> return keys;
> >> }
> >>
> >> --
> >> tg: (7a21030..) u/xfs-clang-align (depends on: master)
> >>
> >> _______________________________________________
> >> Grub-devel mailing list
> >> address@hidden
> >> https://lists.gnu.org/mailman/listinfo/grub-devel
> >
> > _______________________________________________
> > Grub-devel mailing list
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] zfs: fix compilation failure with clang due to alignment, Vladimir 'φ-coder/phcoder' Serbinenko, 2015/07/15