On Thu, Mar 17, 2022 at 02:43:41PM +0800, Michael Chang via Grub-devel wrote:
> The grub is failing to build with gcc-12 in many places like this:
>
> In function 'init_cbfsdisk',
> inlined from 'grub_mod_init' at ../../grub-core/fs/cbfs.c:391:3:
> ../../grub-core/fs/cbfs.c:345:7: error: array subscript 0 is outside array
> bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
> 345 | ptr = *(grub_uint32_t *) 0xfffffffc;
> | ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This is caused by gcc regression in 11/12 [1]. In a nut shell, the
> warning is about detected invalid accesses at non-zero offsets to null
May I ask you to be more consistent and use NULL instead of null everywhere?
> pointers. Since hardwired constant address is treated as NULL plus an
> offset in the same underlying code, the warning is therefore triggered.
>
> Instead of inserting #pragma all over the places where literal pointers
> are accessed to avoid diagnosing array-bounds, we can try to borrow the
> idea from linux kernel that the absolute_pointer macro [2][3] is used to
> disconnect a pointer using literal address from it's original object,
> hence gcc won't be able to make assumptions on the boundary while doing
> pointer arithmetic. With that we can greatly reduce the code we have to
> cover up by making initial literal pointer assignment to use the new
> wrapper but not having to track everywhere literal pointers are
> accessed. This also makes code looks cleaner.
>
> [1]
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> [2]
> https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler.h#L180
> [3]
> https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler-gcc.h#L31
>
> Signed-off-by: Michael Chang <mch...@suse.com>
[...]
> diff --git a/grub-core/commands/i386/pc/drivemap.c
> b/grub-core/commands/i386/pc/drivemap.c
> index 3fb22dc460..5e13f82eb5 100644
> --- a/grub-core/commands/i386/pc/drivemap.c
> +++ b/grub-core/commands/i386/pc/drivemap.c
> @@ -31,9 +31,6 @@
>
> GRUB_MOD_LICENSE ("GPLv3+");
>
> -/* Real mode IVT slot (seg:off far pointer) for interrupt 0x13. */
> -static grub_uint32_t *const int13slot = (grub_uint32_t *) (4 * 0x13);
> -
> /* Remember to update enum opt_idxs accordingly. */
> static const struct grub_arg_option options[] = {
> /* TRANSLATORS: In this file "mapping" refers to a change GRUB makes so if
> @@ -280,6 +277,9 @@ install_int13_handler (int noret __attribute__ ((unused)))
> grub_uint8_t *handler_base = 0;
> /* Address of the map within the deployed bundle. */
> int13map_node_t *handler_map;
> + /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13. */
> + grub_uint32_t *int13slot = (grub_uint32_t *) grub_absolute_pointer (4 *
> 0x13);
This shuffling looks strange and should be explained in the commit message.
I understood what is going here when I took a look at patch #3.
> +
Please drop this empty line addition.
> int i;
> int entries = 0;
> @@ -354,6 +354,9 @@ install_int13_handler (int noret __attribute__ ((unused)))
> static grub_err_t
> uninstall_int13_handler (void)
> {
> + /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13. */
> + grub_uint32_t *int13slot = (grub_uint32_t *) grub_absolute_pointer (4 *
> 0x13);
WRT shuffling, ditto.
> +
> if (! grub_drivemap_oldhandler)
> return GRUB_ERR_NONE;
[...]
> diff --git a/grub-core/term/i386/pc/console.c
> b/grub-core/term/i386/pc/console.c
> index d44937c305..9403390f1f 100644
> --- a/grub-core/term/i386/pc/console.c
> +++ b/grub-core/term/i386/pc/console.c
> @@ -238,12 +238,11 @@ grub_console_getkey (struct grub_term_input *term
> __attribute__ ((unused)))
> return (regs.eax & 0xff) + (('a' - 1) | GRUB_TERM_CTRL);
> }
>
> -static const struct grub_machine_bios_data_area *bios_data_area =
> - (struct grub_machine_bios_data_area *)
> GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR;
> -
> static int
> grub_console_getkeystatus (struct grub_term_input *term __attribute__
> ((unused)))
> {
> + const struct grub_machine_bios_data_area *bios_data_area =
> + (struct grub_machine_bios_data_area *) grub_absolute_pointer
> (GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR);
Ditto and below...
> /* conveniently GRUB keystatus is modelled after BIOS one. */
> return bios_data_area->keyboard_flag_lower & ~0x80;
> }
[...]
> diff --git a/include/grub/compiler.h b/include/grub/compiler.h
> index 8f3be3ae70..e159f0e292 100644
> --- a/include/grub/compiler.h
> +++ b/include/grub/compiler.h
> @@ -56,4 +56,15 @@
> # define CLANG_PREREQ(maj,min) 0
> #endif
>
> +#if defined(__GNUC__)
> +# define grub_absolute_pointer(val) \
> +({ \
> + unsigned long __ptr; \
I think grub_addr_t would be more appropriate here. But this requires
include/grub/types.h. So, maybe we should move this macro there.
> + __asm__ ("" : "=r"(__ptr) : "0"((void *)(val))); \
s/__asm__/asm/
Why did you decide to use "asm() version" of this macro [1] instead of more
C-ish one [2]? Anyway, I would mention the idea comes from the Linux
kernel RELOC_HIDE() macro.
Daniel
[1]
https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler-gcc.h#L31
[2]
https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler.h#L180
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
