[PATCH 0/6] Various fixes and cleanups

[PATCH 0/6] Various fixes and cleanups

[Daniel Kiper]

Hey,

Mostly build fixes plus one cleanup and INSTALL file update.

Daniel

 INSTALL                              |  3 ++-
 conf/i386-cygwin-img-ld.sc           |  4 ++++
 configure.ac                         |  4 ++--
 grub-core/commands/i386/pc/sendkey.c |  8 ++++----
 grub-core/loader/i386/bsd.c          |  2 +-
 grub-core/osdep/windows/platform.c   | 10 ++++++++++
 6 files changed, 23 insertions(+), 8 deletions(-)

Daniel Kiper (6):
      osdep/windows/platform: Disable gcc9 -Waddress-of-packed-member
      loader/i386/bsd: Initialize ptr variable in grub_bsd_add_meta()
      commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error
      conf/i386-cygwin-img-ld: Do not discard .data and .edata sections
      configure: Drop ${grub_coredir} unneeded references
      INSTALL: Add more cross-compiling Debian packages

[PATCH 1/6] osdep/windows/platform: Disable gcc9 -Waddress-of-packed-member

[Daniel Kiper]

$ ./configure --target=x86_64-w64-mingw32 --with-platform=efi --host=x86_64-w64-mingw32
$ make

[...]

In file included from grub-core/osdep/platform.c:4:
grub-core/osdep/windows/platform.c: In function ‘grub_install_register_efi’:
grub-core/osdep/windows/platform.c:382:41: error: taking address of packed member of ‘struct grub_efi_file_path_device_path’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
  382 |   path16_len = grub_utf8_to_utf16 (filep->path_name,
      |                                    ~~~~~^~~~~~~~~~~

Disable the -Wadress-of-packaed-member diagnostic for grub_utf8_to_utf16()
call which contains filep->path_name reference. It seems safe because the
structure is defined according to the UEFI spec and we hope authors did not
make any mistake... :-)

This fix is similar to the fix in the commit 8e8723a6b
(f2fs: Disable gcc9 -Waddress-of-packed-member).

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/osdep/windows/platform.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/grub-core/osdep/windows/platform.c b/grub-core/osdep/windows/platform.c
index 253f8d101..075776d50 100644
--- a/grub-core/osdep/windows/platform.c
+++ b/grub-core/osdep/windows/platform.c
@@ -379,10 +379,20 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
   filep->header.type = GRUB_EFI_MEDIA_DEVICE_PATH_TYPE;
   filep->header.subtype = GRUB_EFI_FILE_PATH_DEVICE_PATH_SUBTYPE;
 
+#if __GNUC__ >= 9
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
+#endif
+
   path16_len = grub_utf8_to_utf16 (filep->path_name,
 				   path8_len * GRUB_MAX_UTF16_PER_UTF8,
 				   (const grub_uint8_t *) efifile_path,
 				   path8_len, 0);
+
+#if __GNUC__ >= 9
+#pragma GCC diagnostic pop
+#endif
+
   filep->path_name[path16_len] = 0;
   filep->header.length = sizeof (*filep) + (path16_len + 1) * sizeof (grub_uint16_t);
   pathptr = &filep->path_name[path16_len + 1];
-- 
2.11.0

[PATCH 2/6] loader/i386/bsd: Initialize ptr variable in grub_bsd_add_meta()

[Daniel Kiper]

Latest GCC may complain in that way:

  In file included from ../include/grub/disk.h:31,
                   from ../include/grub/file.h:26,
                   from ../include/grub/loader.h:23,
                   from loader/i386/bsd.c:19:
  loader/i386/bsd.c: In function ‘grub_cmd_openbsd’:
  ../include/grub/misc.h:71:10: error: ‘ptr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     71 |   return grub_memmove (dest, src, n);
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
  loader/i386/bsd.c:266:9: note: ‘ptr’ was declared here
    266 |   void *ptr;
        |         ^~~

So, let's fix it by assigning NULL to ptr in grub_bsd_add_meta().

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/loader/i386/bsd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index 5f3290ce1..346c4f14a 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -263,7 +263,7 @@ grub_err_t
 grub_bsd_add_meta (grub_uint32_t type, const void *data, grub_uint32_t len)
 {
   grub_err_t err;
-  void *ptr;
+  void *ptr = NULL;
 
   err = grub_bsd_add_meta_ptr (type, &ptr, len);
   if (err)
-- 
2.11.0

[PATCH 3/6] commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error

[Daniel Kiper]

Latest GCC may complain in that way:

  commands/i386/pc/sendkey.c: In function ‘grub_sendkey_postboot’:
  commands/i386/pc/sendkey.c:223:21: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
    223 |   *((char *) 0x41a) = 0x1e;
        |   ~~~~~~~~~~~~~~~~~~^~~~~~

The volatile keyword addition helps and additionally assures us the
compiler will not optimize out fixed assignments.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/commands/i386/pc/sendkey.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/grub-core/commands/i386/pc/sendkey.c b/grub-core/commands/i386/pc/sendkey.c
index 26d9acd3d..ab4bca9e9 100644
--- a/grub-core/commands/i386/pc/sendkey.c
+++ b/grub-core/commands/i386/pc/sendkey.c
@@ -220,8 +220,8 @@ grub_sendkey_postboot (void)
 
   *flags = oldflags;
 
-  *((char *) 0x41a) = 0x1e;
-  *((char *) 0x41c) = 0x1e;
+  *((volatile char *) 0x41a) = 0x1e;
+  *((volatile char *) 0x41c) = 0x1e;
 
   return GRUB_ERR_NONE;
 }
@@ -236,8 +236,8 @@ grub_sendkey_preboot (int noret __attribute__ ((unused)))
   oldflags = *flags;
   
   /* Set the sendkey.  */
-  *((char *) 0x41a) = 0x1e;
-  *((char *) 0x41c) = keylen + 0x1e;
+  *((volatile char *) 0x41a) = 0x1e;
+  *((volatile char *) 0x41c) = keylen + 0x1e;
   grub_memcpy ((char *) 0x41e, sendkey, 0x20);
 
   /* Transform "any ctrl" to "right ctrl" flag.  */
-- 
2.11.0

[PATCH 4/6] conf/i386-cygwin-img-ld: Do not discard .data and .edata sections

[Daniel Kiper]

$ ./configure --target=i686-w64-mingw32 --with-platform=efi --host=i686-w64-mingw32

[...]

checking if __bss_start is defined by the compiler... no
checking if edata is defined by the compiler... no
checking if _edata is defined by the compiler... no
configure: error: none of __bss_start, edata or _edata is defined

This happens on machines with quite recent ld due to an error:

  `edata' referenced in section `.text' of /tmp/cc72w9E4.o: defined in discarded section `.data' of conftest.exe
  collect2: error: ld returned 1 exit status

So, we have to tell linker to not discard .data and .edata sections.
The trick comes from ld documentation:

  3.6.7 Output Section Discarding

  The linker will not normally create output sections with no contents.
  This is for convenience when referring to input sections that may or may
  not be present in any of the input files. For example:

  .foo : { *(.foo) }

  will only create a ‘.foo’ section in the output file if there is a
  ‘.foo’ section in at least one input file, and if the input sections are
  not all empty. Other link script directives that allocate space in an
  output section will also create the output section. So too will
  assignments to dot even if the assignment does not create space, except
  for ‘. = 0’, ‘. = . + 0’, ‘. = sym’, ‘. = . + sym’ and ‘. = ALIGN (. !=
  0, expr, 1)’ when ‘sym’ is an absolute symbol of value 0 defined in the
  script. This allows you to force output of an empty section with ‘. = .’.

This change does not impact generated binaries because the
conf/i386-cygwin-img-ld.sc linker script is used only when
you run configure.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 conf/i386-cygwin-img-ld.sc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/conf/i386-cygwin-img-ld.sc b/conf/i386-cygwin-img-ld.sc
index 3ac26fcce..578da91b0 100644
--- a/conf/i386-cygwin-img-ld.sc
+++ b/conf/i386-cygwin-img-ld.sc
@@ -14,6 +14,8 @@ SECTIONS
   {
     __data_start__ = . ;
     *(.data)
+    /* Do not discard this section. */
+    . = . ;
     __data_end__ = . ;
     __rdata_start__ = . ;
     *(.rdata)
@@ -34,6 +36,8 @@ SECTIONS
   .edata :
   {
     *(.edata)
+    /* Do not discard this section. */
+    . = . ;
     end = . ;
     _end = . ;
     __end = . ;
-- 
2.11.0

[PATCH 5/6] configure: Drop ${grub_coredir} unneeded references

[Daniel Kiper]

These are probably stray references left after earlier removals.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 configure.ac | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 50f7bc6a0..560d40515 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1128,9 +1128,9 @@ if test x$grub_cv_target_cc_link_format = x-arch,i386 || test x$grub_cv_target_c
 elif test x$grub_cv_target_cc_link_format = x-mi386pe || test x$grub_cv_target_cc_link_format = x-mi386pep ; then
   TARGET_APPLE_LINKER=0
   TARGET_LDFLAGS_OLDMAGIC="-Wl,-N"
-  TARGET_IMG_LDSCRIPT='$(top_srcdir)'"/${grub_coredir}/conf/i386-cygwin-img-ld.sc"
+  TARGET_IMG_LDSCRIPT='$(top_srcdir)'"/conf/i386-cygwin-img-ld.sc"
   TARGET_IMG_LDFLAGS="-Wl,-T${TARGET_IMG_LDSCRIPT}"
-  TARGET_IMG_LDFLAGS_AC="-Wl,-T${srcdir}/${grub_coredir}/conf/i386-cygwin-img-ld.sc"
+  TARGET_IMG_LDFLAGS_AC="-Wl,-T${srcdir}/conf/i386-cygwin-img-ld.sc"
   TARGET_IMG_BASE_LDOPT="-Wl,-Ttext"
   TARGET_IMG_CFLAGS=
 else
-- 
2.11.0

[PATCH 6/6] INSTALL: Add more cross-compiling Debian packages

[Daniel Kiper]

The mingw-w64-tools is especially important because with out it some
Windows builds may fail due to lack of proper pkg-config.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 INSTALL | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/INSTALL b/INSTALL
index a64f63723..aff44f75c 100644
--- a/INSTALL
+++ b/INSTALL
@@ -52,7 +52,8 @@ need the following.
 
 Your distro may package cross-compiling toolchains such as the following
 incomplete list on Debian: gcc-aarch64-linux-gnu, gcc-arm-linux-gnueabihf,
-gcc-mipsel-linux-gnu, and mingw-w64.
+gcc-mips-linux-gnu, gcc-mipsel-linux-gnu, gcc-powerpc64-linux-gnu,
+gcc-riscv64-linux-gnu, gcc-sparc64-linux-gnu, mingw-w64 and mingw-w64-tools.
 
 More cross compiling toolchains can be found at the following trusted sites:
 
-- 
2.11.0

Re: [PATCH 1/6] osdep/windows/platform: Disable gcc9 -Waddress-of-packed-member

[Vladimir 'phcoder' Serbinenko]

LGTM

On Fri, Mar 11, 2022 at 12:38 AM Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> $ ./configure --target=x86_64-w64-mingw32 --with-platform=efi --host=x86_64-w64-mingw32
> $ make
>
> [...]
>
> In file included from grub-core/osdep/platform.c:4:
> grub-core/osdep/windows/platform.c: In function ‘grub_install_register_efi’:
> grub-core/osdep/windows/platform.c:382:41: error: taking address of packed member of ‘struct grub_efi_file_path_device_path’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
>   382 |   path16_len = grub_utf8_to_utf16 (filep->path_name,
>       |                                    ~~~~~^~~~~~~~~~~
>
> Disable the -Wadress-of-packaed-member diagnostic for grub_utf8_to_utf16()
> call which contains filep->path_name reference. It seems safe because the
> structure is defined according to the UEFI spec and we hope authors did not
> make any mistake... :-)
>
> This fix is similar to the fix in the commit 8e8723a6b
> (f2fs: Disable gcc9 -Waddress-of-packed-member).
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  grub-core/osdep/windows/platform.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/grub-core/osdep/windows/platform.c b/grub-core/osdep/windows/platform.c
> index 253f8d101..075776d50 100644
> --- a/grub-core/osdep/windows/platform.c
> +++ b/grub-core/osdep/windows/platform.c
> @@ -379,10 +379,20 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
>    filep->header.type = GRUB_EFI_MEDIA_DEVICE_PATH_TYPE;
>    filep->header.subtype = GRUB_EFI_FILE_PATH_DEVICE_PATH_SUBTYPE;
>
> +#if __GNUC__ >= 9
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
> +#endif
> +
>    path16_len = grub_utf8_to_utf16 (filep->path_name,
>                                    path8_len * GRUB_MAX_UTF16_PER_UTF8,
>                                    (const grub_uint8_t *) efifile_path,
>                                    path8_len, 0);
> +
> +#if __GNUC__ >= 9
> +#pragma GCC diagnostic pop
> +#endif
> +
>    filep->path_name[path16_len] = 0;
>    filep->header.length = sizeof (*filep) + (path16_len + 1) * sizeof (grub_uint16_t);
>    pathptr = &filep->path_name[path16_len + 1];
> --
> 2.11.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH 3/6] commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error

[Glenn Washburn]

On Fri, 11 Mar 2022 00:35:57 +0100
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> Latest GCC may complain in that way:
> 
>   commands/i386/pc/sendkey.c: In function ‘grub_sendkey_postboot’:
>   commands/i386/pc/sendkey.c:223:21: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
>     223 |   *((char *) 0x41a) = 0x1e;
>         |   ~~~~~~~~~~~~~~~~~~^~~~~~

I'm curious why I'm not seeing this. It looks like the target was
i386-pc, correct? I'm guessing this is on GCC11. What compiler are you
using? Any extra CFLAGS? Anything else I might need to reproduce?

Glenn

> 
> The volatile keyword addition helps and additionally assures us the
> compiler will not optimize out fixed assignments.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  grub-core/commands/i386/pc/sendkey.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/commands/i386/pc/sendkey.c b/grub-core/commands/i386/pc/sendkey.c
> index 26d9acd3d..ab4bca9e9 100644
> --- a/grub-core/commands/i386/pc/sendkey.c
> +++ b/grub-core/commands/i386/pc/sendkey.c
> @@ -220,8 +220,8 @@ grub_sendkey_postboot (void)
>  
>    *flags = oldflags;
>  
> -  *((char *) 0x41a) = 0x1e;
> -  *((char *) 0x41c) = 0x1e;
> +  *((volatile char *) 0x41a) = 0x1e;
> +  *((volatile char *) 0x41c) = 0x1e;
>  
>    return GRUB_ERR_NONE;
>  }
> @@ -236,8 +236,8 @@ grub_sendkey_preboot (int noret __attribute__ ((unused)))
>    oldflags = *flags;
>    
>    /* Set the sendkey.  */
> -  *((char *) 0x41a) = 0x1e;
> -  *((char *) 0x41c) = keylen + 0x1e;
> +  *((volatile char *) 0x41a) = 0x1e;
> +  *((volatile char *) 0x41c) = keylen + 0x1e;
>    grub_memcpy ((char *) 0x41e, sendkey, 0x20);
>  
>    /* Transform "any ctrl" to "right ctrl" flag.  */

Re: [PATCH 3/6] commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error

[Toomas Soome]

> On 11. Mar 2022, at 19:10, Glenn Washburn <development@efficientek.com> wrote:
> 
> On Fri, 11 Mar 2022 00:35:57 +0100
> Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
>> Latest GCC may complain in that way:
>> 
>>  commands/i386/pc/sendkey.c: In function ‘grub_sendkey_postboot’:
>>  commands/i386/pc/sendkey.c:223:21: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
>>    223 |   *((char *) 0x41a) = 0x1e;
>>        |   ~~~~~~~~~~~~~~~~~~^~~~~~
> 
> I'm curious why I'm not seeing this. It looks like the target was
> i386-pc, correct? I'm guessing this is on GCC11. What compiler are you
> using? Any extra CFLAGS? Anything else I might need to reproduce?
> 
> Glenn

gcc 11 can report those, I have used workaround of assigning address to variable.  It is interesting that volatile qualifier can pacify it too.

Rgds,
Toomas
> 
>> 
>> The volatile keyword addition helps and additionally assures us the
>> compiler will not optimize out fixed assignments.
>> 
>> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>> grub-core/commands/i386/pc/sendkey.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/grub-core/commands/i386/pc/sendkey.c b/grub-core/commands/i386/pc/sendkey.c
>> index 26d9acd3d..ab4bca9e9 100644
>> --- a/grub-core/commands/i386/pc/sendkey.c
>> +++ b/grub-core/commands/i386/pc/sendkey.c
>> @@ -220,8 +220,8 @@ grub_sendkey_postboot (void)
>> 
>>   *flags = oldflags;
>> 
>> -  *((char *) 0x41a) = 0x1e;
>> -  *((char *) 0x41c) = 0x1e;
>> +  *((volatile char *) 0x41a) = 0x1e;
>> +  *((volatile char *) 0x41c) = 0x1e;
>> 
>>   return GRUB_ERR_NONE;
>> }
>> @@ -236,8 +236,8 @@ grub_sendkey_preboot (int noret __attribute__ ((unused)))
>>   oldflags = *flags;
>> 
>>   /* Set the sendkey.  */
>> -  *((char *) 0x41a) = 0x1e;
>> -  *((char *) 0x41c) = keylen + 0x1e;
>> +  *((volatile char *) 0x41a) = 0x1e;
>> +  *((volatile char *) 0x41c) = keylen + 0x1e;
>>   grub_memcpy ((char *) 0x41e, sendkey, 0x20);
>> 
>>   /* Transform "any ctrl" to "right ctrl" flag.  */
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/6] commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error

[Vladimir 'phcoder' Serbinenko]

LGTM

On Fri, Mar 11, 2022 at 12:37 AM Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> Latest GCC may complain in that way:
>
>   commands/i386/pc/sendkey.c: In function ‘grub_sendkey_postboot’:
>   commands/i386/pc/sendkey.c:223:21: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
>     223 |   *((char *) 0x41a) = 0x1e;
>         |   ~~~~~~~~~~~~~~~~~~^~~~~~
>
> The volatile keyword addition helps and additionally assures us the
> compiler will not optimize out fixed assignments.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  grub-core/commands/i386/pc/sendkey.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/commands/i386/pc/sendkey.c b/grub-core/commands/i386/pc/sendkey.c
> index 26d9acd3d..ab4bca9e9 100644
> --- a/grub-core/commands/i386/pc/sendkey.c
> +++ b/grub-core/commands/i386/pc/sendkey.c
> @@ -220,8 +220,8 @@ grub_sendkey_postboot (void)
>
>    *flags = oldflags;
>
> -  *((char *) 0x41a) = 0x1e;
> -  *((char *) 0x41c) = 0x1e;
> +  *((volatile char *) 0x41a) = 0x1e;
> +  *((volatile char *) 0x41c) = 0x1e;
>
>    return GRUB_ERR_NONE;
>  }
> @@ -236,8 +236,8 @@ grub_sendkey_preboot (int noret __attribute__ ((unused)))
>    oldflags = *flags;
>
>    /* Set the sendkey.  */
> -  *((char *) 0x41a) = 0x1e;
> -  *((char *) 0x41c) = keylen + 0x1e;
> +  *((volatile char *) 0x41a) = 0x1e;
> +  *((volatile char *) 0x41c) = keylen + 0x1e;
>    grub_memcpy ((char *) 0x41e, sendkey, 0x20);
>
>    /* Transform "any ctrl" to "right ctrl" flag.  */
> --
> 2.11.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH 0/6] Various fixes and cleanups

[Robbie Harwood]

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

Daniel Kiper <daniel.kiper@oracle.com> writes:

> Hey,
>
> Mostly build fixes plus one cleanup and INSTALL file update.
>
> Daniel
>
>  INSTALL                              |  3 ++-
>  conf/i386-cygwin-img-ld.sc           |  4 ++++
>  configure.ac                         |  4 ++--
>  grub-core/commands/i386/pc/sendkey.c |  8 ++++----
>  grub-core/loader/i386/bsd.c          |  2 +-
>  grub-core/osdep/windows/platform.c   | 10 ++++++++++
>  6 files changed, 23 insertions(+), 8 deletions(-)
>
> Daniel Kiper (6):
>       osdep/windows/platform: Disable gcc9 -Waddress-of-packed-member
>       loader/i386/bsd: Initialize ptr variable in grub_bsd_add_meta()
>       commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error
>       conf/i386-cygwin-img-ld: Do not discard .data and .edata sections
>       configure: Drop ${grub_coredir} unneeded references
>       INSTALL: Add more cross-compiling Debian packages

