grub/head build with pre-release GCC10 ; fail @ "grub-core/disk/mdraid1x_linux.c:181:15: error: ..."

grub/head build with pre-release GCC10 ; fail @ "grub-core/disk/mdraid1x_linux.c:181:15: error: ..."

[PGNet Dev]

 building curren head,

	git clone https://git.savannah.gnu.org/git/grub.git
	cd grub
	git describe
		grub-2.04-71-g552c9fd08

with soon(ish)-to-be-release GCC10

	gcc --version
		gcc (SUSE Linux) 10.0.1 20200320 (experimental) [revision 7d4549b2cd209eb621453ce13be7ffd84ffa720a]

config's OK

	unset CC CPP
	./bootstrap
	./autogen.sh
	./configure

build fails,

make V=1
	...
	gcc -DHAVE_CONFIG_H -I.  -Wall -W -DGRUB_UTIL=1 -D_FILE_OFFSET_BITS=64 -I./include -DGRUB_FILE=\"grub-core/disk/mdraid1x_linux.c\" -I. -I. -I. -I. -I./include -I./include -I./grub-core/lib/libgcrypt-grub/src/  -I./grub-core/lib/minilzo -I./grub-core/lib/xzembed -I./grub-core/lib/zstd -DMINILZO_HAVE_CONFIG_H -O3 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fmessage-length=0 -grecord-gcc-switches -march=native -mtune=native -D_FORTIFY_SOURCE=2 -D_FILE_OFFSET_BITS=64 -Wall -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces -Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type -Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs -Wunknown-pragmas -Wunused -Wunused-function -Wunused-label -Wunused-parameter -Wunused-value  -Wunused-variable -Wwrite-strings -Wnested-externs -Wstrict-prototypes -Wcast-align  -Wextra -Wattributes -Wendif-labels -Winit-self -Wint-to-pointer-cast -Winvalid-pch -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla -Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros -Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs -Wmissing-prototypes -Wmissing-declarations -Wformat=2 -Werror  -fno-builtin -Wno-undef -O3 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fmessage-length=0 -grecord-gcc-switches -march=native -mtune=native -MT grub-core/disk/libgrubmods_a-mdraid1x_linux.o -MD -MP -MF grub-core/disk/.deps-util/libgrubmods_a-mdraid1x_linux.Tpo -c -o grub-core/disk/libgrubmods_a-mdraid1x_linux.o `test -f 'grub-core/disk/mdraid1x_linux.c' || echo './'`grub-core/disk/mdraid1x_linux.c
	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
	make[2]: *** [Makefile:7059: grub-core/disk/libgrubmods_a-mdraid1x_linux.o] Error 1
	make[2]: Leaving directory '/usr/local/src/grub'
	make[1]: *** [Makefile:11920: all-recursive] Error 1
	make[1]: Leaving directory '/usr/local/src/grub'
	make: *** [Makefile:3772: all] Error 2


known/expected issue? bug-worthy?  or still too early for GCC10 issues?

Re: grub/head build with pre-release GCC10 ; fail @ "grub-core/disk/mdraid1x_linux.c:181:15: error: ..."

[Paul Menzel]

Dear PGNet Dev,


Already reported and analyzed [1]. It’d be nice, if you could verify 
Thomas’ analysis.


Kind regards,

Paul


[1]: https://lists.gnu.org/archive/html/grub-devel/2020-03/msg00206.html

Re: grub/head build with pre-release GCC10 ; fail @ "grub-core/disk/mdraid1x_linux.c:181:15: error: ..."

[PGNet Dev]

Thomas,


On 3/24/20 7:51 AM, Paul Menzel wrote:
> Dear PGNet Dev,
> 
> 
> Already reported and analyzed [1]. It’d be nice, if you could verify Thomas’ analysis.

not sure I'm reading your intent from your post,

>	...
>	I think that the following expression produces the same number without
>	virtual access to a virtual array member:
>
>	         (char *) &sb.dev_roles - (char *) sb
>	         + grub_le_to_cpu32 (sb.dev_number) * sizeof(grub_uint16_t)
>	...

