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

[Michael Chang]

On Thu, Mar 26, 2020 at 12:13:11PM +0800, Michael Chang wrote:
> 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 <address@hidden>
> > > 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 <address@hidden>
> > > ---
> > >  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?

Ah, indeed I had mistake here. It has to take "HASH_NUMENTRIES * 2) /
sizeof (grub_properly_aligned_t)" for computing the entry number of
l_entries from given HASH_NUMENTRIES.

I will fix that and repost patches.

Thanks,
Michael

> 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
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel