| Message ID | 5439399D.4020004@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 10/11/14 22:07, Chen Gang wrote: > 'instructions-a64.h' has unused variables for qemu which can be found by > gcc 5.0.0 or higher. and qemu needs "-Werror", and cause building break. > But they may be used by another projects (not qemu). > > So for gcc 5.0.0 or higher, need still keep them, but ignore diagnostic > (still print warning, but not break building). The related warnings: > > CXX disas/arm-a64.o > In file included from /upstream/qemu/disas/libvixl/a64/disasm-a64.h:32:0, > from disas/arm-a64.cc:20: > disas/libvixl/a64/instructions-a64.h:98:13: error: 'vixl::kFP32PositiveInfinity' defined but not used [-Werror=unused-variable] > const float kFP32PositiveInfinity = rawbits_to_float(0x7f800000); > ^ > disas/libvixl/a64/instructions-a64.h:99:13: error: 'vixl::kFP32NegativeInfinity' defined but not used [-Werror=unused-variable] > const float kFP32NegativeInfinity = rawbits_to_float(0xff800000); > ^ > disas/libvixl/a64/instructions-a64.h:100:14: error: 'vixl::kFP64PositiveInfinity' defined but not used [-Werror=unused-variable] > const double kFP64PositiveInfinity = > ^ > disas/libvixl/a64/instructions-a64.h:102:14: error: 'vixl::kFP64NegativeInfinity' defined but not used [-Werror=unused-variable] > const double kFP64NegativeInfinity = > ^ > disas/libvixl/a64/instructions-a64.h:107:21: error: 'vixl::kFP64SignallingNaN' defined but not used [-Werror=unused-variable] > static const double kFP64SignallingNaN = > ^ > disas/libvixl/a64/instructions-a64.h:109:20: error: 'vixl::kFP32SignallingNaN' defined but not used [-Werror=unused-variable] > static const float kFP32SignallingNaN = rawbits_to_float(0x7f800001); > ^ > disas/libvixl/a64/instructions-a64.h:112:21: error: 'vixl::kFP64QuietNaN' defined but not used [-Werror=unused-variable] > static const double kFP64QuietNaN = > ^ > disas/libvixl/a64/instructions-a64.h:114:20: error: 'vixl::kFP32QuietNaN' defined but not used [-Werror=unused-variable] > static const float kFP32QuietNaN = rawbits_to_float(0x7fc00001); > ^ > disas/libvixl/a64/instructions-a64.h:117:21: error: 'vixl::kFP64DefaultNaN' defined but not used [-Werror=unused-variable] > static const double kFP64DefaultNaN = > ^ > disas/libvixl/a64/instructions-a64.h:119:20: error: 'vixl::kFP32DefaultNaN' defined but not used [-Werror=unused-variable] > static const float kFP32DefaultNaN = rawbits_to_float(0x7fc00000); > ^ > cc1plus: all warnings being treated as errors > make: *** [disas/arm-a64.o] Error 1 > > After this patch, can pass upstream gcc 5.0.0 building (print warning, > but not break building), and fedora 20 gcc 4.8.1 building (not find Oh, sorry, under fedora 20, it is "gcc version 4.8.3 20140624 (Red Hat 4.8.3-1) (GCC)". And the detail gcc 5.0.0 is "gcc version 5.0.0 20141003 (experimental) (GCC)" > warnings). > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > disas/libvixl/a64/instructions-a64.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/disas/libvixl/a64/instructions-a64.h b/disas/libvixl/a64/instructions-a64.h > index d5b90c5..5f707f3 100644 > --- a/disas/libvixl/a64/instructions-a64.h > +++ b/disas/libvixl/a64/instructions-a64.h > @@ -95,6 +95,12 @@ const unsigned kDoubleExponentBits = 11; > const unsigned kFloatMantissaBits = 23; > const unsigned kFloatExponentBits = 8; > > +/* For QEMU, gcc 5.0.0 or higher finds unused variables, so ignore diagnostic */ > +#if __GNUC__ >= 5 > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wunused-variable" > +#endif > + > const float kFP32PositiveInfinity = rawbits_to_float(0x7f800000); > const float kFP32NegativeInfinity = rawbits_to_float(0xff800000); > const double kFP64PositiveInfinity = > @@ -118,6 +124,9 @@ static const double kFP64DefaultNaN = > rawbits_to_double(UINT64_C(0x7ff8000000000000)); > static const float kFP32DefaultNaN = rawbits_to_float(0x7fc00000); > > +#if __GNUC__ >= 5 > +#pragma GCC diagnostic pop > +#endif > > enum LSDataSize { > LSByte = 0, >
On 11 October 2014 15:13, Chen Gang <gang.chen.5i5j@gmail.com> wrote: MJT: please don't put this in -trivial, it will clash with the update to libvixl 1.6 currently on the list. > On 10/11/14 22:07, Chen Gang wrote: >> 'instructions-a64.h' has unused variables for qemu which can be found by >> gcc 5.0.0 or higher. and qemu needs "-Werror", and cause building break. >> But they may be used by another projects (not qemu). >> >> So for gcc 5.0.0 or higher, need still keep them, but ignore diagnostic >> (still print warning, but not break building). The related warnings: >> >> CXX disas/arm-a64.o >> In file included from /upstream/qemu/disas/libvixl/a64/disasm-a64.h:32:0, >> from disas/arm-a64.cc:20: >> disas/libvixl/a64/instructions-a64.h:98:13: error: 'vixl::kFP32PositiveInfinity' defined but not used [-Werror=unused-variable] >> const float kFP32PositiveInfinity = rawbits_to_float(0x7f800000); >> ^ >> disas/libvixl/a64/instructions-a64.h:99:13: error: 'vixl::kFP32NegativeInfinity' defined but not used [-Werror=unused-variable] So, several questions here: (1) Does anybody know how the header in question should be providing constant floats and doubles to its users in a way that doesn't cause gcc to complain? If so, I can pass the answer along so at least the next version of the library is OK... (2) Bonus question, anybody know why gcc is perfectly happy with the unused integer constants in the .h file but doesn't like the float and doubles? (3) Does gcc 5 emit these warnings for all the .cc files that pull in this header, or only for the arm-a64.cc one? Some other approaches to this that would confine the fix to the makefiles rather than requiring us to modify the vixl source itself: a) add a -Wno- option for the affected .o files b) use -isystem rather than -I to include the libvixl directory on the include path (a)'s probably better I guess. thanks -- PMM
On 10/12/14 5:25, Peter Maydell wrote: > On 11 October 2014 15:13, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > > MJT: please don't put this in -trivial, it will clash with > the update to libvixl 1.6 currently on the list. > OK thanks (also remove -trivial from replying mailing list). > >> On 10/11/14 22:07, Chen Gang wrote: >>> 'instructions-a64.h' has unused variables for qemu which can be found by >>> gcc 5.0.0 or higher. and qemu needs "-Werror", and cause building break. >>> But they may be used by another projects (not qemu). >>> >>> So for gcc 5.0.0 or higher, need still keep them, but ignore diagnostic >>> (still print warning, but not break building). The related warnings: >>> >>> CXX disas/arm-a64.o >>> In file included from /upstream/qemu/disas/libvixl/a64/disasm-a64.h:32:0, >>> from disas/arm-a64.cc:20: >>> disas/libvixl/a64/instructions-a64.h:98:13: error: 'vixl::kFP32PositiveInfinity' defined but not used [-Werror=unused-variable] >>> const float kFP32PositiveInfinity = rawbits_to_float(0x7f800000); >>> ^ >>> disas/libvixl/a64/instructions-a64.h:99:13: error: 'vixl::kFP32NegativeInfinity' defined but not used [-Werror=unused-variable] > > So, several questions here: > > (1) Does anybody know how the header in question should be > providing constant floats and doubles to its users in a way > that doesn't cause gcc to complain? If so, I can pass the > answer along so at least the next version of the library > is OK... > Hmm... may try to simplify rawbits_to_float() and rawbits_to_double() to let gcc know about it is only for constant? (it seems original authors intended to do it in function, I shall try to know why) I shall try to get a conclusion within this month. > (2) Bonus question, anybody know why gcc is perfectly happy > with the unused integer constants in the .h file but doesn't > like the float and doubles? > For (2), they use a real function rawbits_to_float(), is it gcc own issue? I shall try to analyze it (at present, I am also upstream gcc members, and make a patch for it per month). I shall try to finish analyzing within this month. > (3) Does gcc 5 emit these warnings for all the .cc files > that pull in this header, or only for the arm-a64.cc one? > For (3), yes, it is, so I have to let it in header again. > Some other approaches to this that would confine the > fix to the makefiles rather than requiring us to modify > the vixl source itself: > a) add a -Wno- option for the affected .o files It is one way, but may have effect with gcc 4 version, and also it is effect with the whole file which is wider than current way. > b) use -isystem rather than -I to include the libvixl > directory on the include path > It sounds good to me, although for me, it is not related with current issue. Thanks
On 12 October 2014 01:32, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > On 10/12/14 5:25, Peter Maydell wrote: >> Some other approaches to this that would confine the >> fix to the makefiles rather than requiring us to modify >> the vixl source itself: >> a) add a -Wno- option for the affected .o files > > It is one way, but may have effect with gcc 4 version, and also it is > effect with the whole file which is wider than current way. > >> b) use -isystem rather than -I to include the libvixl >> directory on the include path >> > > It sounds good to me, although for me, it is not related with current > issue. -isystem disables a bunch of gcc warnings automatically, which is why I suggested it. I'm not overall sure it's a great idea though. -- PMM
On 10/12/14 15:50, Peter Maydell wrote: > On 12 October 2014 01:32, Chen Gang <gang.chen.5i5j@gmail.com> wrote: >> On 10/12/14 5:25, Peter Maydell wrote: >>> Some other approaches to this that would confine the >>> fix to the makefiles rather than requiring us to modify >>> the vixl source itself: >>> a) add a -Wno- option for the affected .o files >> >> It is one way, but may have effect with gcc 4 version, and also it is >> effect with the whole file which is wider than current way. >> >>> b) use -isystem rather than -I to include the libvixl >>> directory on the include path >>> >> >> It sounds good to me, although for me, it is not related with current >> issue. > > -isystem disables a bunch of gcc warnings automatically, > which is why I suggested it. I'm not overall sure it's > a great idea though. > OK, thanks. "-isystem" really can skip this warning, originally, I am not notice about it. :-) But unlucky, other files within 'libvix' which also include this header file, also report this warning. So for me, it is not a good idea to let '-isystem' for the 'libvix' own source files. Next, I shall firstly confirm whether it is a gcc 5.0 (g++) issue or not in gcc upstream mailing list. - if it is gcc 5.0 issue, I shall try to fix it within this month. - else, for me, this patch v2 can still continue. Thanks.
On 10/12/2014 01:50 AM, Peter Maydell wrote: > On 12 October 2014 01:32, Chen Gang <gang.chen.5i5j@gmail.com> wrote: >> On 10/12/14 5:25, Peter Maydell wrote: >>> Some other approaches to this that would confine the >>> fix to the makefiles rather than requiring us to modify >>> the vixl source itself: >>> a) add a -Wno- option for the affected .o files >> >> It is one way, but may have effect with gcc 4 version, and also it is >> effect with the whole file which is wider than current way. >> >>> b) use -isystem rather than -I to include the libvixl >>> directory on the include path >>> >> >> It sounds good to me, although for me, it is not related with current >> issue. > > -isystem disables a bunch of gcc warnings automatically, > which is why I suggested it. I'm not overall sure it's > a great idea though. -isystem is a heavy hammer, affecting the entire compilation. Better might be just marking the ONE header as being a system header (silence various warnings caused by just that header, while still letting the rest of the compilation warn). If the header comes from third-party sources, this is probably the best approach. It is done by adding: #if __GNUC__ >= 3 #pragma GCC system_header #endif to the header that would otherwise trigger warnings.
On 13 October 2014 16:59, Eric Blake <eblake@redhat.com> wrote: > -isystem is a heavy hammer, affecting the entire compilation. Better > might be just marking the ONE header as being a system header (silence > various warnings caused by just that header, while still letting the > rest of the compilation warn). If the header comes from third-party > sources, this is probably the best approach. It is done by adding: > > #if __GNUC__ >= 3 > #pragma GCC system_header > #endif > > to the header that would otherwise trigger warnings. If we're going to modify the header then we might as well just disable the specific warning that's being triggered. The only benefit of -isystem is you can do it in the makefile and leave the source file untouched. -- PMM
On 11.10.2014 23:25, Peter Maydell wrote: > On 11 October 2014 15:13, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > > MJT: please don't put this in -trivial, it will clash with > the update to libvixl 1.6 currently on the list. Actually I'd not put that to anywhere anyway. Because to me, *especially* in a case like this when the code is imported from some other place and will be updated later (so we should not modify it), this issue can be more easily dealt with externally, by adding a compiler option in a Makefile. Provided, ofcourse, that it is not fixed upstream properly to start with - and if it is (and this is the very sane way to go really, to fix it upstream), that fix can be pulled to qemu as well, so no clashes with further upstream changes will happen. And aso because really, this prob should be fixed properly, not worked around like this.. [] > Some other approaches to this that would confine the > fix to the makefiles rather than requiring us to modify > the vixl source itself: > a) add a -Wno- option for the affected .o files > b) use -isystem rather than -I to include the libvixl > directory on the include path > > (a)'s probably better I guess. That's what I'm after too (after trying to fix it properly). And no, at this time I dont know how gcc5 handles this. Thanks, /mjt
On 10/15/2014 03:58 AM, Michael Tokarev wrote: > On 11.10.2014 23:25, Peter Maydell wrote: >> On 11 October 2014 15:13, Chen Gang <gang.chen.5i5j@gmail.com> wrote: >> >> MJT: please don't put this in -trivial, it will clash with >> the update to libvixl 1.6 currently on the list. > > Actually I'd not put that to anywhere anyway. Because to me, > *especially* in a case like this when the code is imported from > some other place and will be updated later (so we should not > modify it), this issue can be more easily dealt with externally, > by adding a compiler option in a Makefile. Provided, ofcourse, > that it is not fixed upstream properly to start with - and if > it is (and this is the very sane way to go really, to fix it > upstream), that fix can be pulled to qemu as well, so no > clashes with further upstream changes will happen. > > And aso because really, this prob should be fixed properly, not > worked around like this.. > Within qemu, what you said sounds reasonable, but if we consider both libvixl and qemu together, for me, I'like to fix it in related source code. Maybe I need send the related patch to libvixl firstly, then Cc to qemu. If the patch can not pass libvixl's checking, but qemu still need, we have to do in the way like what you said above. By the way, originally, I misunderstood "Makefile.objs" under libvixl: I thought they belong to libvixl, but in fact, they belong to qemu. > [] >> Some other approaches to this that would confine the >> fix to the makefiles rather than requiring us to modify >> the vixl source itself: >> a) add a -Wno- option for the affected .o files >> b) use -isystem rather than -I to include the libvixl >> directory on the include path >> >> (a)'s probably better I guess. > > That's what I'm after too (after trying to fix it properly). > And no, at this time I dont know how gcc5 handles this. > At present, I have sent the related information to gcc upstream mailing list for gcc5, we are just discussing about it. - Some gcc members stick to what gcc5 has done is correct (need still report warning). - But for me, I am just trying to get another gcc members' confirmation. I am not quite familiar with C++, for me it is a complex language, so I need additional confirmation by another gcc members, at least. Thanks.
On 10/15/14 4:47, Chen Gang wrote: > On 10/15/2014 03:58 AM, Michael Tokarev wrote: >> >> That's what I'm after too (after trying to fix it properly). >> And no, at this time I dont know how gcc5 handles this. >> > > At present, I have sent the related information to gcc upstream mailing > list for gcc5, we are just discussing about it. > > - Some gcc members stick to what gcc5 has done is correct (need still > report warning). > > - But for me, I am just trying to get another gcc members' confirmation. > I am not quite familiar with C++, for me it is a complex language, so > I need additional confirmation by another gcc members, at least. > After consult the gcc related members, we are sure what gcc has done is correct, and need process this warning in our qemu or 'libvixl'. The related mail is below: -------- Forwarded Message -------- Subject: Re: [Consult] g++: About "-Wunused-variable" for constant variable in header file Date: Wed, 15 Oct 2014 10:18:44 +0100 From: Jonathan Wakely <jwakely.gcc@gmail.com> To: Chen Gang <gang.chen.5i5j@gmail.com> CC: gcc-help <gcc-help@gcc.gnu.org>, Jeff Law <law@redhat.com>, Peter Maydell <peter.maydell@linaro.org> On 14 October 2014 22:57, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > Hello All: > > At present, I met one warning issue about gcc 5.0.0. > > - For "const float a = 3.4 - 2.1 / 3;", if it is unused, gcc5 will not > report warning. Because there is no cost to initializing the variable. > - "const char n() {return 1;}; const a = n();", if 'a' is unused, gcc5 > will report warning. Because it requires dynamic initialization, running a function at startup, which has a cost. If you don't use the variable then you might not want to run the initialization code at startup, so you get a warning. > For gcc old version (e.g. gcc4), it will not report warning. Is it the > new feature for gcc5, or just a gcc5's bug? I think this behaviour is intended and is not a bug.
diff --git a/disas/libvixl/a64/instructions-a64.h b/disas/libvixl/a64/instructions-a64.h index d5b90c5..5f707f3 100644 --- a/disas/libvixl/a64/instructions-a64.h +++ b/disas/libvixl/a64/instructions-a64.h @@ -95,6 +95,12 @@ const unsigned kDoubleExponentBits = 11; const unsigned kFloatMantissaBits = 23; const unsigned kFloatExponentBits = 8; +/* For QEMU, gcc 5.0.0 or higher finds unused variables, so ignore diagnostic */ +#if __GNUC__ >= 5 +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wunused-variable" +#endif + const float kFP32PositiveInfinity = rawbits_to_float(0x7f800000); const float kFP32NegativeInfinity = rawbits_to_float(0xff800000); const double kFP64PositiveInfinity = @@ -118,6 +124,9 @@ static const double kFP64DefaultNaN = rawbits_to_double(UINT64_C(0x7ff8000000000000)); static const float kFP32DefaultNaN = rawbits_to_float(0x7fc00000); +#if __GNUC__ >= 5 +#pragma GCC diagnostic pop +#endif enum LSDataSize { LSByte = 0,
'instructions-a64.h' has unused variables for qemu which can be found by gcc 5.0.0 or higher. and qemu needs "-Werror", and cause building break. But they may be used by another projects (not qemu). So for gcc 5.0.0 or higher, need still keep them, but ignore diagnostic (still print warning, but not break building). The related warnings: CXX disas/arm-a64.o In file included from /upstream/qemu/disas/libvixl/a64/disasm-a64.h:32:0, from disas/arm-a64.cc:20: disas/libvixl/a64/instructions-a64.h:98:13: error: 'vixl::kFP32PositiveInfinity' defined but not used [-Werror=unused-variable] const float kFP32PositiveInfinity = rawbits_to_float(0x7f800000); ^ disas/libvixl/a64/instructions-a64.h:99:13: error: 'vixl::kFP32NegativeInfinity' defined but not used [-Werror=unused-variable] const float kFP32NegativeInfinity = rawbits_to_float(0xff800000); ^ disas/libvixl/a64/instructions-a64.h:100:14: error: 'vixl::kFP64PositiveInfinity' defined but not used [-Werror=unused-variable] const double kFP64PositiveInfinity = ^ disas/libvixl/a64/instructions-a64.h:102:14: error: 'vixl::kFP64NegativeInfinity' defined but not used [-Werror=unused-variable] const double kFP64NegativeInfinity = ^ disas/libvixl/a64/instructions-a64.h:107:21: error: 'vixl::kFP64SignallingNaN' defined but not used [-Werror=unused-variable] static const double kFP64SignallingNaN = ^ disas/libvixl/a64/instructions-a64.h:109:20: error: 'vixl::kFP32SignallingNaN' defined but not used [-Werror=unused-variable] static const float kFP32SignallingNaN = rawbits_to_float(0x7f800001); ^ disas/libvixl/a64/instructions-a64.h:112:21: error: 'vixl::kFP64QuietNaN' defined but not used [-Werror=unused-variable] static const double kFP64QuietNaN = ^ disas/libvixl/a64/instructions-a64.h:114:20: error: 'vixl::kFP32QuietNaN' defined but not used [-Werror=unused-variable] static const float kFP32QuietNaN = rawbits_to_float(0x7fc00001); ^ disas/libvixl/a64/instructions-a64.h:117:21: error: 'vixl::kFP64DefaultNaN' defined but not used [-Werror=unused-variable] static const double kFP64DefaultNaN = ^ disas/libvixl/a64/instructions-a64.h:119:20: error: 'vixl::kFP32DefaultNaN' defined but not used [-Werror=unused-variable] static const float kFP32DefaultNaN = rawbits_to_float(0x7fc00000); ^ cc1plus: all warnings being treated as errors make: *** [disas/arm-a64.o] Error 1 After this patch, can pass upstream gcc 5.0.0 building (print warning, but not break building), and fedora 20 gcc 4.8.1 building (not find warnings). Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- disas/libvixl/a64/instructions-a64.h | 9 +++++++++ 1 file changed, 9 insertions(+)