with this,

	diff -ur grub.ORIG/grub-core/disk/mdraid1x_linux.c grub/grub-core/disk/mdraid1x_linux.c
	--- grub.ORIG/grub-core/disk/mdraid1x_linux.c	2020-03-24 09:24:08.656640265 -0700
	+++ grub/grub-core/disk/mdraid1x_linux.c	2020-03-24 09:25:20.615871693 -0700
	@@ -178,8 +178,9 @@
		return NULL;
	 
	       if (grub_disk_read (disk, sector, 
	-			  (char *) &sb.dev_roles[grub_le_to_cpu32 (sb.dev_number)]
	-			  - (char *) &sb,
	+			  (char *) &sb.dev_roles
	+			  - (char *) sb
	+			  + grub_le_to_cpu32 (sb.dev_number) * sizeof(grub_uint16_t),
				  sizeof (role), &role))
		return NULL;
 

I'm seeing,

	...
	gcc -DHAVE_CONFIG_H -I.  -Wall -W -DGRUB_UTIL=1 -D_FILE_OFFSET_BITS=64 -I./include -DGRUB_FILE=\"grub-core/disk/mdraid1x_linux.c\" -I. -I. -I. -I. -I./include -I./include -I./grub-core/lib/libgcrypt-grub/src/  -I./grub-core/lib/minilzo -I./grub-core/lib/xzembed -I./grub-core/lib/zstd -DMINILZO_HAVE_CONFIG_H -O3 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fmessage-length=0 -grecord-gcc-switches -march=native -mtune=native -D_FORTIFY_SOURCE=2 -D_FILE_OFFSET_BITS=64 -Wall -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces -Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type -Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs -Wunknown-pragmas -Wunused -Wunused-function -Wunused-label -Wunused-parameter -Wunused-value  -Wunused-variable -Wwrite-strings -Wnested-externs -Wstrict-prototypes -Wcast-align  -Wextra -Wattributes -Wendif-labels -Winit-self -Wint-to-pointer-cast -Winvalid-pch -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla -Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros -Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs -Wmissing-prototypes -Wmissing-declarations -Wformat=2 -Werror  -fno-builtin -Wno-undef -O3 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fmessage-length=0 -grecord-gcc-switches -march=native -mtune=native -MT grub-core/disk/libgrubmods_a-mdraid1x_linux.o -MD -MP -MF grub-core/disk/.deps-util/libgrubmods_a-mdraid1x_linux.Tpo -c -o grub-core/disk/libgrubmods_a-mdraid1x_linux.o `test -f 'grub-core/disk/mdraid1x_linux.c' || echo './'`grub-core/disk/mdraid1x_linux.c
	grub-core/disk/mdraid1x_linux.c: In function ‘grub_mdraid_detect’:
	grub-core/disk/mdraid1x_linux.c:183:6: error: cannot convert to a pointer type
	  183 |      + grub_le_to_cpu32 (sb.dev_number) * sizeof(grub_uint16_t),
	      |      ^
	make[2]: *** [Makefile:7059: grub-core/disk/libgrubmods_a-mdraid1x_linux.o] Error 1

my patch attempt wrong? or still needs tweaks?

Re: grub/head build with pre-release GCC10 ; fail @ "grub-core/disk/mdraid1x_linux.c:181:15: error: ..."

[PGNet Dev]

my bad re: patch. pebkac (looking UP might help :-/ )

with

	diff -ur grub.ORIG/grub-core/disk/mdraid1x_linux.c grub/grub-core/disk/mdraid1x_linux.c
	--- grub.ORIG/grub-core/disk/mdraid1x_linux.c	2020-03-24 09:24:08.656640265 -0700
	+++ grub/grub-core/disk/mdraid1x_linux.c	2020-03-24 09:58:27.638619782 -0700
	@@ -179,7 +179,8 @@
	 
	       if (grub_disk_read (disk, sector, 
				  (char *) &sb.dev_roles[grub_le_to_cpu32 (sb.dev_number)]
	-			  - (char *) &sb,
	+			  ((char *) &sb.dev_roles - (char *) sb)
	+			  + grub_le_to_cpu32 (sb.dev_number) * sizeof(grub_uint16_t),
				  sizeof (role), &role))
		return NULL;

