On Tue, Mar 22, 2022 at 10:19:26PM +0100, Daniel Kiper wrote:
> 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?
Yes, no problem.
>
> > 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.
Yes the shuffling was due to the same reasoning provided in patch 3.
I'll include it to flesh out in the commit message.
>
> > +
>
> Please drop this empty line addition.
OK.
>
> > 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.
OK.
>
> > +
> > 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...
OK.
>
> > /* 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.
Looks good to me. I will do in next version,
>
> > + __asm__ ("" : "=r"(__ptr) : "0"((void *)(val))); \
>
> s/__asm__/asm/
OK.
>
> Why did you decide to use "asm() version" of this macro [1] instead of more
> C-ish one [2]?
The C-ish one doesn't work as it is acting as fallback for compilers
other than gcc without the need for such hack.
> Anyway, I would mention the idea comes from the Linux
> kernel RELOC_HIDE() macro.
I will add a comment to referencing it.
Thanks a lot for review,
Michael
>
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
