Re: disk/mdraid1x_linux.c:181:15: warning: array subscript ...

[Michael Chang <mchang@suse.com>]

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 <mchang@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 <mchang@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