On Wed, Mar 25, 2020 at 07:35:27PM +0100, Daniel Kiper wrote:
> On Wed, Mar 25, 2020 at 03:27:28PM +0800, Michael Chang wrote:
> > On Tue, Mar 24, 2020 at 06:54:43PM +0100, Thomas Schmitt wrote:
[snip]
> > >From 8666468ac1a35f0678672de5c89a3f062d1aeb39 Mon Sep 17 00:00:00 2001
> > From: Michael Chang <mch...@suse.com>
> > Date: Wed, 25 Mar 2020 14:28:15 +0800
> > Subject: [PATCH 2/2] zfs: Fix gcc10 error -Werror=zero-length-bounds
> >
> > We bumped into the build error while testing gcc-10 pre-release.
> >
> > In file included from ../../include/grub/file.h:22,
> > from ../../grub-core/fs/zfs/zfs.c:34:
> > ../../grub-core/fs/zfs/zfs.c: In function 'zap_leaf_lookup':
> > ../../grub-core/fs/zfs/zfs.c:2263:44: error: array subscript '<unknown>' is
> > outside the bounds of an interior zero-length array 'grub_uint16_t[0]' {aka
> > 'short unsigned int[0]'} [-Werror=zero-length-bounds]
> > 2263 | for (chunk = grub_zfs_to_cpu16 (l->l_hash[LEAF_HASH (blksft, h,
> > l)], endian);
> > ../../include/grub/types.h:241:48: note: in definition of macro
> > 'grub_le_to_cpu16'
> > 241 | # define grub_le_to_cpu16(x) ((grub_uint16_t) (x))
> > | ^
> > ../../grub-core/fs/zfs/zfs.c:2263:16: note: in expansion of macro
> > 'grub_zfs_to_cpu16'
> > 2263 | for (chunk = grub_zfs_to_cpu16 (l->l_hash[LEAF_HASH (blksft, h,
> > l)], endian);
> > | ^~~~~~~~~~~~~~~~~
> > In file included from ../../grub-core/fs/zfs/zfs.c:48:
> > ../../include/grub/zfs/zap_leaf.h:72:16: note: while referencing 'l_hash'
> > 72 | grub_uint16_t l_hash[0];
> > | ^~~~~~
> >
> > Here I'd like to quote from the gcc document [1] which seems to be best
> > to explain what is going on here.
> >
> > "Declaring zero-length arrays in other contexts, including as interior
> > members of structure objects or as non-member objects, is discouraged.
> > Accessing elements of zero-length arrays declared in such contexts is
> > undefined and may be diagnosed."
> >
> > The l_hash[0] is apparnetly an interior member to the enclosed structure
> > while l_entries[0] is the trailing member. And the offending code tries
> > to access members in l_hash[0] array that triggers the diagnose.
> >
> > Given that the l_entries[0] is used to get proper alignment to access
> > leaf chunks, we can accomplish the same thing through the ALIGN_UP macro
> > thus eliminating l_entries[0] from the structure. In this way we can
> > pacify the warning as l_hash[0] now becomes the last member to the
> > enclosed structure.
> >
> > [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> >
> > Signed-off-by: Michael Chang <mch...@suse.com>
> > ---
> > grub-core/fs/zfs/zfs.c | 7 ++++---
> > include/grub/zfs/zap_leaf.h | 1 -
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> > index 2f72e42bf..f38f5b102 100644
> > --- a/grub-core/fs/zfs/zfs.c
> > +++ b/grub-core/fs/zfs/zfs.c
> > @@ -141,9 +141,10 @@ ZAP_LEAF_NUMCHUNKS (int bs)
> > static inline zap_leaf_chunk_t *
> > ZAP_LEAF_CHUNK (zap_leaf_phys_t *l, int bs, int idx)
> > {
> > - return &((zap_leaf_chunk_t *) (l->l_entries
> > - + (ZAP_LEAF_HASH_NUMENTRIES(bs) * 2)
> > - / sizeof (grub_properly_aligned_t)))[idx];
> > + grub_properly_aligned_t *l_entries;
> > +
> > + l_entries = (grub_properly_aligned_t *) ALIGN_UP((grub_addr_t)l->l_hash,
> > sizeof (grub_properly_aligned_t));
> > + return &((zap_leaf_chunk_t *) (l_entries +
> > ZAP_LEAF_HASH_NUMENTRIES(bs)))[idx];
>
> Why are you skipping "* 2) / sizeof (grub_properly_aligned_t)" here?
It is based on this comment before the function.
"The chunks start immediately after the hash table. The end of the hash
table is at l_hash + HASH_NUMENTRIES, which we simply cast to a
chunk_t."
I suppose the magic number "2" could be "sizeof (l->l_hash[0])", so the
computatio1n is likely to get number of entries for l->l_entries[] from
which we can take address as the chunk start. And since it is indexed by
type grub_properly_aligned_t, the alignment is automagically satisfied.
>
> And could you add following excerpt from [1] to the commit message:
> Although the size of a zero-length array is zero, an array member of
> this kind may increase the size of the enclosing type as a result of
> tail padding. The offset of a zero-length array member from the
> beginning of the enclosing structure is the same as the offset of an
> array with one or more elements of the same type. The alignment of a
> zero-length array is the same as the alignment of its elements.
OK. I will do so.
>
> > }
> >
> > static inline struct zap_leaf_entry *
> > diff --git a/include/grub/zfs/zap_leaf.h b/include/grub/zfs/zap_leaf.h
> > index 95c67dcba..11447c166 100644
> > --- a/include/grub/zfs/zap_leaf.h
> > +++ b/include/grub/zfs/zap_leaf.h
> > @@ -70,7 +70,6 @@ typedef struct zap_leaf_phys {
> > */
> >
> > grub_uint16_t l_hash[0];
> > - grub_properly_aligned_t l_entries[0];
> > } zap_leaf_phys_t;
> >
> > typedef union zap_leaf_chunk {
> > --
> > 2.16.4
> >
>
> May I ask you to repost the patches in the proper format and CC all
> people involved in the discussion?
OK. I will.
Thanks,
Michael
>
> Daniel
>
> _______________________________________________
> 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