For all in series:

I don't think any of these will break anything, but I haven't
experienced any of these errors so I can't confirm they fix anything
either.  Subject to that,

Reviewed-by: Robbie Harwood <rharwood@redhat.com>

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH 3/6] commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error

[Daniel Kiper]

On Fri, Mar 11, 2022 at 11:09:16AM -0600, Glenn Washburn wrote:
> On Fri, 11 Mar 2022 00:35:57 +0100
> Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > Latest GCC may complain in that way:
> >
> >   commands/i386/pc/sendkey.c: In function ‘grub_sendkey_postboot’:
> >   commands/i386/pc/sendkey.c:223:21: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
> >     223 |   *((char *) 0x41a) = 0x1e;
> >         |   ~~~~~~~~~~~~~~~~~~^~~~~~
>
> I'm curious why I'm not seeing this. It looks like the target was
> i386-pc, correct? I'm guessing this is on GCC11. What compiler are you

Debian GNU/Linux bookworm/sid
gcc version 11.2.0 (Debian 11.2.0-16)

> using? Any extra CFLAGS? Anything else I might need to reproduce?

IIRC e.g. ./configure --target=i386 --with-platform=pc

Daniel

Re: [PATCH 3/6] commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error

[Michael Chang]

On Fri, Mar 11, 2022 at 12:35:57AM +0100, Daniel Kiper wrote:
> Latest GCC may complain in that way:
> 
>   commands/i386/pc/sendkey.c: In function ‘grub_sendkey_postboot’:
>   commands/i386/pc/sendkey.c:223:21: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
>     223 |   *((char *) 0x41a) = 0x1e;
>         |   ~~~~~~~~~~~~~~~~~~^~~~~~
> 
> The volatile keyword addition helps and additionally assures us the
> compiler will not optimize out fixed assignments.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  grub-core/commands/i386/pc/sendkey.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/commands/i386/pc/sendkey.c b/grub-core/commands/i386/pc/sendkey.c
> index 26d9acd3d..ab4bca9e9 100644
> --- a/grub-core/commands/i386/pc/sendkey.c
> +++ b/grub-core/commands/i386/pc/sendkey.c
> @@ -220,8 +220,8 @@ grub_sendkey_postboot (void)
>  
>    *flags = oldflags;
>  
> -  *((char *) 0x41a) = 0x1e;
> -  *((char *) 0x41c) = 0x1e;
> +  *((volatile char *) 0x41a) = 0x1e;
> +  *((volatile char *) 0x41c) = 0x1e;
>  
>    return GRUB_ERR_NONE;
>  }
> @@ -236,8 +236,8 @@ grub_sendkey_preboot (int noret __attribute__ ((unused)))
>    oldflags = *flags;
>    
>    /* Set the sendkey.  */
> -  *((char *) 0x41a) = 0x1e;
> -  *((char *) 0x41c) = keylen + 0x1e;
> +  *((volatile char *) 0x41a) = 0x1e;
> +  *((volatile char *) 0x41c) = keylen + 0x1e;
>    grub_memcpy ((char *) 0x41e, sendkey, 0x20);
>  
>    /* Transform "any ctrl" to "right ctrl" flag.  */
> -- 
> 2.11.0

Unfortunately the volatile keyword doesn't help on a more recent gcc-12
build [1]. With that -Warray-bounds warning [2] is emitted this time
than -Wstringop-overflow.

I believe this is caused by this gcc regression in which hardwired
literal address is treated as NULL plus an offset, while there's no good
way to tell that literal address being an invalid accesses at non-zero
offsets to null pointers or not:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578

Before upstream comes up with a solution or introducing new annotation
for literal pointer. For the time being their recommedation is to
suppress the warning with #pragma or use volatile pointer which appears
to not work for gcc-12.

Instead of inserting #pragma all over the places to avoid the diagnose
of Warray-bounds. I think we can try to port absolute_pointer [3] and
RELOC_HIDE [4] used by the linux kernel to disconnect a pointer using
literal address from its original object hence gcc won't make
assumptions on the boundary while doing pointer arithmetic. This sounds
a better way to go as it can work across gcc versions.

I am halfway in preparing the patch to wrap literal pointers in
absolute_pointer. Before posting it here, I'd like to know do we have
any other prefernece like just disabling it via #pragma as that sounds
more like a norm?

Thanks,
Michael

[3]
https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler.h#L180

[4]
https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler-gcc.h#L31

[1]
abuild@local:~> gcc --version
gcc (SUSE Linux) 12.0.1 20220228 (experimental) [revision 37b583b9d7719f663656ce65ac822c11471fb540]
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[2]
[  195s] ../../grub-core/commands/i386/pc/sendkey.c: In function 'grub_sendkey_postboot':
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:221:3: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   221 |   *flags = oldflags;
[  195s]       |   ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:223:3: error: array subscript 0 is outside array bounds of 'volatile char[0]' [-Werror=array-bounds]
[  195s]   223 |   *((volatile char *) 0x41a) = 0x1e;
[  195s]       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:224:3: error: array subscript 0 is outside array bounds of 'volatile char[0]' [-Werror=array-bounds]
[  195s]   224 |   *((volatile char *) 0x41c) = 0x1e;
[  195s]       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
[  195s] ../../grub-core/term/ns8250.c: In function 'grub_ns8250_init':
[  195s] ../../grub-core/term/ns8250.c:242:26: error: array subscript 0 is outside array bounds of 'const short unsigned int[0]' [-Werror=array-bounds]
[  195s]   242 |     if (serial_hw_io_addr[i])
[  195s]       |         ~~~~~~~~~~~~~~~~~^~~
[  195s] ../../grub-core/term/ns8250.c:262:46: error: array subscript 0 is outside array bounds of 'const short unsigned int[0]' [-Werror=array-bounds]
[  195s]   262 |         com_ports[i].port = serial_hw_io_addr[i];
[  195s]       |                             ~~~~~~~~~~~~~~~~~^~~
[  195s] ../../grub-core/term/ns8250.c: In function 'grub_ns8250_hw_get_port':
[  195s] ../../grub-core/term/ns8250.c:277:29: error: array subscript 0 is outside array bounds of 'const short unsigned int[0]' [-Werror=array-bounds]
[  195s]   277 |     return serial_hw_io_addr[unit];
[  195s]       |            ~~~~~~~~~~~~~~~~~^~~~~~
[  195s] cc1: all warnings being treated as errors
[  195s] ../../grub-core/commands/i386/pc/sendkey.c: In function 'grub_sendkey_preboot':
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:236:14: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   236 |   oldflags = *flags;
[  195s]       |              ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:239:3: error: array subscript 0 is outside array bounds of 'volatile char[0]' [-Werror=array-bounds]
[  195s]   239 |   *((volatile char *) 0x41a) = 0x1e;
[  195s]       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:240:3: error: array subscript 0 is outside array bounds of 'volatile char[0]' [-Werror=array-bounds]
[  195s]   240 |   *((volatile char *) 0x41c) = keylen + 0x1e;
[  195s]       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:244:7: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   244 |   if (*flags & (1 << 8))
[  195s]       |       ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:245:5: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   245 |     *flags &= ~(1 << 2);
[  195s]       |     ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:248:7: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   248 |   if (*flags & (1 << 9))
[  195s]       |       ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:249:5: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   249 |     *flags &= ~(1 << 3);
[  195s]       |     ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:251:13: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   251 |   *flags = (*flags & andmask) | ormask;
[  195s]       |             ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:251:3: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   251 |   *flags = (*flags & andmask) | ormask;
[  195s]       |   ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:255:5: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   255 |     *flags |= (1 << 2);
[  195s]       |     ^~~~~~
[  195s] make[3]: *** [Makefile:46365: term/serial_module-ns8250.o] Error 1
[  195s] make[3]: *** Waiting for unfinished jobs....
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:258:7: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   258 |   if (*flags & (1 << 9))
[  195s]       |       ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:259:5: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   259 |     *flags |= (1 << 3);
[  195s]       |     ^~~~~~
[  195s] ../../grub-core/commands/i386/pc/sendkey.c:281:27: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
[  195s]   281 |               grub_outb ((*flags >> 4) & 7, 0x60);
[  195s]       |                           ^~~~~~
[  195s] cc1: all warnings being treated as errors

Thanks,
Michael

> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/6] commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error

[Daniel Kiper]

On Mon, Mar 14, 2022 at 12:16:16PM +0800, Michael Chang wrote:
> On Fri, Mar 11, 2022 at 12:35:57AM +0100, Daniel Kiper wrote:
> > Latest GCC may complain in that way:
> >
> >   commands/i386/pc/sendkey.c: In function ‘grub_sendkey_postboot’:
> >   commands/i386/pc/sendkey.c:223:21: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
> >     223 |   *((char *) 0x41a) = 0x1e;
> >         |   ~~~~~~~~~~~~~~~~~~^~~~~~
> >
> > The volatile keyword addition helps and additionally assures us the
> > compiler will not optimize out fixed assignments.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > ---
> >  grub-core/commands/i386/pc/sendkey.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/grub-core/commands/i386/pc/sendkey.c b/grub-core/commands/i386/pc/sendkey.c
> > index 26d9acd3d..ab4bca9e9 100644
> > --- a/grub-core/commands/i386/pc/sendkey.c
> > +++ b/grub-core/commands/i386/pc/sendkey.c
> > @@ -220,8 +220,8 @@ grub_sendkey_postboot (void)
> >
> >    *flags = oldflags;
> >
> > -  *((char *) 0x41a) = 0x1e;
> > -  *((char *) 0x41c) = 0x1e;
> > +  *((volatile char *) 0x41a) = 0x1e;
> > +  *((volatile char *) 0x41c) = 0x1e;
> >
> >    return GRUB_ERR_NONE;
> >  }
> > @@ -236,8 +236,8 @@ grub_sendkey_preboot (int noret __attribute__ ((unused)))
> >    oldflags = *flags;
> >
> >    /* Set the sendkey.  */
> > -  *((char *) 0x41a) = 0x1e;
> > -  *((char *) 0x41c) = keylen + 0x1e;
> > +  *((volatile char *) 0x41a) = 0x1e;
> > +  *((volatile char *) 0x41c) = keylen + 0x1e;
> >    grub_memcpy ((char *) 0x41e, sendkey, 0x20);
> >
> >    /* Transform "any ctrl" to "right ctrl" flag.  */
> > --
> > 2.11.0
>
> Unfortunately the volatile keyword doesn't help on a more recent gcc-12
> build [1]. With that -Warray-bounds warning [2] is emitted this time
> than -Wstringop-overflow.

Ugh... Though I have taken the patch because it at least fix the problem
partially.

> I believe this is caused by this gcc regression in which hardwired
> literal address is treated as NULL plus an offset, while there's no good
> way to tell that literal address being an invalid accesses at non-zero
> offsets to null pointers or not:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
>
> Before upstream comes up with a solution or introducing new annotation
> for literal pointer. For the time being their recommedation is to
> suppress the warning with #pragma or use volatile pointer which appears
> to not work for gcc-12.
>
> Instead of inserting #pragma all over the places to avoid the diagnose
> of Warray-bounds. I think we can try to port absolute_pointer [3] and
> RELOC_HIDE [4] used by the linux kernel to disconnect a pointer using
> literal address from its original object hence gcc won't make
> assumptions on the boundary while doing pointer arithmetic. This sounds
> a better way to go as it can work across gcc versions.
>
> I am halfway in preparing the patch to wrap literal pointers in
> absolute_pointer. Before posting it here, I'd like to know do we have
> any other prefernece like just disabling it via #pragma as that sounds
> more like a norm?

I think absolute_pointer() looks like a better solution.

Thank you for taking a stab at this.

Daniel

Re: [PATCH 6/6] INSTALL: Add more cross-compiling Debian packages

[Paul Menzel]

Dear Daniel,


Am 11.03.22 um 00:36 schrieb Daniel Kiper:
> The mingw-w64-tools is especially important because with out it some

without

> Windows builds may fail due to lack of proper pkg-config.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>   INSTALL | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/INSTALL b/INSTALL
> index a64f63723..aff44f75c 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -52,7 +52,8 @@ need the following.
>   
>   Your distro may package cross-compiling toolchains such as the following
>   incomplete list on Debian: gcc-aarch64-linux-gnu, gcc-arm-linux-gnueabihf,
> -gcc-mipsel-linux-gnu, and mingw-w64.
> +gcc-mips-linux-gnu, gcc-mipsel-linux-gnu, gcc-powerpc64-linux-gnu,
> +gcc-riscv64-linux-gnu, gcc-sparc64-linux-gnu, mingw-w64 and mingw-w64-tools.
>   
>   More cross compiling toolchains can be found at the following trusted sites:
>   

Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [PATCH 6/6] INSTALL: Add more cross-compiling Debian packages

[Daniel Kiper]

Paul,

On Thu, Mar 17, 2022 at 07:02:22AM +0100, Paul Menzel wrote:
> Dear Daniel,
>
> Am 11.03.22 um 00:36 schrieb Daniel Kiper:
> > The mingw-w64-tools is especially important because with out it some
>
> without
>
> > Windows builds may fail due to lack of proper pkg-config.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > ---
> >   INSTALL | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/INSTALL b/INSTALL
> > index a64f63723..aff44f75c 100644
> > --- a/INSTALL
> > +++ b/INSTALL
> > @@ -52,7 +52,8 @@ need the following.
> >   Your distro may package cross-compiling toolchains such as the following
> >   incomplete list on Debian: gcc-aarch64-linux-gnu, gcc-arm-linux-gnueabihf,
> > -gcc-mipsel-linux-gnu, and mingw-w64.
> > +gcc-mips-linux-gnu, gcc-mipsel-linux-gnu, gcc-powerpc64-linux-gnu,
> > +gcc-riscv64-linux-gnu, gcc-sparc64-linux-gnu, mingw-w64 and mingw-w64-tools.
> >   More cross compiling toolchains can be found at the following trusted sites:
>
> Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>

Sadly I committed the patch earlier. So, I am not able to fix it right now.
Though thank you for review.

Daniel

Re: [PATCH 2/6] loader/i386/bsd: Initialize ptr variable in grub_bsd_add_meta()

[Paul Menzel]

Dear Daniel,


Am 11.03.22 um 00:35 schrieb Daniel Kiper:
> Latest GCC may complain in that way:

Just for the record, what is the latest GCC for you? 12?