gcc10 build returns,

	...
	make
		...
		grub-core/disk/mdraid1x_linux.c: In function ‘grub_mdraid_detect’:
		grub-core/disk/mdraid1x_linux.c:182:6: error: cannot convert to a pointer type
		  182 |      ((char *) &sb.dev_roles - (char *) sb)
		      |      ^
		grub-core/disk/mdraid1x_linux.c:181:16: error: called object is not a function or function pointer
		  181 |      (char *) &sb.dev_roles[grub_le_to_cpu32 (sb.dev_number)]
		      |                ^~
		make[2]: *** [Makefile:7059: grub-core/disk/libgrubmods_a-mdraid1x_linux.o] Error 1
		make[2]: Leaving directory '/usr/local/src/grub'
		make[1]: *** [Makefile:11920: all-recursive] Error 1
		make[1]: Leaving directory '/usr/local/src/grub'
		make: *** [Makefile:3772: all] Error 2

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

[Thomas Schmitt]

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[]".


Have a nice day :)

Thomas

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

[Michael Chang]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

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

[-- Attachment #2: 0001-mdraid1x_linux-Fix-gcc10-error-Werror-array-bounds.patch --]
[-- Type: text/x-patch, Size: 2291 bytes --]

From 19f55c05ea592b1339ee0d503c86be349447553f Mon Sep 17 00:00:00 2001
From: Michael Chang <mchang@suse.com>
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 <mchang@suse.com>
---
 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


[-- Attachment #3: 0002-zfs-Fix-gcc10-error-Werror-zero-length-bounds.patch --]
[-- Type: text/x-patch, Size: 3512 bytes --]

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];
 }
 
 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

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

[Thomas Schmitt]

Hi,

Michael Chang wrote in patch 2/2:
> ../../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]
> [...]
> The l_hash[0] is apparnetly an interior member to the enclosed structure
> [...] the l_entries[0] is used to get proper alignment to access leaf chunks

That's what i call a dirty hack.

I wonder what gcc would say to a union of [0]-sized arrays as last
member of the struct type:

  struct typedef struct zap_leaf_phys {
        ...
        union {
                grub_uint16_t l_hash[0];
                grub_properly_aligned_t l_entries[0];
        } l_;
  } zap_leaf_phys_t;

Mine likes such a gesture, but it is way behind in respect to modern bitrot.


So in the end, your patch looks like the solid and unambiguous way to
implement what is desired.


> It would be great if you can help to test patch to solve the build
> problem for gcc-10 in your system or not.

Due to lack of modernity i can only contribute statements that the
concepts and motivations of your two patches look good to me.

(I also lack the occasion to test the two use cases.
 Just lurking here for grub-mkrescue issues, where i provide the last stage
 of packing up the ISO image.)


Have a nice day :)

Thomas

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

[Michael Chang]

On Wed, Mar 25, 2020 at 12:13:49PM +0100, Thomas Schmitt wrote:
> Hi,
> 
> Michael Chang wrote in patch 2/2:
> > ../../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]
> > [...]
> > The l_hash[0] is apparnetly an interior member to the enclosed structure
> > [...] the l_entries[0] is used to get proper alignment to access leaf chunks
> 
> That's what i call a dirty hack.
> 
> I wonder what gcc would say to a union of [0]-sized arrays as last
> member of the struct type:
> 
>   struct typedef struct zap_leaf_phys {
>         ...
>         union {
>                 grub_uint16_t l_hash[0];
>                 grub_properly_aligned_t l_entries[0];
>         } l_;
>   } zap_leaf_phys_t;

As far as I know the union will store different types in same memory
location, thus the allocation is determined by the largest member in the
union, so does the padding has to meet the alignment requirement by the
largest member.

So in this case l_hash[0] (2 bytes) will be aligned to
grub_properly_aligned_t (8 bytes) and for the result is desired or not
really depends on what you want to achieve.

> 
> Mine likes such a gesture, but it is way behind in respect to modern bitrot.

Admittedly that would produce much more readable code, but it is just
not work for the extraordinary case here because of specific requirment
to the alignment.

> 
> 
> So in the end, your patch looks like the solid and unambiguous way to
> implement what is desired.
> 
> 
> > It would be great if you can help to test patch to solve the build
> > problem for gcc-10 in your system or not.
> 
> Due to lack of modernity i can only contribute statements that the
> concepts and motivations of your two patches look good to me.
> 
> (I also lack the occasion to test the two use cases.
>  Just lurking here for grub-mkrescue issues, where i provide the last stage
>  of packing up the ISO image.)

