Re: [Qemu-devel] [PULL 18/76] optionrom: add new PVH option rom

[Stefano Garzarella]

On Fri, Mar 19, 2021 at 7:25 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Mar 19, 2021 at 7:20 PM Stefano Garzarella <sgarzare@redhat.com> 
> wrote:
> >
> > On Fri, Mar 19, 2021 at 06:52:39PM +0100, Paolo Bonzini wrote:
> > >It's likely that the compiler will online it. But indeed it's better to add
> > >-minline-all-stringops to the compiler command line.
> > >
> >
> > Cool, I didn't know that one!
> > I tried but I did something wrong because the linker is not happy, next
> > week I'll check better:
> > ld: pvh_main.o: in function `search_rsdp':
> > /home/stefano/repos/qemu/pc-bios/optionrom/pvh_main.c:62: undefined 
> > reference to `memcmp'
> > ld: /home/stefano/repos/qemu/pc-bios/optionrom/pvh_main.c:62: undefined 
> > reference to `memcmp'
> >
> >
> > In the mean time, I simply tried to assign the RSDP_SIGNATURE macro to
> > an uint64_t variable and I have this new warning, using gcc 10.2.1 "cc
> > (Alpine 10.2.1_pre2) 10.2.1 20210313":
> >
> > In file included from /tmp/qemu-test/src/pc-bios/optionrom/pvh_main.c:25:
> > /tmp/qemu-test/src/pc-bios/optionrom/pvh_main.c: In function 'search_rsdp':
> > /tmp/qemu-test/src/pc-bios/optionrom/pvh_main.c:30:42: warning: conversion 
> > from 'long long unsigned int' to 'uint64_t' {aka 'long unsigned int'} 
> > changes value from '2329016660419433298' to '541348690' [-Woverflow]
> >     30 | #define RSDP_SIGNATURE          UINT64_C(0x2052545020445352) /* 
> > "RSD PTR " */
> >        |                                          ^~~~~~~~~~~~~~~~~~
> > /tmp/qemu-test/src/pc-bios/optionrom/pvh_main.c:69:31: note: in expansion 
> > of macro 'RSDP_SIGNATURE'
> >     69 |     uint64_t rsdp_signature = RSDP_SIGNATURE;
> >        |
> >
> > Using gcc (GCC) 10.2.1 20201125 (Red Hat 10.2.1-9) I don't have it.
> >
> > It seems a bit strange, I don't know if it's related to the fact that we
> > compile with -m16, I'll check better.
>
> Anyway I think that using memcmp() I can switch to a character array to
> store the signature, but this gcc warnings confused me a bit...
>

I'll send a patch to add a simple implementation of memcmp and use it 
since I wasn't able to embedded it with -minline-all-stringops.

In the mean time, I played a bit with sizeof() to understand what's 
going on and I think there is something strange with Alpine gcc 
10.2.1_pre2.

I added this 3 lines in pvh_main.c to print the sizes at compile time 
(maybe there is a better way :-):

+    char (*__size1)[sizeof(uint64_t)] = 1;
+    char (*__size2)[sizeof(UINT64_C(1))] = 1;
+    char (*__size3)[sizeof(UINT64_C(0x2052545020445352))] = 1;


If I build with gcc 10.2.1 20201125 (Red Hat 10.2.1-9) everything looks 
normal, since they are all 8 bytes:

   warning: initialization of ‘char (*)[8]’ from ‘int’ makes pointer from 
integer without a cast [-Wint-conversion]
   74 |     char (*__size1)[sizeof(uint64_t)] = 1;
      |                                         ^
   warning: initialization of ‘char (*)[8]’ from ‘int’ makes pointer from 
integer without a cast [-Wint-conversion]
   75 |     char (*__size2)[sizeof(UINT64_C(1))] = 1;
      |                                            ^
   warning: initialization of ‘char (*)[8]’ from ‘int’ makes pointer from 
integer without a cast [-Wint-conversion]
   76 |     char (*__size3)[sizeof(UINT64_C(0x2052545020445352))] = 1;


If I build with gcc 10.2.1 20210313 (Alpine 10.2.1_pre2) uint64_t and 
UINT64_C(1) have a size of 4 bytes, while UINT64_C(0x2052545020445352) 
has a size of 8 bytes:

   warning: initialization of ‘char (*)[4]’ from ‘int’ makes pointer from 
integer without a cast [-Wint-conversion]
   74 |     char (*__size1)[sizeof(uint64_t)] = 1;
      |                                         ^
   warning: initialization of ‘char (*)[4]’ from ‘int’ makes pointer from 
integer without a cast [-Wint-conversion]
   75 |     char (*__size2)[sizeof(UINT64_C(1))] = 1;
      |                                            ^
   warning: initialization of ‘char (*)[8]’ from ‘int’ makes pointer from 
integer without a cast [-Wint-conversion]
   76 |     char (*__size3)[sizeof(UINT64_C(0x2052545020445352))] = 1;

This is a bit strange...

Thanks,
Stefano