>    In file included from ../include/grub/disk.h:31,
>                     from ../include/grub/file.h:26,
>                     from ../include/grub/loader.h:23,
>                     from loader/i386/bsd.c:19:
>    loader/i386/bsd.c: In function ‘grub_cmd_openbsd’:
>    ../include/grub/misc.h:71:10: error: ‘ptr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>       71 |   return grub_memmove (dest, src, n);
>          |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>    loader/i386/bsd.c:266:9: note: ‘ptr’ was declared here
>      266 |   void *ptr;
>          |         ^~~
> 
> So, let's fix it by assigning NULL to ptr in grub_bsd_add_meta().
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>   grub-core/loader/i386/bsd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
> index 5f3290ce1..346c4f14a 100644
> --- a/grub-core/loader/i386/bsd.c
> +++ b/grub-core/loader/i386/bsd.c
> @@ -263,7 +263,7 @@ grub_err_t
>   grub_bsd_add_meta (grub_uint32_t type, const void *data, grub_uint32_t len)
>   {
>     grub_err_t err;
> -  void *ptr;
> +  void *ptr = NULL;
>   
>     err = grub_bsd_add_meta_ptr (type, &ptr, len);
>     if (err)

It’s a bogus warning though, as `grub_bsd_add_meta_ptr()` return an 
error when ptr wasn’t initialized yet, right?


Kind regards,

Paul

Re: [PATCH 2/6] loader/i386/bsd: Initialize ptr variable in grub_bsd_add_meta()

[Glenn Washburn]

On Fri, 18 Mar 2022 08:57:27 +0100
Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> Dear Daniel,
> 
> 
> Am 11.03.22 um 00:35 schrieb Daniel Kiper:
> > Latest GCC may complain in that way:
> 
> Just for the record, what is the latest GCC for you? 12?

I reported this issue on Debian 11 GCC version 10.2.1[1].

> 
> >    In file included from ../include/grub/disk.h:31,
> >                     from ../include/grub/file.h:26,
> >                     from ../include/grub/loader.h:23,
> >                     from loader/i386/bsd.c:19:
> >    loader/i386/bsd.c: In function ‘grub_cmd_openbsd’:
> >    ../include/grub/misc.h:71:10: error: ‘ptr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >       71 |   return grub_memmove (dest, src, n);
> >          |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    loader/i386/bsd.c:266:9: note: ‘ptr’ was declared here
> >      266 |   void *ptr;
> >          |         ^~~
> > 
> > So, let's fix it by assigning NULL to ptr in grub_bsd_add_meta().
> > 
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > ---
> >   grub-core/loader/i386/bsd.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
> > index 5f3290ce1..346c4f14a 100644
> > --- a/grub-core/loader/i386/bsd.c
> > +++ b/grub-core/loader/i386/bsd.c
> > @@ -263,7 +263,7 @@ grub_err_t
> >   grub_bsd_add_meta (grub_uint32_t type, const void *data, grub_uint32_t len)
> >   {
> >     grub_err_t err;
> > -  void *ptr;
> > +  void *ptr = NULL;
> >   
> >     err = grub_bsd_add_meta_ptr (type, &ptr, len);
> >     if (err)
> 
> It’s a bogus warning though, as `grub_bsd_add_meta_ptr()` return an 
> error when ptr wasn’t initialized yet, right?

Yes, as far as I can tell this is bogus and noted that this seems to be
a GCC issue[1]. I was only able to trigger this when using -O2, which
seems odd to me.

Glenn

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

> 
> 
> Kind regards,
> 
> Paul

Re: [PATCH 2/6] loader/i386/bsd: Initialize ptr variable in grub_bsd_add_meta()

[Daniel Kiper]

On Fri, Mar 18, 2022 at 02:21:23PM -0500, Glenn Washburn wrote:
> On Fri, 18 Mar 2022 08:57:27 +0100
> Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> > Dear Daniel,
> >
> >
> > Am 11.03.22 um 00:35 schrieb Daniel Kiper:
> > > Latest GCC may complain in that way:
> >
> > Just for the record, what is the latest GCC for you? 12?
>
> I reported this issue on Debian 11 GCC version 10.2.1[1].

Yeah, I could be more precise here.

> > >    In file included from ../include/grub/disk.h:31,
> > >                     from ../include/grub/file.h:26,
> > >                     from ../include/grub/loader.h:23,
> > >                     from loader/i386/bsd.c:19:
> > >    loader/i386/bsd.c: In function ‘grub_cmd_openbsd’:
> > >    ../include/grub/misc.h:71:10: error: ‘ptr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >       71 |   return grub_memmove (dest, src, n);
> > >          |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >    loader/i386/bsd.c:266:9: note: ‘ptr’ was declared here
> > >      266 |   void *ptr;
> > >          |         ^~~
> > >
> > > So, let's fix it by assigning NULL to ptr in grub_bsd_add_meta().
> > >
> > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > > ---
> > >   grub-core/loader/i386/bsd.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
> > > index 5f3290ce1..346c4f14a 100644
> > > --- a/grub-core/loader/i386/bsd.c
> > > +++ b/grub-core/loader/i386/bsd.c
> > > @@ -263,7 +263,7 @@ grub_err_t
> > >   grub_bsd_add_meta (grub_uint32_t type, const void *data, grub_uint32_t len)
> > >   {
> > >     grub_err_t err;
> > > -  void *ptr;
> > > +  void *ptr = NULL;
> > >
> > >     err = grub_bsd_add_meta_ptr (type, &ptr, len);
> > >     if (err)
> >
> > It’s a bogus warning though, as `grub_bsd_add_meta_ptr()` return an
> > error when ptr wasn’t initialized yet, right?
>
> Yes, as far as I can tell this is bogus and noted that this seems to be
> a GCC issue[1]. I was only able to trigger this when using -O2, which
> seems odd to me.

<handwaving>
I think compiler is not able to infer any sensible relation between
grub_errno value and value returned from grub_malloc() because here
compiler sees only grub_malloc() prototype. So, that is why it complains
here. Of course we are smarter because we know how grub_malloc() works.
Or at least we think we know how it works... :-)
</handwaving>

Anyway, I think it is the simplest fix for this issue. Additionally, it
puts us on safe side if we wrongly assumed correct grub_malloc() behavior
in all cases...

Daniel