Message ID | 54369514.5000004@gmail.com |
---|---|
State | New |
Headers | show |
On 9 October 2014 15:00, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > The related variables are useless, need be removed, or can not pass > microblaze building, after fix it, can build microblaze, successfully. > > The related configuration: > > ./configure --target-list="arm-softmmu,microblazeel-softmmu" --enable-fdt --disable-kvm > > The related compiling error: I build this code with both these targets enabled without any problems. There is an odd compiler thing where if you have any *other* compilation issues then these warnings will also be emitted, but once you've fixed that other compiler error then these warnings are no longer produced. Maybe you ran into that? The reason I'm reluctant to make changes to these files is that they're pulled in from a different upstream project (libvixl) so we should only fix critical problems in them, or it makes new versions harder to update to. thanks -- PMM
On 10/09/2014 08:00 AM, Chen Gang wrote: That's a very long subject line. Try to keep subjects around 60 characters or so ('git shortlog -30' can give you an idea of reasonable subjects). Also, s/varialbe/variable/ in the subject. > The related variables are useless, need be removed, or can not pass > microblaze building, after fix it, can build microblaze, successfully. >
On 10/9/14 22:54, Eric Blake wrote: > On 10/09/2014 08:00 AM, Chen Gang wrote: > > That's a very long subject line. Try to keep subjects around 60 > characters or so ('git shortlog -30' can give you an idea of reasonable > subjects). OK, thanks, I shall notice about it next (also let 'git shortlog -30' as reference). > Also, s/varialbe/variable/ in the subject. > >> The related variables are useless, need be removed, or can not pass OK, thanks. If this patch can still continue (pass checking), I shall send patch v2 for it. Thanks.
On 10/9/14 22:34, Peter Maydell wrote: > On 9 October 2014 15:00, Chen Gang <gang.chen.5i5j@gmail.com> wrote: >> The related variables are useless, need be removed, or can not pass >> microblaze building, after fix it, can build microblaze, successfully. >> >> The related configuration: >> >> ./configure --target-list="arm-softmmu,microblazeel-softmmu" --enable-fdt --disable-kvm >> >> The related compiling error: > > I build this code with both these targets enabled without any > problems. > > There is an odd compiler thing where if you have any *other* > compilation issues then these warnings will also be emitted, > but once you've fixed that other compiler error then these > warnings are no longer produced. Maybe you ran into that? > I use the latest upstream gcc (which pulled from master in 2014-10-0?). In my memory (not quite sure), the older version gcc may not notice about this warning. But for me, the warning (compiler worries about) sounds reasonable, and it's harmless to be fixed (after have a look, for me, they are declared, but never be used). > The reason I'm reluctant to make changes to these files is > that they're pulled in from a different upstream project > (libvixl) so we should only fix critical problems in them, > or it makes new versions harder to update to. > Originally, I first try the Xilinx branch (Xilinx-master from Xilinx github), yesterday, and found this issue, then I try upstream main branch, found the same issue. For me, when add the related patch (which will use these variables in 'libvixl'), then declare and set them in the related headers, again. That will let other reviewers and readers easier understanding. - removing them at present, is easy understanding. - add them again when really need them, is also easy understanding. So for me, I still prefer to remove these declarations firstly. Thanks.
On 10 October 2014 02:54, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > I use the latest upstream gcc (which pulled from master in 2014-10-0?). > In my memory (not quite sure), the older version gcc may not notice > about this warning. Hmm. I'll see if I can test with that gcc. > But for me, the warning (compiler worries about) sounds reasonable, and > it's harmless to be fixed (after have a look, for me, they are declared, > but never be used). It's a library. Other users of this code upstream will use these constants; it's just that we don't happen to. >> The reason I'm reluctant to make changes to these files is >> that they're pulled in from a different upstream project >> (libvixl) so we should only fix critical problems in them, >> or it makes new versions harder to update to. >> > > Originally, I first try the Xilinx branch (Xilinx-master from Xilinx > github), yesterday, and found this issue, then I try upstream main > branch, found the same issue. > > For me, when add the related patch (which will use these variables in > 'libvixl'), then declare and set them in the related headers, again. > That will let other reviewers and readers easier understanding. > > - removing them at present, is easy understanding. > > - add them again when really need them, is also easy understanding. But it's all changes which we would have to carry locally and then re-make every time we updated to a new libvixl. I definitely don't want to do that unless it's absolutely required. thanks -- PMM
On 10/10/14 15:37, Peter Maydell wrote: > On 10 October 2014 02:54, Chen Gang <gang.chen.5i5j@gmail.com> wrote: >> I use the latest upstream gcc (which pulled from master in 2014-10-0?). >> In my memory (not quite sure), the older version gcc may not notice >> about this warning. > > Hmm. I'll see if I can test with that gcc. > It is not quite difficult to do that: get upstream source code, follow the related document to build, then use it, it should be OK (I just do like that). >> But for me, the warning (compiler worries about) sounds reasonable, and >> it's harmless to be fixed (after have a look, for me, they are declared, >> but never be used). > > It's a library. Other users of this code upstream will use these > constants; it's just that we don't happen to. > >>> The reason I'm reluctant to make changes to these files is >>> that they're pulled in from a different upstream project >>> (libvixl) so we should only fix critical problems in them, >>> or it makes new versions harder to update to. >>> >> >> Originally, I first try the Xilinx branch (Xilinx-master from Xilinx >> github), yesterday, and found this issue, then I try upstream main >> branch, found the same issue. >> >> For me, when add the related patch (which will use these variables in >> 'libvixl'), then declare and set them in the related headers, again. >> That will let other reviewers and readers easier understanding. >> >> - removing them at present, is easy understanding. >> >> - add them again when really need them, is also easy understanding. > > But it's all changes which we would have to carry locally > and then re-make every time we updated to a new libvixl. > I definitely don't want to do that unless it's absolutely > required. > It is really a little complex, we almost can not touch this header file, sorry for my original misunderstanding. And I guess, "disas/arm-64.cc" is our own file (only for qemu, not from libvixl upstream project). If really it is, we may do something in it to avoid this warning, e.g. "#pragma GCC diagnostic ignored -Wunused-variable" (almost like "include/ui/qemu-pixman.h" have done). Thanks.
On 10/10/14 16:53, Chen Gang wrote: > On 10/10/14 15:37, Peter Maydell wrote: >>>> The reason I'm reluctant to make changes to these files is >>>> that they're pulled in from a different upstream project >>>> (libvixl) so we should only fix critical problems in them, >>>> or it makes new versions harder to update to. >>>> >>> >>> Originally, I first try the Xilinx branch (Xilinx-master from Xilinx >>> github), yesterday, and found this issue, then I try upstream main >>> branch, found the same issue. >>> >>> For me, when add the related patch (which will use these variables in >>> 'libvixl'), then declare and set them in the related headers, again. >>> That will let other reviewers and readers easier understanding. >>> >>> - removing them at present, is easy understanding. >>> >>> - add them again when really need them, is also easy understanding. >> >> But it's all changes which we would have to carry locally >> and then re-make every time we updated to a new libvixl. >> I definitely don't want to do that unless it's absolutely >> required. >> > > It is really a little complex, we almost can not touch this header file, > sorry for my original misunderstanding. > > And I guess, "disas/arm-64.cc" is our own file (only for qemu, not from > libvixl upstream project). If really it is, we may do something in it to > avoid this warning, e.g. > > "#pragma GCC diagnostic ignored -Wunused-variable" (almost like "include/ui/qemu-pixman.h" have done). > Oh, may need "-Wunused-but-set-variable" instead of "-Wunused-variable", and also need check CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE (I guess the latest gcc should be OK). If what I guess is OK, I shall try to test it under both the latest gcc for x86_64 and the host gcc under Fedora 20 x86_64. If they are all OK, I shall send patch v2 for it. Thanks.
On 9 October 2014 15:00, Chen Gang <gang.chen.5i5j@gmail.com> wrote: > The related variables are useless, need be removed, or can not pass > microblaze building, after fix it, can build microblaze, successfully. > > The related configuration: > > ./configure --target-list="arm-softmmu,microblazeel-softmmu" --enable-fdt --disable-kvm > > The related compiling error: > > 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 > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > disas/libvixl/a64/instructions-a64.h | 24 ------------------------ > 1 file changed, 24 deletions(-) > > diff --git a/disas/libvixl/a64/instructions-a64.h b/disas/libvixl/a64/instructions-a64.h > index d5b90c5..1eea851 100644 > --- a/disas/libvixl/a64/instructions-a64.h > +++ b/disas/libvixl/a64/instructions-a64.h > @@ -95,30 +95,6 @@ const unsigned kDoubleExponentBits = 11; > const unsigned kFloatMantissaBits = 23; > const unsigned kFloatExponentBits = 8; > > -const float kFP32PositiveInfinity = rawbits_to_float(0x7f800000); > -const float kFP32NegativeInfinity = rawbits_to_float(0xff800000); > -const double kFP64PositiveInfinity = > - rawbits_to_double(UINT64_C(0x7ff0000000000000)); > -const double kFP64NegativeInfinity = > - rawbits_to_double(UINT64_C(0xfff0000000000000)); > - > -// This value is a signalling NaN as both a double and as a float (taking the > -// least-significant word). > -static const double kFP64SignallingNaN = > - rawbits_to_double(UINT64_C(0x7ff000007f800001)); > -static const float kFP32SignallingNaN = rawbits_to_float(0x7f800001); > - > -// A similar value, but as a quiet NaN. > -static const double kFP64QuietNaN = > - rawbits_to_double(UINT64_C(0x7ff800007fc00001)); > -static const float kFP32QuietNaN = rawbits_to_float(0x7fc00001); > - > -// The default NaN values (for FPCR.DN=1). > -static const double kFP64DefaultNaN = > - rawbits_to_double(UINT64_C(0x7ff8000000000000)); > -static const float kFP32DefaultNaN = rawbits_to_float(0x7fc00000); > - > - Upstream's plan for fixing this is to turn these into 'extern const double foo' in the header with the definition in an appropriate .cc file. Given that, I think I'm happy for us to take this patch in QEMU for the moment, with the expectation that the next libvixl drop will just obsolete it. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> -- PMM
On 10/21/2014 07:50 PM, Peter Maydell wrote: > On 9 October 2014 15:00, Chen Gang <gang.chen.5i5j@gmail.com> wrote: [] >> --- a/disas/libvixl/a64/instructions-a64.h >> +++ b/disas/libvixl/a64/instructions-a64.h >> @@ -95,30 +95,6 @@ const unsigned kDoubleExponentBits = 11; >> const unsigned kFloatMantissaBits = 23; >> const unsigned kFloatExponentBits = 8; >> >> -const float kFP32PositiveInfinity = rawbits_to_float(0x7f800000); >> -const float kFP32NegativeInfinity = rawbits_to_float(0xff800000); ... > Upstream's plan for fixing this is to turn these into > 'extern const double foo' in the header with the definition > in an appropriate .cc file. Given that, I think I'm happy for > us to take this patch in QEMU for the moment, with the expectation > that the next libvixl drop will just obsolete it. If upstream already have a patch for this, why not take the upstream's solution now, so we wont need to revert our solution before applying next drop from upstream? BTW, how the "upstream drop" is done, anyway? If it is something like git merge, we may need to care, but if it is just cp -a upstream/* . when there's no reason to... ;) Thanks, /mjt
On 23 October 2014 07:49, Michael Tokarev <mjt@tls.msk.ru> wrote: > If upstream already have a patch for this, why not take the > upstream's solution now, so we wont need to revert our solution > before applying next drop from upstream? Upstream haven't released it yet. > BTW, how the "upstream drop" is done, anyway? If it is something > like git merge, we may need to care, but if it is just cp -a upstream/* . > when there's no reason to... ;) It's a copy. As you say, since it's just a copy we don't need to worry about merge conflicts so we can just fix it in a convenient way for us for the moment. thanks -- PMM
On 10/09/2014 06:00 PM, Chen Gang wrote: > The related variables are useless, need be removed, or can not pass > microblaze building, after fix it, can build microblaze, successfully. ... Applied to trivial (with subject fixup), thanks! /mjt
On 23 October 2014 09:09, Michael Tokarev <mjt@tls.msk.ru> wrote: > On 10/09/2014 06:00 PM, Chen Gang wrote: >> The related variables are useless, need be removed, or can not pass >> microblaze building, after fix it, can build microblaze, successfully. > ... > > Applied to trivial (with subject fixup), thanks! Please don't -- as I said, I have a libvixl update in my queue, and these will conflict. I'll apply an appropriate variation of this patch to my target-arm tree. thanks -- PMM
On 23 October 2014 10:02, Peter Maydell <peter.maydell@linaro.org> wrote: > On 23 October 2014 09:09, Michael Tokarev <mjt@tls.msk.ru> wrote: >> Applied to trivial (with subject fixup), thanks! > > Please don't -- as I said, I have a libvixl update in my queue, > and these will conflict. I'll apply an appropriate variation of > this patch to my target-arm tree. ...or maybe I just *thought* I'd said that, since I can't find a mail in this thread where I did; sorry. -- PMM
On 10/23/2014 02:09 PM, Peter Maydell wrote: > On 23 October 2014 10:02, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 23 October 2014 09:09, Michael Tokarev <mjt@tls.msk.ru> wrote: >>> Applied to trivial (with subject fixup), thanks! >> >> Please don't -- as I said, I have a libvixl update in my queue, >> and these will conflict. I'll apply an appropriate variation of >> this patch to my target-arm tree. > > ...or maybe I just *thought* I'd said that, since I can't find > a mail in this thread where I did; sorry. You did, in a "v2" version. Which I overlooked really, and tried to apply the original v1 (which has you reviewed-by as a temp. workaround). Oh well.. :) There's quite a lot of noize about these things recently. Not applying it. Thanks, /mjt\
On 23 October 2014 11:27, Michael Tokarev <mjt@tls.msk.ru> wrote: > On 10/23/2014 02:09 PM, Peter Maydell wrote: >> On 23 October 2014 10:02, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 23 October 2014 09:09, Michael Tokarev <mjt@tls.msk.ru> wrote: >>>> Applied to trivial (with subject fixup), thanks! >>> >>> Please don't -- as I said, I have a libvixl update in my queue, >>> and these will conflict. I'll apply an appropriate variation of >>> this patch to my target-arm tree. >> >> ...or maybe I just *thought* I'd said that, since I can't find >> a mail in this thread where I did; sorry. > > You did, in a "v2" version. Which I overlooked really, and tried > to apply the original v1 (which has you reviewed-by as a temp. > workaround). Oh well.. :) Oh yes. I actually prefer v1, so I've applied it (with a tweaked commit message) to target-arm.next. When libvixl 1.7 appears we can just drop this change. -- PMM
diff --git a/disas/libvixl/a64/instructions-a64.h b/disas/libvixl/a64/instructions-a64.h index d5b90c5..1eea851 100644 --- a/disas/libvixl/a64/instructions-a64.h +++ b/disas/libvixl/a64/instructions-a64.h @@ -95,30 +95,6 @@ const unsigned kDoubleExponentBits = 11; const unsigned kFloatMantissaBits = 23; const unsigned kFloatExponentBits = 8; -const float kFP32PositiveInfinity = rawbits_to_float(0x7f800000); -const float kFP32NegativeInfinity = rawbits_to_float(0xff800000); -const double kFP64PositiveInfinity = - rawbits_to_double(UINT64_C(0x7ff0000000000000)); -const double kFP64NegativeInfinity = - rawbits_to_double(UINT64_C(0xfff0000000000000)); - -// This value is a signalling NaN as both a double and as a float (taking the -// least-significant word). -static const double kFP64SignallingNaN = - rawbits_to_double(UINT64_C(0x7ff000007f800001)); -static const float kFP32SignallingNaN = rawbits_to_float(0x7f800001); - -// A similar value, but as a quiet NaN. -static const double kFP64QuietNaN = - rawbits_to_double(UINT64_C(0x7ff800007fc00001)); -static const float kFP32QuietNaN = rawbits_to_float(0x7fc00001); - -// The default NaN values (for FPCR.DN=1). -static const double kFP64DefaultNaN = - rawbits_to_double(UINT64_C(0x7ff8000000000000)); -static const float kFP32DefaultNaN = rawbits_to_float(0x7fc00000); - - enum LSDataSize { LSByte = 0, LSHalfword = 1,
The related variables are useless, need be removed, or can not pass microblaze building, after fix it, can build microblaze, successfully. The related configuration: ./configure --target-list="arm-softmmu,microblazeel-softmmu" --enable-fdt --disable-kvm The related compiling error: 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 Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- disas/libvixl/a64/instructions-a64.h | 24 ------------------------ 1 file changed, 24 deletions(-)