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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

signature.asc
Description: OpenPGP digital signature