Linux Virtualization - May 2013 - [PATCH RFC] virtio-pci: new config layout: using memory BAR

Paolo Bonzini <pbonzini at redhat.com> writes:
> Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto:
>>>> > > +
>>>> > > +    switch (addr) {
>>>> > > +    case offsetof(struct virtio_pci_common_cfg,
device_feature_select):
>>>> > > +        return proxy->device_feature_select;
>>> > 
>>> > Oh dear no...  Please use defines like the rest of QEMU.
>> Any good reason not to use offsetof?
>
> I'm not sure it's portable to use it in case labels.  IIRC, the
> definition ((int)&(((T *)0)->field)) is not a valid C integer
constant
> expression.  Laszlo?
It's defined to yield an integer constant expression in the ISO standard
(and I think ANSI too, though that's not at hand):

    7.19, para 3:

        ...offsetof(type, member-designator)
        which expands to an integer constant expression that has type
        size_t, ...

The real question is whether compilers qemu cares about meet the
standard (there's some evidence that older compilers fail this).  If
not, we'll have to define them as raw offsets...

Cheers,
Rusty.

Il 29/05/2013 06:33, Rusty Russell ha scritto:
> Paolo Bonzini <pbonzini at redhat.com> writes:
>> Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto:
>>>>>>> +
>>>>>>> +    switch (addr) {
>>>>>>> +    case offsetof(struct virtio_pci_common_cfg,
device_feature_select):
>>>>>>> +        return proxy->device_feature_select;
>>>>>
>>>>> Oh dear no...  Please use defines like the rest of QEMU.
>>> Any good reason not to use offsetof?
>>
>> I'm not sure it's portable to use it in case labels.  IIRC, the
>> definition ((int)&(((T *)0)->field)) is not a valid C integer
constant
>> expression.  Laszlo?
> 
> It's defined to yield an integer constant expression in the ISO
standard
> (and I think ANSI too, though that's not at hand):
It's not in C89.  The oldest compiler QEMU cares about is GCC 4.2.  I
don't know if it has a builtin offsetof, probably it does.  But I'm not
sure about other users of virtio headers.

Paolo
> 
>     7.19, para 3:
> 
>         ...offsetof(type, member-designator)
>         which expands to an integer constant expression that has type
>         size_t, ...
> 
> The real question is whether compilers qemu cares about meet the
> standard (there's some evidence that older compilers fail this).  If
> not, we'll have to define them as raw offsets...
> 
> Cheers,
> Rusty.
>

On Wed, May 29, 2013 at 09:27:02AM +0200, Paolo Bonzini
wrote:
> Il 29/05/2013 06:33, Rusty Russell ha scritto:
> > Paolo Bonzini <pbonzini at redhat.com> writes:
> >> Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto:
> >>>>>>> +
> >>>>>>> +    switch (addr) {
> >>>>>>> +    case offsetof(struct
virtio_pci_common_cfg, device_feature_select):
> >>>>>>> +        return
proxy->device_feature_select;
> >>>>>
> >>>>> Oh dear no...  Please use defines like the rest of
QEMU.
> >>> Any good reason not to use offsetof?
> >>
> >> I'm not sure it's portable to use it in case labels. 
IIRC, the
> >> definition ((int)&(((T *)0)->field)) is not a valid C
integer constant
> >> expression.  Laszlo?
> > 
> > It's defined to yield an integer constant expression in the ISO
standard
> > (and I think ANSI too, though that's not at hand):
> 
> It's not in C89.  The oldest compiler QEMU cares about is GCC 4.2.  I
> don't know if it has a builtin offsetof, probably it does.
Yes, I think __builtin_offsetof was added in 4.0:
http://gcc.gnu.org/onlinedocs/gcc-4.0.0/gcc/Offsetof.html

> But I'm not sure about other users of virtio headers.
> 
> Paolo
> > 
> >     7.19, para 3:
> > 
> >         ...offsetof(type, member-designator)
> >         which expands to an integer constant expression that has type
> >         size_t, ...
> > 
> > The real question is whether compilers qemu cares about meet the
> > standard (there's some evidence that older compilers fail this). 
If
> > not, we'll have to define them as raw offsets...
So I think the question is whether we care about GCC 3.  We used to when
mingw in debian stable was GCC 3, but not anymore ...
> > 
> > Cheers,
> > Rusty.
> >

On 05/29/13 09:27, Paolo Bonzini wrote:
> Il 29/05/2013 06:33, Rusty Russell ha scritto:
>> Paolo Bonzini <pbonzini at redhat.com> writes:
>>> Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto:
>>>>>>>> +
>>>>>>>> +    switch (addr) {
>>>>>>>> +    case offsetof(struct
virtio_pci_common_cfg, device_feature_select):
>>>>>>>> +        return
proxy->device_feature_select;
>>>>>>
>>>>>> Oh dear no...  Please use defines like the rest of
QEMU.
>>>> Any good reason not to use offsetof?
>>>
>>> I'm not sure it's portable to use it in case labels.  IIRC,
the
>>> definition ((int)&(((T *)0)->field)) is not a valid C
integer constant
>>> expression.  Laszlo?
>>
>> It's defined to yield an integer constant expression in the ISO
standard
>> (and I think ANSI too, though that's not at hand):
> 
> It's not in C89.
It is, see 7.1.6 Common definitions <stddef.h>.

> The oldest compiler QEMU cares about is GCC 4.2.  I
> don't know if it has a builtin offsetof, probably it does.
Talking about nothing else but the ISO C standard(s), it doesn't matter
how the C implementation deals with offsetof() internally. "C
implementation" in this sense includes both compiler and standard library.

If the standard library header (<stddef.h> or something internal
included by it) expands the offsetof() macro to replacement text that
does pointer black magic, magic that would be otherwise nonportable or
undefined, it is alright as long as the accompanying compiler guarantees
that the replacement text will work.

That is, offsetof() gives the C implementor leeway to implement it in
wherever "distribution" between compiler and standard library; the
main
thing is that the C program use the one offsetof() provided by the C
implementation.

Of course in the FLOSS world OS distros might mix and match gcc and
glibc statistically randomly; normally some documentation should enable
the user to put his/her system into ISO C or even SUSv[1234] mode.

(

An interesting example for, say, "non-unified implementation" is
"getconf" vs. "c99". I'll pick SUSv3 for this example.

http://pubs.opengroup.org/onlinepubs/000095399/utilities/getconf.html
http://pubs.opengroup.org/onlinepubs/000095399/utilities/c99.html

In a nutshell one can interrogate getconf for CFLAGS, LDFLAGS and LIBS
so that c99 builds a program in ILP32_OFF32 / ILP32_OFFBIG / LP64_OFF64
/ LPBIG_OFFBIG mode ("programming environment").

On my x86_64 RHEL-6.4 laptop, "getconf" is part of glibc
(glibc-common-2.12-1.107.el6.x86_64), while c99 is part of gcc
(gcc-4.4.7-3.el6.x86_64).

Supposing I'd like to build a 32-bit program with 64-bit off_t:

  getconf _POSIX_V6_ILP32_OFFBIG
  1

  getconf -v POSIX_V6_ILP32_OFFBIG POSIX_V6_ILP32_OFFBIG_CFLAGS
  -m32 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64

  getconf -v POSIX_V6_ILP32_OFFBIG POSIX_V6_ILP32_OFFBIG_LDFLAGS
  -m32

  getconf -v POSIX_V6_ILP32_OFFBIG POSIX_V6_ILP32_OFFBIG_LIBS
  [empty string]

However if I try to actually build a program using these flags:

  #define _XOPEN_SOURCE 600
  int main(void) { return 0; }

  c99 \
      -m32 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 \
      -o mytest \
      -m32 \
      mytest.c

I get

  /usr/bin/ld: crt1.o: No such file: No such file or directory
  collect2: ld returned 1 exit status

unless I install "glibc-devel.i686".

This problem is rooted in "getconf" (a glibc utility) being unaware of
RHEL packaging choices: if the system can't guarantee the final c99
invocation to succeed, then the very first "getconf" invocation should
write "-1\n" or "undefined\n" to stdout.

(I'm aware that RHEL-6 is not certified UNIX 03; this is just an example
for problems caused by various parts of a standard's
(quasi-)implementation coming from separate projects. In that sense,
caring about the offsetof() internals may have merit.)

)

Laszlo