The grub-mkrescue is a gem, it never disappoints me whenever I need it.
:)

Thanks a lot for your feedback.

Michael

> 
> 
> Have a nice day :)
> 
> Thomas
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

[PGNet Dev]

On 3/25/20 12:27 AM, Michael Chang wrote:
> It would be great if you can help to test patch to solve the build
> problem for gcc-10 in your system or not.

with your patches, no immediate mdraid1x or zfs build errors
one does surface, now, for ntfscomp ...

gcc --version
	gcc (SUSE Linux) 10.0.1 20200320 (experimental) [revision 7d4549b2cd209eb621453ce13be7ffd84ffa720a]

cd grub
git log -n1
      1 commit 552c9fd08122a3036c724ce96dfe68aa2f75705f (HEAD -> master, origin/master, origin/HEAD)
      2 Author: Patrick Steinhardt <ps@pks.im>
      3 Date:   Sat Mar 7 17:29:09 2020 +0100

patch -p1 < /tmp/grub_patches/0001-mdraid1x_linux-Fix-gcc10-error-Werror-array-bounds.patch
	patching file grub-core/disk/mdraid1x_linux.c
patch -p1 < /tmp/grub_patches/0002-zfs-Fix-gcc10-error-Werror-zero-length-bounds.patch
	patching file grub-core/fs/zfs/zfs.c
	patching file include/grub/zfs/zap_leaf.h

unset CC CPP
./bootstrap
./autogen.sh
./configure
make V=1 -j${CORES}
	...
	make[2]: Entering directory '/usr/local/src/grub'
	gcc -DHAVE_CONFIG_H -I.  -Wall -W -DGRUB_UTIL=1 -D_FILE_OFFSET_BITS=64 -I./include -DGRUB_FILE=\"grub-core/fs/ntfscomp.c\" -I. -I. -I. -I. -I./include -I./include -I./grub-core/lib/libgcrypt-grub/src/  -I./grub-core/lib/minilzo -I./grub-core/lib/xzembed -I./grub-core/lib/zstd -DMINILZO_HAVE_CONFIG_H -O3 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fmessage-length=0 -grecord-gcc-switches -march=native -mtune=native -D_FORTIFY_SOURCE=2 -D_FILE_OFFSET_BITS=64 -Wall -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces -Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type -Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs -Wunknown-pragmas -Wunused -Wunused-function -Wunused-label -Wunused-parameter -Wunused-value  -Wunused-variable -Wwrite-strings -Wnested-externs -Wstrict-prototypes -Wcast-align  -Wextra -Wattributes -Wendif-labels -Winit-self -Wint-to-pointer-cast -Winvalid-pch -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla -Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros -Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs -Wmissing-prototypes -Wmissing-declarations -Wformat=2 -Werror  -fno-builtin -Wno-undef -O3 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fmessage-length=0 -grecord-gcc-switches -march=native -mtune=native -MT grub-core/fs/libgrubmods_a-ntfscomp.o -MD -MP -MF grub-core/fs/.deps-util/libgrubmods_a-ntfscomp.Tpo -c -o grub-core/fs/libgrubmods_a-ntfscomp.o `test -f 'grub-core/fs/ntfscomp.c' || echo './'`grub-core/fs/ntfscomp.c
	grub-core/fs/ntfscomp.c: In function ‘read_block’:
	grub-core/fs/ntfscomp.c:82:11: error: ‘flg’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
	   82 |       if (flg & 0x8000)
	      |           ^~~
	grub-core/fs/ntfscomp.c:74:17: note: ‘flg’ was declared here
	   74 |   grub_uint16_t flg, cnt;
	      |                 ^~~
	cc1: all warnings being treated as errors
	make[2]: *** [Makefile:7647: grub-core/fs/libgrubmods_a-ntfscomp.o] Error 1
	make[2]: Leaving directory '/usr/local/src/grub'
	make[1]: *** [Makefile:11920: all-recursive] Error 1
	make[1]: Leaving directory '/usr/local/src/grub'
	make: *** [Makefile:3772: all] Error 2

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

[Paul Menzel]

Dear PGNet Dev,


Am 25.03.20 um 16:35 schrieb PGNet Dev:

[…]

> with your patches, no immediate mdraid1x or zfs build errors
> one does surface, now, for ntfscomp ...

[…]

Thanks, but please follow the mailing list netiquette, and start a new 
discussion thread with a descriptive subject line.


Kind regards,

Paul

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

[PGNet Dev]

On 3/25/20 8:52 AM, Paul Menzel wrote:
> Thanks, but please follow the mailing list netiquette

I was responding to a question asked by a developer in THIS thread

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

[Paul Menzel]

Dear PGNet Dev,


Am 25.03.20 um 16:54 schrieb PGNet Dev:
> On 3/25/20 8:52 AM, Paul Menzel wrote:
>> Thanks, but please follow the mailing list netiquette
> 
> I was responding to a question asked by a developer in THIS thread

Yes, the question was, if the reported(?) build issues were fixed, and 
you replied to them.

But, you new build error, which I quoted, should be discussed and fixed 
in a separate thread with the correct descriptive subject line.


Kind regards,

Paul

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

[Michael Chang]

On Wed, Mar 25, 2020 at 08:35:58AM -0700, PGNet Dev wrote:
> On 3/25/20 12:27 AM, Michael Chang wrote:
> > It would be great if you can help to test patch to solve the build
> > problem for gcc-10 in your system or not.
> 
> with your patches, no immediate mdraid1x or zfs build errors

Thanks a lot for your test and validation.

> one does surface, now, for ntfscomp ...

