Re: [PATCH] zfs: fix compilation failure with clang due to alignment

[Leif Lindholm]

On Thu, Jul 16, 2015 at 12:07:14PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko 
wrote:
> On 15.07.2015 18:52, Leif Lindholm wrote:
> > 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?
> > 
> Actually I think this patch doesn't work as __attribute__((aligned(X)))
> can only increase alignment, not decrease it. I'm going to fix it by
> using char * and grub_get_unaligned64

Yeah, you're right - it should actually have been (packed) again.

Anyway, you already contributed a fix.

/
    Leif

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