From patchwork Thu Oct 4 05:44:27 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?S=C3=B8ren_Sandmann?= X-Patchwork-Id: 189050 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 08E5A2C036D for ; Thu, 4 Oct 2012 15:44:40 +1000 (EST) Received: from localhost ([::1]:41209 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJeEg-0001C5-83 for incoming@patchwork.ozlabs.org; Thu, 04 Oct 2012 01:44:38 -0400 Received: from eggs.gnu.org ([208.118.235.92]:39824) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJeEZ-0001C0-3h for qemu-devel@nongnu.org; Thu, 04 Oct 2012 01:44:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TJeEX-0008By-Op for qemu-devel@nongnu.org; Thu, 04 Oct 2012 01:44:31 -0400 Received: from smtp.nfit.au.dk ([130.225.17.180]:42811 helo=nysmtp.nfit.au.dk) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJeEX-0008Bj-Cc for qemu-devel@nongnu.org; Thu, 04 Oct 2012 01:44:29 -0400 Received: from llama10.cs.au.dk (unknown [10.11.82.10]) by nysmtp.nfit.au.dk (Postfix) with ESMTP id D3F00E0C82; Thu, 4 Oct 2012 07:44:27 +0200 (CEST) From: sandmann@cs.au.dk (=?utf-8?Q?S=C3=B8ren?= Sandmann) To: Stefan Weil References: <1349287498-10475-1-git-send-email-sandmann@cs.au.dk> <506C81B8.908@weilnetz.de> <506CA3E4.1010009@weilnetz.de> Date: Thu, 04 Oct 2012 07:44:27 +0200 In-Reply-To: <506CA3E4.1010009@weilnetz.de> (Stefan Weil's message of "Wed, 3 Oct 2012 22:45:24 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 130.225.17.180 Cc: qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH] Fix compilation on GCC 4.5 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Stefan Weil writes: > That's strange. > > The lines which cause compiler errors look like this: > > vfio_eoi(DO_UPCAST(VFIODevice, bars[bar->nr], bar)); > > There are more uses of DO_UPCAST without any compiler error: > > VFIODevice *vdev = DO_UPCAST(VFIODevice, pdev, pdev); > > Neither of both lines creates a compiler error with any of my > compilers (gcc 4.4 up to latest gcc, Linux and MinGW hosts). Maybe the difference is that bars[bar->nr] is an expression whereas pdev is just an identifier. But on closer look, the macro actually looks pretty suspicious to me. The comment above it that says it does compile time checking, is probably not true. In the case where 'field' is bars[bar->nr] I'd be very surprised if the compiler is smart enough to optimize the array away entirely. It would have to reason like this: The output of __builtin_offset is always >= 0, so the size of the array can only ever be negative or zero, which means the array can be deleted because if the length isn't zero, the program invokes undefined behavior. It *could* do this, but does it, and is that something qemu should rely on? Even if this probably small runtime hit is deemed acceptable, it's likely surprising to many users of the macro that the 'field' expression is evaluated twice, so if the "compile time checking" is staying, maybe it should be done with something like this instead: #endif This compiles with my GCC, but I haven't tested the resulting binary at all. > When I compile with gcc option -save-temps, I get this code > for the first line: > > vfio_eoi(( __extension__ ( { char __attribute__((unused)) > offset_must_be_zero[ -__builtin_offsetof (VFIODevice, bars[bar->nr])]; > ({ const typeof(((VFIODevice *) 0)->bars[bar->nr]) *__mptr = (bar); > (VFIODevice *) ((char *) __mptr - __builtin_offsetof (VFIODevice, > bars[bar->nr]));});}))); I get what appears to be exactly the same: vfio_eoi(( __extension__ ( { char __attribute__((unused)) offset_must_be_zero[ -__builtin_offsetof (VFIODevice, bars[bar->nr])]; ({ const typeof(((VFIODevice *) 0)->bars[bar->nr]) *__mptr = (bar); (VFIODevice *) ((char *) __mptr - __builtin_offsetof (VFIODevice, bars[bar->nr]));});}))); > Could you please replace the first line by that code and try your compiler? > If it remains silent, I suspect a bad offsetof macro. Replacing the vfio_eoi[...] lines with the expansion that you get doesn't change anything. It still produces a warning which becomes an error due to -Werror. > Do you compile on Linux? Which distribution or which C library do you > use? Fedora 14. dhcp-100-3-184:~% rpm -q glibc glibc-2.13-2.x86_64 glibc-2.13-2.i686 Here is a small program that reproduces the behavior: dhcp-100-3-184:~% cat offset.c struct foo { int bar[17]; }; int external (void); int main () { struct foo baz = { { 0 } }; __extension__ ( { char offset_must_be_zero [ - __builtin_offsetof (struct foo, bar[external()])] __attribute ((unused)); }); return baz.bar[8]; } dhcp-100-3-184:~% gcc -Werror -W -Wall -Wunused offset.c cc1: warnings being treated as errors offset.c: In function ‘main’: offset.c:16:25: error: value computed is not used Søren diff --git a/osdep.h b/osdep.h index cb213e0..51ffab0 100644 --- a/osdep.h +++ b/osdep.h @@ -42,10 +42,11 @@ typedef signed int int_fast16_t; /* Convert from a base type to a parent type, with compile time checking. */ #ifdef __GNUC__ -#define DO_UPCAST(type, field, dev) ( __extension__ ( { \ - char __attribute__((unused)) offset_must_be_zero[ \ - -offsetof(type, field)]; \ - container_of(dev, type, field);})) +#define DO_UPCAST(type, field, dev) ( __extension__ ( { \ + const typeof(((type *) 0)->field) *__mptr = (dev); \ + ssize_t __offset = - offsetof (type, field); \ + char __attribute__((unused)) offset_must_be_zero[ __offset ]; \ + ((type *) ((char *) __mptr + __offset)); } ) ) #else #define DO_UPCAST(type, field, dev) container_of(dev, type, field)