Fedora rawhide has just moved onto gcc 4.8, and I have had to apply the
attached patch to get it to build (I constructed the patch against 4.2.1
but I believe it will apply to xen-unstable).
There are two types of problem, the first is from
xen/common/compat/memory.c where the error is
memory.c: In function ''compat_memory_op'':
/builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:35:33:
error: typedef ''__guest_handle_const_compat_memory_exchange_t''
locally
defined but not used [-Werror=unused-local-typedefs]
typedef struct { type *p; } __guest_handle_ ## name
^
/builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:43:5:
note: in expansion of macro ''___DEFINE_XEN_GUEST_HANDLE''
___DEFINE_XEN_GUEST_HANDLE(const_##name, const type)
^
/builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:44:41:
note: in expansion of macro ''__DEFINE_XEN_GUEST_HANDLE''
#define DEFINE_XEN_GUEST_HANDLE(name) __DEFINE_XEN_GUEST_HANDLE(name,
name)
^
memory.c:261:13: note: in expansion of macro
''DEFINE_XEN_GUEST_HANDLE''
DEFINE_XEN_GUEST_HANDLE(compat_memory_exchange_t);
^
so we are defining something that isn''t used.
Secondly gcc 4.8 objects to lines like
memset(ctxt, 0, sizeof(ctxt));
where I think you are just zeroing a pointer''s worth of memory. There
are
a few cases of this in the xen code fixed in the patch, and I am also
suspicious of line 630 of
stubdom/grub-upstream/stage2/fsys_reiserfs.c which is
memset (INFO->blocks, 0, sizeof (INFO->blocks));
which is noted in the build log but not flagged as an error.
Michael Young
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
On 06/02/2013 20:37, M A Young wrote:> Fedora rawhide has just moved onto gcc 4.8, and I have had to apply the > attached patch to get it to build (I constructed the patch against 4.2.1 > but I believe it will apply to xen-unstable). > > There are two types of problem, the first is from > xen/common/compat/memory.c where the error is > > memory.c: In function ''compat_memory_op'': > /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:35:33: > error: typedef ''__guest_handle_const_compat_memory_exchange_t'' locally > defined but not used [-Werror=unused-local-typedefs] > typedef struct { type *p; } __guest_handle_ ## name > ^ > /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:43:5: > note: in expansion of macro ''___DEFINE_XEN_GUEST_HANDLE'' > ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type) > ^ > /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:44:41: > note: in expansion of macro ''__DEFINE_XEN_GUEST_HANDLE'' > #define DEFINE_XEN_GUEST_HANDLE(name) __DEFINE_XEN_GUEST_HANDLE(name, > name) > ^ > memory.c:261:13: note: in expansion of macro ''DEFINE_XEN_GUEST_HANDLE'' > DEFINE_XEN_GUEST_HANDLE(compat_memory_exchange_t); > ^ > > so we are defining something that isn''t used.But it is used, by guest_handle_cast, albeit through several layers of macros. I would tentativly object to the use of "__attribute__((unused));" to work around what appears to be a gcc bug. If 4.8 support is desperately wanted, then a better solution would be to specify -Wno-unused-local-typedefs to disable the buggy feature. ~Andrew> > Secondly gcc 4.8 objects to lines like > > memset(ctxt, 0, sizeof(ctxt)); > > where I think you are just zeroing a pointer''s worth of memory. There are > a few cases of this in the xen code fixed in the patch, and I am also > suspicious of line 630 of > stubdom/grub-upstream/stage2/fsys_reiserfs.c which is > > memset (INFO->blocks, 0, sizeof (INFO->blocks)); > > which is noted in the build log but not flagged as an error. > > Michael Young
On Thu, 7 Feb 2013, Andrew Cooper wrote:> On 06/02/2013 20:37, M A Young wrote: > >> There are two types of problem, the first is from >> xen/common/compat/memory.c where the error is >> >> memory.c: In function ''compat_memory_op'': >> /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:35:33: >> error: typedef ''__guest_handle_const_compat_memory_exchange_t'' locally >> defined but not used [-Werror=unused-local-typedefs] >> typedef struct { type *p; } __guest_handle_ ## name >> ^ >> /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:43:5: >> note: in expansion of macro ''___DEFINE_XEN_GUEST_HANDLE'' >> ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type) >> ^ >> /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:44:41: >> note: in expansion of macro ''__DEFINE_XEN_GUEST_HANDLE'' >> #define DEFINE_XEN_GUEST_HANDLE(name) __DEFINE_XEN_GUEST_HANDLE(name, >> name) >> ^ >> memory.c:261:13: note: in expansion of macro ''DEFINE_XEN_GUEST_HANDLE'' >> DEFINE_XEN_GUEST_HANDLE(compat_memory_exchange_t); >> ^ >> >> so we are defining something that isn''t used. > > But it is used, by guest_handle_cast, albeit through several layers of > macros. > > I would tentativly object to the use of "__attribute__((unused));" to > work around what appears to be a gcc bug. If 4.8 support is desperately > wanted, then a better solution would be to specify > -Wno-unused-local-typedefs to disable the buggy feature.It looked to me as if the DEFINE macro does two typedefs and the code only uses the first, so gcc 4.8 was correct in its objections. The suggested workaround of using -Wno-unused-local-typedefs isn''t very portible as earlier versions of gcc don''t recognize it. Michael Young
On Thu, 2013-02-07 at 08:59 +0000, M A Young wrote:> On Thu, 7 Feb 2013, Andrew Cooper wrote: > > > On 06/02/2013 20:37, M A Young wrote: > > > >> There are two types of problem, the first is from > >> xen/common/compat/memory.c where the error is > >> > >> memory.c: In function ''compat_memory_op'': > >> /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:35:33: > >> error: typedef ''__guest_handle_const_compat_memory_exchange_t'' locally > >> defined but not used [-Werror=unused-local-typedefs] > >> typedef struct { type *p; } __guest_handle_ ## name > >> ^ > >> /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:43:5: > >> note: in expansion of macro ''___DEFINE_XEN_GUEST_HANDLE'' > >> ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type) > >> ^ > >> /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:44:41: > >> note: in expansion of macro ''__DEFINE_XEN_GUEST_HANDLE'' > >> #define DEFINE_XEN_GUEST_HANDLE(name) __DEFINE_XEN_GUEST_HANDLE(name, > >> name) > >> ^ > >> memory.c:261:13: note: in expansion of macro ''DEFINE_XEN_GUEST_HANDLE'' > >> DEFINE_XEN_GUEST_HANDLE(compat_memory_exchange_t); > >> ^ > >> > >> so we are defining something that isn''t used. > > > > But it is used, by guest_handle_cast, albeit through several layers of > > macros. > > > > I would tentativly object to the use of "__attribute__((unused));" to > > work around what appears to be a gcc bug. If 4.8 support is desperately > > wanted, then a better solution would be to specify > > -Wno-unused-local-typedefs to disable the buggy feature. > > It looked to me as if the DEFINE macro does two typedefs and the code only > uses the first, so gcc 4.8 was correct in its objections. The suggested > workaround of using -Wno-unused-local-typedefs isn''t very portible as > earlier versions of gcc don''t recognize it.We have a cc-option macro in the makefiles to enable these sorts of options to be enabled conditionally. However it seems a bit odd to be using DEFINE_XEN_GUEST_HANDLE in this manner, should it not be declared in some header? I can see a DEFINE_COMPAT_HANDLE(compat_memory_exchange_t) in xen/include/compat/memory.h. Perhaps the one in memory.c can just be dropped or otherwise modified to use this? Ian.
On Thu, 7 Feb 2013, M A Young wrote:> It looked to me as if the DEFINE macro does two typedefs and the code only > uses the first, so gcc 4.8 was correct in its objections. The suggested > workaround of using -Wno-unused-local-typedefs isn''t very portible as earlier > versions of gcc don''t recognize it.Actually, I could be wrong about -Wno-unused-local-typedefs as I think I am confusing it with the flag to ignore the other error which I attempted to use while tracing the occurrences for that problem. Michael Young
On Thu, 2013-02-07 at 00:55 +0000, Andrew Cooper wrote:> On 06/02/2013 20:37, M A Young wrote: > > Fedora rawhide has just moved onto gcc 4.8, and I have had to apply the > > attached patch to get it to build (I constructed the patch against 4.2.1 > > but I believe it will apply to xen-unstable). > > > > There are two types of problem, the first is from > > xen/common/compat/memory.c where the error is > > > > memory.c: In function ''compat_memory_op'': > > /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:35:33: > > error: typedef ''__guest_handle_const_compat_memory_exchange_t'' locally > > defined but not used [-Werror=unused-local-typedefs] > > typedef struct { type *p; } __guest_handle_ ## name > > ^ > > /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:43:5: > > note: in expansion of macro ''___DEFINE_XEN_GUEST_HANDLE'' > > ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type) > > ^ > > /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:44:41: > > note: in expansion of macro ''__DEFINE_XEN_GUEST_HANDLE'' > > #define DEFINE_XEN_GUEST_HANDLE(name) __DEFINE_XEN_GUEST_HANDLE(name, > > name) > > ^ > > memory.c:261:13: note: in expansion of macro ''DEFINE_XEN_GUEST_HANDLE'' > > DEFINE_XEN_GUEST_HANDLE(compat_memory_exchange_t); > > ^ > > > > so we are defining something that isn''t used. > > But it is used, by guest_handle_cast, albeit through several layers of > macros. > > I would tentativly object to the use of "__attribute__((unused));" to > work around what appears to be a gcc bug. If 4.8 support is desperately > wanted, then a better solution would be to specify > -Wno-unused-local-typedefs to disable the buggy feature. >A workaround would be to put the defined outside the function. At the end it''s just a typedef.> ~Andrew > > > > > Secondly gcc 4.8 objects to lines like > > > > memset(ctxt, 0, sizeof(ctxt)); > > > > where I think you are just zeroing a pointer''s worth of memory. There are > > a few cases of this in the xen code fixed in the patch, and I am also > > suspicious of line 630 of > > stubdom/grub-upstream/stage2/fsys_reiserfs.c which is > > > > memset (INFO->blocks, 0, sizeof (INFO->blocks)); > > > > which is noted in the build log but not flagged as an error. > > > > Michael Young >This seems real problems even to me. The md5 code is know and wrong to me for sure. Frediano
>>> On 07.02.13 at 10:06, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Thu, 2013-02-07 at 08:59 +0000, M A Young wrote: >> On Thu, 7 Feb 2013, Andrew Cooper wrote: >> >> > On 06/02/2013 20:37, M A Young wrote: >> > >> >> There are two types of problem, the first is from >> >> xen/common/compat/memory.c where the error is >> >> >> >> memory.c: In function ''compat_memory_op'': >> >> /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:35:33: >> >> error: typedef ''__guest_handle_const_compat_memory_exchange_t'' locally >> >> defined but not used [-Werror=unused-local-typedefs] >> >> typedef struct { type *p; } __guest_handle_ ## name >> >> ^ >> >> /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:43:5: >> >> note: in expansion of macro ''___DEFINE_XEN_GUEST_HANDLE'' >> >> ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type) >> >> ^ >> >> /builddir/build/BUILD/xen-4.2.1/xen/include/public/arch-x86/xen.h:44:41: >> >> note: in expansion of macro ''__DEFINE_XEN_GUEST_HANDLE'' >> >> #define DEFINE_XEN_GUEST_HANDLE(name) __DEFINE_XEN_GUEST_HANDLE(name, >> >> name) >> >> ^ >> >> memory.c:261:13: note: in expansion of macro ''DEFINE_XEN_GUEST_HANDLE'' >> >> DEFINE_XEN_GUEST_HANDLE(compat_memory_exchange_t); >> >> ^ >> >> >> >> so we are defining something that isn''t used. >> > >> > But it is used, by guest_handle_cast, albeit through several layers of >> > macros. >> > >> > I would tentativly object to the use of "__attribute__((unused));" to >> > work around what appears to be a gcc bug. If 4.8 support is desperately >> > wanted, then a better solution would be to specify >> > -Wno-unused-local-typedefs to disable the buggy feature. >> >> It looked to me as if the DEFINE macro does two typedefs and the code only >> uses the first, so gcc 4.8 was correct in its objections. The suggested >> workaround of using -Wno-unused-local-typedefs isn''t very portible as >> earlier versions of gcc don''t recognize it. > > We have a cc-option macro in the makefiles to enable these sorts of > options to be enabled conditionally. > > However it seems a bit odd to be using DEFINE_XEN_GUEST_HANDLE in this > manner, should it not be declared in some header? I can see a > DEFINE_COMPAT_HANDLE(compat_memory_exchange_t) in > xen/include/compat/memory.h. Perhaps the one in memory.c can just be > dropped or otherwise modified to use this?Actually, I''m of the opposite opinion - it should be in other places too that handles don''t get needlessly defined by the public headers. They should get defined only when there actually is a use for them. Everything else can be defined where actually consumed, as in this case: For one, a handle of a compat_* type can''t be defined in a public header anyway, as the compat types don''t get defined there. And then, as you say, the oddity of this type makes it desirable to scope restrict it as much as possible. Now, for the actual solution, I''d prefer the -Wno-... option suggested above, or as a second best choice generalizing the solution suggested by Michael, applying the attribute in DEFINE_XEN_GUEST_HANDLE() itself. This is the more that attaching it in to the use of the macro just happens to work, but isn''t guaranteed to in the future: Switching around the order of the two lines of the expansion #define __DEFINE_XEN_GUEST_HANDLE(name, type) \ ___DEFINE_XEN_GUEST_HANDLE(name, type); \ ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type) or adding a third (say, volatile) one would re-expose the problem. Jan
>>> On 06.02.13 at 21:37, M A Young <m.a.young@durham.ac.uk> wrote: >@@ -264,7 +264,7 @@ > return -1; > } > >- memset(buf,0,sizeof(buf)); >+ memset(buf,0,sizeof(*buf));While everything else looks right, this one for sure is wrong - buf is an array, and hence sizeof(*buf) would clear just the first array element. Also, we''d need you Signed-off-by to get the patch committed, and I''d suggest splitting off the public header related change from the (tools side) rest. Jan
On 07/02/2013 09:57, "Jan Beulich" <JBeulich@suse.com> wrote:> Actually, I''m of the opposite opinion - it should be in other places > too that handles don''t get needlessly defined by the public > headers. They should get defined only when there actually is a > use for them. Everything else can be defined where actually > consumed, as in this case: For one, a handle of a compat_* > type can''t be defined in a public header anyway, as the compat > types don''t get defined there. And then, as you say, the oddity > of this type makes it desirable to scope restrict it as much as > possible. > > Now, for the actual solution, I''d prefer the -Wno-... Option+1. It''s not a compiler warning that I care about. -- Keir> suggested above, or as a second best choice generalizing the > solution suggested by Michael, applying the attribute in > DEFINE_XEN_GUEST_HANDLE() itself. This is the more that > attaching it in to the use of the macro just happens to work, > but isn''t guaranteed to in the future: Switching around the > order of the two lines of the expansion > > #define __DEFINE_XEN_GUEST_HANDLE(name, type) \ > ___DEFINE_XEN_GUEST_HANDLE(name, type); \ > ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type) > > or adding a third (say, volatile) one would re-expose the > problem. > > Jan
On Thu, 7 Feb 2013, Keir Fraser wrote:> On 07/02/2013 09:57, "Jan Beulich" <JBeulich@suse.com> wrote: > >> Actually, I''m of the opposite opinion - it should be in other places >> too that handles don''t get needlessly defined by the public >> headers. They should get defined only when there actually is a >> use for them. Everything else can be defined where actually >> consumed, as in this case: For one, a handle of a compat_* >> type can''t be defined in a public header anyway, as the compat >> types don''t get defined there. And then, as you say, the oddity >> of this type makes it desirable to scope restrict it as much as >> possible. >> >> Now, for the actual solution, I''d prefer the -Wno-... Option > > +1. It''s not a compiler warning that I care about. > > -- Keir > >> suggested above, or as a second best choice generalizing the >> solution suggested by Michael, applying the attribute in >> DEFINE_XEN_GUEST_HANDLE() itself. This is the more that >> attaching it in to the use of the macro just happens to work, >> but isn''t guaranteed to in the future: Switching around the >> order of the two lines of the expansion >> >> #define __DEFINE_XEN_GUEST_HANDLE(name, type) \ >> ___DEFINE_XEN_GUEST_HANDLE(name, type); \ >> ___DEFINE_XEN_GUEST_HANDLE(const_##name, const type) >> >> or adding a third (say, volatile) one would re-expose the >> problem. >> >> JanI am attaching a patch that turns off this check. Michael Young _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Thu, 7 Feb 2013, Jan Beulich wrote:>>>> On 06.02.13 at 21:37, M A Young <m.a.young@durham.ac.uk> wrote: >> @@ -264,7 +264,7 @@ >> return -1; >> } >> >> - memset(buf,0,sizeof(buf)); >> + memset(buf,0,sizeof(*buf)); > > While everything else looks right, this one for sure is wrong - buf > is an array, and hence sizeof(*buf) would clear just the first array > element.Yes, I now agree it is wrong (I found the other example in that file and assumed this one was broken as well). I am attaching a patch that fixes the other cases. Michael Young _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 08.02.13 at 00:20, M A Young <m.a.young@durham.ac.uk> wrote: > I am attaching a patch that turns off this check.You were told how to do it and still did it wrong: You can''t blindly pass an option to the compiler that not all supported versions recognize. Once again, please do it the same way as done for other warnings (using cc-option-add). Jan
>>> On 08.02.13 at 00:25, M A Young <m.a.young@durham.ac.uk> wrote: > On Thu, 7 Feb 2013, Jan Beulich wrote: > >>>>> On 06.02.13 at 21:37, M A Young <m.a.young@durham.ac.uk> wrote: >>> @@ -264,7 +264,7 @@ >>> return -1; >>> } >>> >>> - memset(buf,0,sizeof(buf)); >>> + memset(buf,0,sizeof(*buf)); >> >> While everything else looks right, this one for sure is wrong - buf >> is an array, and hence sizeof(*buf) would clear just the first array >> element. > > Yes, I now agree it is wrong (I found the other example in that file and > assumed this one was broken as well). I am attaching a patch that fixes > the other cases.Reviewed-by: Jan Beulich <jbeulich@suse.com> Ian - please also consider applying to the 4.x trees. Jan
On Fri, 8 Feb 2013, Jan Beulich wrote:>>>> On 08.02.13 at 00:20, M A Young <m.a.young@durham.ac.uk> wrote: >> I am attaching a patch that turns off this check. > > You were told how to do it and still did it wrong: You can''t blindly > pass an option to the compiler that not all supported versions > recognize. Once again, please do it the same way as done for > other warnings (using cc-option-add).Here is a revised version that adds the -Wno-unused-local-typedefs option in a cc-option-add clause. Michael Young _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel