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

[Daniel Kiper]

On Sun, Feb 28, 2021 at 02:13:06PM -0600, Glenn Washburn wrote:
> 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.

That would be nice...

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

Yeah, I think we should add more than that... And we have to have a tool
which checks formatting in the patches too.

Daniel