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
