| Message ID | 20150707171713.GB8790@bivouac.eciton.net |
|---|---|
| State | New |
| Headers | show |
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 > >> 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
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 > >>>> 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 > > > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel
--- 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