[Qemu-devel] packed structures and unaligned accesses (sparc)

[Qemu-devel] packed structures and unaligned accesses (sparc)

[Peter Maydell]

At the moment the 9p QEMU tests fail on SPARC. This turns out to
be because the test case itself gets a SIGBUS. Looking at the
code I guess it makes sense, but I don't understand why the
code didn't at least generate a warning. Here's a cutdown testcase:

pm215@stadler:~$ cat packed.c
#include <stdio.h>
#include <inttypes.h>

typedef struct {
  uint32_t size;
  uint8_t id;
  uint16_t tag;
} __attribute__((packed)) hdr;

uint32_t getval(uint32_t *p) {
  return *p;
}

uint32_t foo(void) {
  hdr h;
  h.size = 42;

  return getval(&h.size);
}

int main(void) {
  printf("got 0x%x\n", foo());
  return 0;
}
pm215@stadler:~$ gcc -Wall -o packed packed.c
pm215@stadler:~$ ./packed
Bus error

(This is with gcc (Debian 6.3.0-10) 6.3.0 20170321.)

The bus error happens because:
 * the compiler decides to put the local 'hdr h' in foo() at
an unaligned address (which is allowed although I'm not
quite sure why it chooses to do so here given it's the only
local in the function)
 * getval() is compiled to code that assumes the pointer is
aligned
 * the address of h.size isn't aligned, so the call to getval()
blows up

That all makes sense in isolation, but shouldn't something have
at least warned that "&h.size" isn't actually a uint32_t* in
the sense of being something you can validly pass to a
function that takes a uint32_t* ?

thanks
-- PMM

Re: [Qemu-devel] packed structures and unaligned accesses (sparc)

[Richard Henderson]

On 03/28/2017 03:34 AM, Peter Maydell wrote:
> That all makes sense in isolation, but shouldn't something have
> at least warned that "&h.size" isn't actually a uint32_t* in
> the sense of being something you can validly pass to a
> function that takes a uint32_t* ?

That's a long-known bug in the implementation of packed data.  Yes, it should, 
but it doesn't.


r~

Re: [Qemu-devel] packed structures and unaligned accesses (sparc)

[John Paul Adrian Glaubitz]

On 03/27/2017 07:34 PM, Peter Maydell wrote:
> That all makes sense in isolation, but shouldn't something have
> at least warned that "&h.size" isn't actually a uint32_t* in
> the sense of being something you can validly pass to a
> function that takes a uint32_t* ?

This seems to be related to this gcc bug report [1].

Adrian

> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] packed structures and unaligned accesses (sparc)

[John Paul Adrian Glaubitz]

On 03/27/2017 07:34 PM, Peter Maydell wrote:
> That all makes sense in isolation, but shouldn't something have
> at least warned that "&h.size" isn't actually a uint32_t* in
> the sense of being something you can validly pass to a
> function that takes a uint32_t* ?

It turns out that clang actually emits a warning in this case:

root@deb4g:~# clang-4.0 packed.c -o packed
packed.c:18:18: warning: taking address of packed member 'size' of class or structure 'hdr' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
  return getval(&h.size);
                 ^~~~~~
1 warning generated.
root@deb4g:~#

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] packed structures and unaligned accesses (sparc)

[Peter Maydell]

On 28 March 2017 at 11:19, John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On 03/27/2017 07:34 PM, Peter Maydell wrote:
>> That all makes sense in isolation, but shouldn't something have
>> at least warned that "&h.size" isn't actually a uint32_t* in
>> the sense of being something you can validly pass to a
>> function that takes a uint32_t* ?
>
> It turns out that clang actually emits a warning in this case:
>
> root@deb4g:~# clang-4.0 packed.c -o packed
> packed.c:18:18: warning: taking address of packed member 'size' of class or structure 'hdr' may result in an unaligned pointer value
>       [-Waddress-of-packed-member]
>   return getval(&h.size);
>                  ^~~~~~
> 1 warning generated.
> root@deb4g:~#

Ah, good. (Unfortunately my clang isn't new enough for that,
being 3.8.)

thanks
-- PMM