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

[Daniel Kiper]

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:
> > Hi,
> >
> > i wrote:
> > > >                (char *) &sb.dev_roles - (char *) sb
> > > >                + grub_le_to_cpu32 (sb.dev_number) * 
> > > > sizeof(grub_uint16_t)
> >
> > PGNet Dev wrote:
> > > grub-core/disk/mdraid1x_linux.c:183:6: error: cannot convert to a
> > > pointer type
> >
> > My fault. I forgot the "&" before "sb".
> >
> >   (char *) &sb.dev_roles - (char *) &sb
> >
> > I invested time in examining the C riddle, not in testing my proposal
> > by at least some dummy.
> >
> > Now this compiles for me without complaint by gcc -Wall
> >
> >  struct {
> >    char a_array[10];
> >    uint16_t dev_roles[0];
> >  } sb;
> >
> >  printf("%u\n", (unsigned int) (((char *) &sb.dev_roles - (char *) &sb)
> >                                 + 2 * sizeof(uint16_t)));
> >
> > Running this program yields 14 as result. The same as with the equivalent
> > of the old expression
> >
> >  printf("%u\n", (unsigned int) ((char *) &sb.dev_roles[2] - (char *) &sb));
> >
> >
> > > not sure I'm reading your intent from your post,
> >
> > My observation is that not "dev_roles[0]" is to blame for the warning, but
> > rather the computation which involves taking the address of an array
> > element while not a single one is allocated.
> > The resulting number is used as offset in a file, not in the sparsely
> > allocated "struct grub_raid_super_1x sb".
> >
> > My proposal is to avoid "[...]" in the course of the computation.
> > This should be valid for both ways to express an open ended struct:
> > "dev_roles[0]" and "dev_roles[]".
>
> I am also investigating the GCC 10 build problems, and my conclusion
> mostly coincided with yours. There we can replce the offset computation
> with pointer arithmetic like this.
>
> (char *) (sb.dev_roles + grub_le_to_cpu32 (sb.dev_number)) - (char *) &sb,
>
> Besides, there is also build error in zfs that I managed to come up with
> a patch as well. I attached both my patch here for your refernce.
>
> It would be great if you can help to test patch to solve the build
> problem for gcc-10 in your system or not.
>
> Thanks,
> Michael
>
> >
> >
> > Have a nice day :)
> >
> > Thomas
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel

> >From 19f55c05ea592b1339ee0d503c86be349447553f Mon Sep 17 00:00:00 2001
> From: Michael Chang <address@hidden>
> Date: Wed, 25 Mar 2020 13:52:51 +0800
> Subject: [PATCH 1/2] mdraid1x_linux: Fix gcc10 error -Werror=array-bounds
>
> We bumped into the build error while testing gcc-10 pre-release.
>
> ../../grub-core/disk/mdraid1x_linux.c: In function 'grub_mdraid_detect':
> ../../grub-core/disk/mdraid1x_linux.c:181:15: error: array subscript 
> <unknown> is outside array bounds of 'grub_uint16_t[0]' {aka 'short unsigned 
> int[0]'} [-Werror=array-bounds]
>   181 |      (char *) &sb.dev_roles[grub_le_to_cpu32 (sb.dev_number)]
>       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../grub-core/disk/mdraid1x_linux.c:98:17: note: while referencing 
> 'dev_roles'
>    98 |   grub_uint16_t dev_roles[0]; /* Role in array, or 0xffff for a 
> spare, or 0xfffe for faulty.  */
>       |                 ^~~~~~~~~
> ../../grub-core/disk/mdraid1x_linux.c:127:33: note: defined here 'sb'
>   127 |       struct grub_raid_super_1x sb;
>       |                                 ^~
> cc1: all warnings being treated as errors
>
> Apparently gcc issues the warning when trying to access sb.dev_roles
> array's member, since it is a zero length array as the last element of
> struct grub_raid_super_1x that is allocated sparsely without extra
> chunks for the trailing bits, so the warning looks legitimate in this
> regard.
>
> As the whole thing here is doing offset computation, it is undue to use
> syntax that would imply array member access then take address from it
> later. Instead we could accomplish the same thing through basic array
> pointer arithmetic to pacify the warning.
>
> Signed-off-by: Michael Chang <address@hidden>

Reviewed-by: Daniel Kiper <address@hidden>

> ---
>  grub-core/disk/mdraid1x_linux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/disk/mdraid1x_linux.c b/grub-core/disk/mdraid1x_linux.c
> index 7cc80d3df..c980feba4 100644
> --- a/grub-core/disk/mdraid1x_linux.c
> +++ b/grub-core/disk/mdraid1x_linux.c
> @@ -178,7 +178,7 @@ grub_mdraid_detect (grub_disk_t disk,
>       return NULL;
>
>        if (grub_disk_read (disk, sector,
> -                       (char *) &sb.dev_roles[grub_le_to_cpu32 
> (sb.dev_number)]
> +                       (char *) (sb.dev_roles + grub_le_to_cpu32 
> (sb.dev_number))
>                         - (char *) &sb,
>                         sizeof (role), &role))
>       return NULL;
> --
> 2.16.4
>

> >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?

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.

>  }
>
>  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?

Daniel