I am also using openSUSE, somehow I didn't have the ntfscomp build error
on my gcc-10 build test. :(

Would you mind to start a new thread for it ?

Thanks,
Michael

> 
> gcc --version
> 	gcc (SUSE Linux) 10.0.1 20200320 (experimental) [revision 7d4549b2cd209eb621453ce13be7ffd84ffa720a]
> 
> cd grub
> git log -n1
>       1 commit 552c9fd08122a3036c724ce96dfe68aa2f75705f (HEAD -> master, origin/master, origin/HEAD)
>       2 Author: Patrick Steinhardt <ps@pks.im>
>       3 Date:   Sat Mar 7 17:29:09 2020 +0100
> 
> patch -p1 < /tmp/grub_patches/0001-mdraid1x_linux-Fix-gcc10-error-Werror-array-bounds.patch
> 	patching file grub-core/disk/mdraid1x_linux.c
> patch -p1 < /tmp/grub_patches/0002-zfs-Fix-gcc10-error-Werror-zero-length-bounds.patch
> 	patching file grub-core/fs/zfs/zfs.c
> 	patching file include/grub/zfs/zap_leaf.h
> 
> unset CC CPP
> ./bootstrap
> ./autogen.sh
> ./configure
> make V=1 -j${CORES}
> 	...
> 	make[2]: Entering directory '/usr/local/src/grub'
> 	gcc -DHAVE_CONFIG_H -I.  -Wall -W -DGRUB_UTIL=1 -D_FILE_OFFSET_BITS=64 -I./include -DGRUB_FILE=\"grub-core/fs/ntfscomp.c\" -I. -I. -I. -I. -I./include -I./include -I./grub-core/lib/libgcrypt-grub/src/  -I./grub-core/lib/minilzo -I./grub-core/lib/xzembed -I./grub-core/lib/zstd -DMINILZO_HAVE_CONFIG_H -O3 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fmessage-length=0 -grecord-gcc-switches -march=native -mtune=native -D_FORTIFY_SOURCE=2 -D_FILE_OFFSET_BITS=64 -Wall -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces -Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type -Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs -Wunknown-pragmas -Wunused -Wunused-function -Wunused-label -Wunused-parameter -Wunused-value  -Wunused-variable -Wwrite-strings -Wnested-externs -Wstrict-prototypes -Wcast-align  -Wextra -Wattributes -Wendif-labels -Winit-self -Wint-to-pointer-cast -Winvalid-pch -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla -Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros -Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs -Wmissing-prototypes -Wmissing-declarations -Wformat=2 -Werror  -fno-builtin -Wno-undef -O3 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fmessage-length=0 -grecord-gcc-switches -march=native -mtune=native -MT grub-core/fs/libgrubmods_a-ntfscomp.o -MD -MP -MF grub-core/fs/.deps-util/libgrubmods_a-ntfscomp.Tpo -c -o grub-core/fs/libgrubmods_a-ntfscomp.o `test -f 'grub-core/fs/ntfscomp.c' || echo './'`grub-core/fs/ntfscomp.c
> 	grub-core/fs/ntfscomp.c: In function ‘read_block’:
> 	grub-core/fs/ntfscomp.c:82:11: error: ‘flg’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 	   82 |       if (flg & 0x8000)
> 	      |           ^~~
> 	grub-core/fs/ntfscomp.c:74:17: note: ‘flg’ was declared here
> 	   74 |   grub_uint16_t flg, cnt;
> 	      |                 ^~~
> 	cc1: all warnings being treated as errors
> 	make[2]: *** [Makefile:7647: grub-core/fs/libgrubmods_a-ntfscomp.o] Error 1
> 	make[2]: Leaving directory '/usr/local/src/grub'
> 	make[1]: *** [Makefile:11920: all-recursive] Error 1
> 	make[1]: Leaving directory '/usr/local/src/grub'
> 	make: *** [Makefile:3772: all] Error 2

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

[PGNet Dev]

On 3/25/20 8:29 PM, Michael Chang wrote:
>> with your patches, no immediate mdraid1x or zfs build errors
> 
> Thanks a lot for your test and validation.
> 
>> one does surface, now, for ntfscomp ...
> 
> I am also using openSUSE, somehow I didn't have the ntfscomp build error
> on my gcc-10 build test. :(
> 
> Would you mind to start a new thread for it ?
> 
> Thanks,
> Michael

done

-->	https://lists.gnu.org/archive/html/grub-devel/2020-03/msg00240.html

o/

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

> >From 19f55c05ea592b1339ee0d503c86be349447553f Mon Sep 17 00:00:00 2001
> From: Michael Chang <mchang@suse.com>
> 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 <mchang@suse.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

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

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

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

[Michael Chang]

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

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

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

disk/mdraid1x_linux.c:181:15: warning: array subscript <unknown> is outside array bounds of ‘grub_uint16_t[0]’ {aka ‘short unsigned int[0]’} [-Warray-bounds]

[Paul Menzel]

Dear GRUB folks,


Using Debian Sid/unstable with

     gcc (Debian 10-20200312-2) 10.0.1 20200312 (experimental) [master 
revision c56871dd15a:7ba6e7f0f21:daf2852b883762d921361462dad1f99320faca2a]

building GRUB fails with the error below due to treating warnings as errors.

> gcc -DHAVE_CONFIG_H -I. -I..  -Wall -W  -DGRUB_MACHINE_EFI=1 -DGRUB_MACHINE=X86_64_EFI -m64 -nostdinc -isystem /usr/lib/gcc/x86_64-linux-gnu/10/include -I../include -I../include -DGRUB_FILE=\"disk/mdraid1x_linux.c\" -I. -I. -I.. -I.. -I../include -I../include -I../grub-core/lib/libgcrypt-grub/src/    -D_FILE_OFFSET_BITS=64 -Os -m64 -Wall -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces -Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type -Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs -Wunknown-pragmas -Wunused -Wunused-function -Wunused-label -Wunused-parameter -Wunused-value  -Wunused-variable -Wwrite-strings -Wnested-externs -Wstrict-prototypes -g -Wredundant-decls -Wmissing-prototypes -Wmissing-declarations  -Wextra -Wattributes -Wendif-labels -Winit-self -Wint-to-pointer-cast -Winvalid-pch -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla -Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros -Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs -Wmissing-prototypes -Wmissing-declarations -Wformat=2 -freg-struct-return -mno-mmx -mno-sse -mno-sse2 -mno-sse3 -mno-3dnow -msoft-float -fno-dwarf2-cfi-asm -mno-stack-arg-probe -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-ident -mcmodel=large -mno-red-zone -fno-PIE -fno-pie -fno-stack-protector -Wtrampolines   -ffreestanding   -MT disk/mdraid1x_module-mdraid1x_linux.o -MD -MP -MF disk/.deps-core/mdraid1x_module-mdraid1x_linux.Tpo -c -o disk/mdraid1x_module-mdraid1x_linux.o `test -f 'disk/mdraid1x_linux.c' || echo './'`disk/mdraid1x_linux.c
> disk/mdraid1x_linux.c: In function ‘grub_mdraid_detect’:
> disk/mdraid1x_linux.c:181:15: warning: array subscript <unknown> is outside array bounds of ‘grub_uint16_t[0]’ {aka ‘short unsigned int[0]’} [-Warray-bounds]
>   181 |      (char *) &sb.dev_roles[grub_le_to_cpu32 (sb.dev_number)]
>       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> disk/mdraid1x_linux.c:98:17: note: while referencing ‘dev_roles’
>    98 |   grub_uint16_t dev_roles[]; /* Role in array, or 0xffff for a spare, or 0xfffe for faulty.  */
>       |                 ^~~~~~~~~
> disk/mdraid1x_linux.c:127:33: note: defined here ‘sb’
>   127 |       struct grub_raid_super_1x sb;
>       |

Code in question seems to be:

       if (grub_disk_read (disk, sector,
                           (char *) &sb.dev_roles[grub_le_to_cpu32 
(sb.dev_number)]
                           - (char *) &sb,
                           sizeof (role), &role))
         return NULL;

Normally, it should be fixed by using `grub_uint16_t[]` instead of 
`grub_uint16_t[0]`, but I haven’t found out where yet.


Kind regards,

Paul

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

[Thomas Schmitt]

Hi,

Paul Menzel wrote:
>   181 |      (char *) &sb.dev_roles[grub_le_to_cpu32 (sb.dev_number)]
>    98 |   grub_uint16_t dev_roles[]; /* Role in array, or 0xffff for a
>   127 |       struct grub_raid_super_1x sb;
> [...]
> Normally, it should be fixed by using `grub_uint16_t[]` instead of
> `grub_uint16_t[0]`, but I haven’t found out where yet.

I rather propose to consider this untested and not properly styled
change:

--- grub-core/disk/mdraid1x_linux.c	2018-09-05 11:41:09.690721520 +0200
+++ grub-core/disk/mdraid1x_linux.c.ts	2020-03-20 13:57:21.159675792 +0100
@@ -178,8 +178,9 @@ 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,
+			  ((char *) &sb.dev_roles - (char *) sb)
+			  + grub_le_to_cpu32 (sb.dev_number) * sizeof(grub_uint16_t),
 			  sizeof (role), &role))
 	return NULL;

------------------------------------------------------------------------
Reasoning:

I see
  grub_uint16_t dev_roles[0];
in
  http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/disk/mdraid1x_linux.c#n98
It's a gcc extension.
  https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Declaring-Arrays
  "As a GNU extension, the number of elements can be as small as zero.
   Zero-length arrays are useful as the last element of a structure which
   is really a header for a variable-length object"

So isn't the problem rather about the allocation of the struct which
hosts .dev_roles ?
Currently there is in mdraid1x_linux.c:

  struct grub_raid_super_1x
  {
    ...
    grub_uint16_t dev_roles[0];   /* Role in array, or 0xffff for a spare, or 0xfffe for faulty.  */
  };

  ...

  static struct grub_diskfilter_vg *
  grub_mdraid_detect (grub_disk_t disk, ...
    ...
      struct grub_raid_super_1x sb;

The allocation as local variable does not appear to provide this extra
memory storage, which shall host the array members of dev_roles.
I fail to see how sb could get enlarged later. The stack neighbors of sb
do not look like they would provide their storage for an array.

Now why didn't this fail earlier ?
That's because the bad array index use is not for memory access but for
pointer arithmetics:

 http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/disk/mdraid1x_linux.c#n180

 if (grub_disk_read (disk, sector,
          (char *) &sb.dev_roles[grub_le_to_cpu32 (sb.dev_number)]
           - (char *) &sb,
          sizeof (role), &role))

The code wants a number which shall be used as parameter grub_off_t offset
of grub_disk_read() (in grub-core/kern/disk.c).

I think that the following expression produces the same number without
virtual access to a virtual array member:

         (char *) &sb.dev_roles - (char *) sb
         + grub_le_to_cpu32 (sb.dev_number) * sizeof(grub_uint16_t)


Have a nice day :)

Thomas