Re: [PATCH v4 10/13] error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error

[Glenn Washburn]

On Sat, 27 Feb 2021 12:48:11 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Feb 18, 2021 at 08:47:11PM -0600, Glenn Washburn wrote:
> > The macro ELF_R_TYPE does not change the underlying type. Here its
> > argument is a 64-bit Elf64_Xword. Make sure the format code matches.
> >
> > For the riscv architecture, rel->r_info could be either Elf32_Xword
> > or Elf64_Xword depending on if 32 or 64-bit risc is being built. So
> > cast to 64-bit value regardless.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/efiemu/i386/loadcore64.c | 3 ++-
> >  grub-core/kern/arm64/dl.c          | 3 ++-
> >  grub-core/kern/ia64/dl.c           | 3 ++-
> >  grub-core/kern/riscv/dl.c          | 5 +++--
> >  grub-core/kern/sparc64/dl.c        | 3 ++-
> >  grub-core/kern/x86_64/dl.c         | 3 ++-
> >  6 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/grub-core/efiemu/i386/loadcore64.c
> > b/grub-core/efiemu/i386/loadcore64.c index 18facf47f..3e9a71cfd
> > 100644 --- a/grub-core/efiemu/i386/loadcore64.c
> > +++ b/grub-core/efiemu/i386/loadcore64.c
> > @@ -122,7 +122,8 @@ grub_arch_efiemu_relocate_symbols64
> > (grub_efiemu_segment_t segs, break;
> >               default:
> >                 return grub_error
> > (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > -                                  N_("relocation 0x%x is not
> > implemented yet"),
> > +                                  N_("relocation
> > 0x%"PRIxGRUB_UINT64_T
> 
> Next time please add space between '"' and PRIxGRUB_UINT64_T.
> I will fix this before committing.

Okay, hadn't noticed that rule. I'll see about adding a patch that
cleans that up in a few other places in the code too.

> > diff --git a/grub-core/kern/riscv/dl.c b/grub-core/kern/riscv/dl.c
> > index 6fb8385ef..77f5b7648 100644
> > --- a/grub-core/kern/riscv/dl.c
> > +++ b/grub-core/kern/riscv/dl.c
> > @@ -331,8 +331,9 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod,
> > void *ehdr, break;
> >     default:
> >       return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> > -                        N_("relocation 0x%x is not
> > implemented yet"),
> > -                        ELF_R_TYPE (rel->r_info));
> > +                        N_("relocation 0x%"PRIxGRUB_UINT64_T
> > +                           " is not implemented yet"),
> > +                        (grub_uint64_t)ELF_R_TYPE
> > (rel->r_info));
> 
> Missing space between "(grub_uint64_t)" and ELF_R_TYPE().

Hadn't noticed this rule. Grepping the source shows that it is super
common to not have a space after cast. Perhaps this would be good to
add to the developer documentation.

Glenn