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

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

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

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?
> 
Yes
>> 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
>>>> Grub-devel@gnu.org
>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>
>>
> 
> 
> 
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]