[Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Radim Krčmář]

Future patches should address the out-of-bounds access that is left by
using proposed solution for [2/2].

Radim Krčmář (2):
  fix GCC 5.0.0 logical-not-parentheses warnings
  milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

 hw/misc/milkymist-pfpu.c | 2 +-
 hw/net/virtio-net.c      | 4 ++--
 kvm-all.c                | 2 +-
 qemu-img.c               | 3 ++-
 4 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.3.0

[Qemu-devel] [PATCH 1/2] fix GCC 5.0.0 logical-not-parentheses warnings

[Radim Krčmář]

man gcc:
  Warn about logical not used on the left hand side operand of a
  comparison.  This option does not warn if the RHS operand is of a
  boolean type.

By preferring bool over int where sensible, but without modifying any
depending code, make GCC happy in cases like this,
  qemu-img.c: In function ‘compare_sectors’:
  qemu-img.c:992:39: error: logical not is only applied to the left hand
  side of comparison [-Werror=logical-not-parentheses]
           if (!!memcmp(buf1, buf2, 512) != res) {

hw/ide/core.c:1836 doesn't throw an error,
  assert(!!s->error == !!(s->status & ERR_STAT));
even thought the second operand is int (and first hunk of this patch has
a very similar case), maybe GCC developers still have a little faith in
C programmers.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: swapped lines in hunk 1 to to avoid parentheses [Paolo]

 hw/net/virtio-net.c | 4 ++--
 kvm-all.c           | 2 +-
 qemu-img.c          | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 45da34ad6129..93818675588e 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -120,8 +120,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
         return;
     }
 
-    if (!!n->vhost_started ==
-        (virtio_net_started(n, status) && !nc->peer->link_down)) {
+    if ((virtio_net_started(n, status) && !nc->peer->link_down) ==
+        !!n->vhost_started) {
         return;
     }
     if (!n->vhost_started) {
diff --git a/kvm-all.c b/kvm-all.c
index 05a79c20e0bb..07ef62cb3227 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -366,7 +366,7 @@ static void kvm_log_stop(MemoryListener *listener,
     }
 }
 
-static int kvm_set_migration_log(int enable)
+static int kvm_set_migration_log(bool enable)
 {
     KVMState *s = kvm_state;
     KVMSlot *mem;
diff --git a/qemu-img.c b/qemu-img.c
index e148af8a3e64..21fff2ad53d5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -977,7 +977,8 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
 static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
     int *pnum)
 {
-    int res, i;
+    bool res;
+    int i;
 
     if (n <= 0) {
         *pnum = 0;
-- 
2.3.0

Re: [Qemu-devel] [PATCH 1/2] fix GCC 5.0.0 logical-not-parentheses warnings

[Michael Tokarev]

20.02.2015 19:06, Radim Krčmář wrote:
[]
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 45da34ad6129..93818675588e 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -120,8 +120,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>          return;
>      }
>  
> -    if (!!n->vhost_started ==
> -        (virtio_net_started(n, status) && !nc->peer->link_down)) {
> +    if ((virtio_net_started(n, status) && !nc->peer->link_down) ==
> +        !!n->vhost_started) {

btw, can this be rewritten as
  (bool)n->vhost_started
instead of
  !!n->vhos_started

?

Not questioning the patch itself, just wondering, as these
double-negatives look ugly...

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/2] fix GCC 5.0.0 logical-not-parentheses warnings

[Radim Krčmář]

2015-03-04 17:36+0300, Michael Tokarev:
> 20.02.2015 19:06, Radim Krčmář wrote:
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > -    if (!!n->vhost_started ==
> > -        (virtio_net_started(n, status) && !nc->peer->link_down)) {
> > +    if ((virtio_net_started(n, status) && !nc->peer->link_down) ==
> > +        !!n->vhost_started) {
> 
> btw, can this be rewritten as
>   (bool)n->vhost_started
> instead of
>   !!n->vhos_started

Yes.  (It's the same as long as we use bool from stdbool.h.)

> Not questioning the patch itself, just wondering, as these
> double-negatives look ugly...

Casting to bool looks better to me as well, yet the QEMU design guide,
`grep $x | wc -l`, greatly prefers bangs.

I think that it is best to define it as bool in struct VirtIONet,
but I prefer not to change decision that I don't understand ...

Thanks for accepting the patches.

[Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Radim Krčmář]

man gcc:
  Warn if in a loop with constant number of iterations the compiler
  detects undefined behavior in some statement during one or more of
  the iterations.

Milkymist pfpu has no jump instructions, so checking for MICROCODE_WORDS
instructions should have kept us in bounds of s->microcode, but i++
allowed one loop too many,

  hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
  hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be reached after undefined behavior [-Werror=aggressive-loop-optimizations]
                   if (i++ >= MICROCODE_WORDS) {
                      ^
  hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement is here
       uint32_t insn = s->microcode[pc];
                ^

The code can still access out of bounds, because it presumes that PC register
always begins at 0, and we allow writing to it.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: used a simpler solution [Paolo, Peter]

 hw/misc/milkymist-pfpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
index 609f33f9cd14..08b604f13f4b 100644
--- a/hw/misc/milkymist-pfpu.c
+++ b/hw/misc/milkymist-pfpu.c
@@ -362,7 +362,7 @@ static void pfpu_start(MilkymistPFPUState *s)
             i = 0;
             while (pfpu_decode_insn(s)) {
                 /* decode at most MICROCODE_WORDS instructions */
-                if (i++ >= MICROCODE_WORDS) {
+                if (++i >= MICROCODE_WORDS) {
                     error_report("milkymist_pfpu: too many instructions "
                             "executed in microcode. No VECTOUT?");
                     break;
-- 
2.3.0

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Michael Walle]

Am 2015-02-20 17:06, schrieb Radim Krčmář:
> man gcc:
>   Warn if in a loop with constant number of iterations the compiler
>   detects undefined behavior in some statement during one or more of
>   the iterations.
> 
> Milkymist pfpu has no jump instructions, so checking for 
> MICROCODE_WORDS
> instructions should have kept us in bounds of s->microcode, but i++
> allowed one loop too many,
> 
>   hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
>   hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be
> reached after undefined behavior
> [-Werror=aggressive-loop-optimizations]
>                    if (i++ >= MICROCODE_WORDS) {
>                       ^
>   hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement 
> is here
>        uint32_t insn = s->microcode[pc];
>                 ^
> 
> The code can still access out of bounds, because it presumes that PC 
> register
> always begins at 0, and we allow writing to it.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: used a simpler solution [Paolo, Peter]
> 
>  hw/misc/milkymist-pfpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
> index 609f33f9cd14..08b604f13f4b 100644
> --- a/hw/misc/milkymist-pfpu.c
> +++ b/hw/misc/milkymist-pfpu.c
> @@ -362,7 +362,7 @@ static void pfpu_start(MilkymistPFPUState *s)
>              i = 0;
>              while (pfpu_decode_insn(s)) {
>                  /* decode at most MICROCODE_WORDS instructions */
> -                if (i++ >= MICROCODE_WORDS) {
> +                if (++i >= MICROCODE_WORDS) {
>                      error_report("milkymist_pfpu: too many 
> instructions "
>                              "executed in microcode. No VECTOUT?");
>                      break;

Acked-by: Michael Walle <michael@walle.cc>

Re: [Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Radim Krčmář]

Cc: qemu-trivial@nongnu.org

Re: [Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Michael Tokarev]

20.02.2015 19:06, Radim Krčmář wrote:
> Future patches should address the out-of-bounds access that is left by
> using proposed solution for [2/2].
> 
> Radim Krčmář (2):
>   fix GCC 5.0.0 logical-not-parentheses warnings
>   milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning
> 
>  hw/misc/milkymist-pfpu.c | 2 +-
>  hw/net/virtio-net.c      | 4 ++--
>  kvm-all.c                | 2 +-
>  qemu-img.c               | 3 ++-
>  4 files changed, 6 insertions(+), 5 deletions(-)

Applied both to -trivial finally.  Thank you!

/mjt

[Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Radim Krčmář]

GCC is more strict now.

Radim Krčmář (2):
  fix GCC 5.0.0 logical-not-parentheses warnings
  milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

 hw/misc/milkymist-pfpu.c | 18 ++++++++----------
 hw/net/virtio-net.c      |  2 +-
 kvm-all.c                |  2 +-
 qemu-img.c               |  3 ++-
 4 files changed, 12 insertions(+), 13 deletions(-)

-- 
2.3.0

[Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Radim Krčmář]

man gcc:
  Warn if in a loop with constant number of iterations the compiler
  detects undefined behavior in some statement during one or more of
  the iterations.

Refactored the code a bit to avoid the GCC warning, in an objectionable
way,
  hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
  hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be reached after undefined behavior [-Werror=aggressive-loop-optimizations]
                   if (i++ >= MICROCODE_WORDS) {
                      ^
  hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement is here
       uint32_t insn = s->microcode[pc];
                ^

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/misc/milkymist-pfpu.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
index 609f33f9cd14..133f5b8c5153 100644
--- a/hw/misc/milkymist-pfpu.c
+++ b/hw/misc/milkymist-pfpu.c
@@ -164,6 +164,13 @@ output_queue_advance(MilkymistPFPUState *s)
 static int pfpu_decode_insn(MilkymistPFPUState *s)
 {
     uint32_t pc = s->regs[R_PC];
+
+    if (pc > MICROCODE_WORDS) {
+        error_report("milkymist_pfpu: too many instructions "
+                     "executed in microcode. No VECTOUT?");
+        return 0;
+    }
+
     uint32_t insn = s->microcode[pc];
     uint32_t reg_a = (insn >> 18) & 0x7f;
     uint32_t reg_b = (insn >> 11) & 0x7f;
@@ -348,7 +355,6 @@ static int pfpu_decode_insn(MilkymistPFPUState *s)
 static void pfpu_start(MilkymistPFPUState *s)
 {
     int x, y;
-    int i;
 
     for (y = 0; y <= s->regs[R_VMESHLAST]; y++) {
         for (x = 0; x <= s->regs[R_HMESHLAST]; x++) {
@@ -359,15 +365,7 @@ static void pfpu_start(MilkymistPFPUState *s)
             s->gp_regs[GPR_Y] = y;
 
             /* run microcode on this position */
-            i = 0;
-            while (pfpu_decode_insn(s)) {
-                /* decode at most MICROCODE_WORDS instructions */
-                if (i++ >= MICROCODE_WORDS) {
-                    error_report("milkymist_pfpu: too many instructions "
-                            "executed in microcode. No VECTOUT?");
-                    break;
-                }
-            }
+            while (pfpu_decode_insn(s));
 
             /* reset pc for next run */
             s->regs[R_PC] = 0;
-- 
2.3.0

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Radim Krčmář]

2015-02-20 15:18+0100, Radim Krčmář:
> diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
> @@ -164,6 +164,13 @@ output_queue_advance(MilkymistPFPUState *s)
>  static int pfpu_decode_insn(MilkymistPFPUState *s)
>  {
>      uint32_t pc = s->regs[R_PC];
> +
> +    if (pc > MICROCODE_WORDS) {

Ugh, should have been '>=' here.

> +        error_report("milkymist_pfpu: too many instructions "
> +                     "executed in microcode. No VECTOUT?");
> +        return 0;
> +    }
> +
>      uint32_t insn = s->microcode[pc];

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Paolo Bonzini]

On 20/02/2015 15:18, Radim Krčmář wrote:
> man gcc:
>   Warn if in a loop with constant number of iterations the compiler
>   detects undefined behavior in some statement during one or more of
>   the iterations.
> 
> Refactored the code a bit to avoid the GCC warning, in an objectionable
> way,
>   hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
>   hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be reached after undefined behavior [-Werror=aggressive-loop-optimizations]
>                    if (i++ >= MICROCODE_WORDS) {
>                       ^
>   hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement is here
>        uint32_t insn = s->microcode[pc];
>                 ^
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  hw/misc/milkymist-pfpu.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
> index 609f33f9cd14..133f5b8c5153 100644
> --- a/hw/misc/milkymist-pfpu.c
> +++ b/hw/misc/milkymist-pfpu.c
> @@ -164,6 +164,13 @@ output_queue_advance(MilkymistPFPUState *s)
>  static int pfpu_decode_insn(MilkymistPFPUState *s)
>  {
>      uint32_t pc = s->regs[R_PC];
> +
> +    if (pc > MICROCODE_WORDS) {
> +        error_report("milkymist_pfpu: too many instructions "
> +                     "executed in microcode. No VECTOUT?");
> +        return 0;
> +    }
> +
>      uint32_t insn = s->microcode[pc];
>      uint32_t reg_a = (insn >> 18) & 0x7f;
>      uint32_t reg_b = (insn >> 11) & 0x7f;
> @@ -348,7 +355,6 @@ static int pfpu_decode_insn(MilkymistPFPUState *s)
>  static void pfpu_start(MilkymistPFPUState *s)
>  {
>      int x, y;
> -    int i;
>  
>      for (y = 0; y <= s->regs[R_VMESHLAST]; y++) {
>          for (x = 0; x <= s->regs[R_HMESHLAST]; x++) {
> @@ -359,15 +365,7 @@ static void pfpu_start(MilkymistPFPUState *s)
>              s->gp_regs[GPR_Y] = y;
>  
>              /* run microcode on this position */
> -            i = 0;
> -            while (pfpu_decode_insn(s)) {
> -                /* decode at most MICROCODE_WORDS instructions */
> -                if (i++ >= MICROCODE_WORDS) {

Isn't the fix just to say "++i" instead of "i++"?

Paolo

> -                    error_report("milkymist_pfpu: too many instructions "
> -                            "executed in microcode. No VECTOUT?");
> -                    break;
> -                }
> -            }
> +            while (pfpu_decode_insn(s));
>  
>              /* reset pc for next run */
>              s->regs[R_PC] = 0;
> 

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Michael Walle]

Am 2015-02-20 15:36, schrieb Paolo Bonzini:
> On 20/02/2015 15:18, Radim Krčmář wrote:
>> man gcc:
>>   Warn if in a loop with constant number of iterations the compiler
>>   detects undefined behavior in some statement during one or more of
>>   the iterations.
>> 
>> Refactored the code a bit to avoid the GCC warning, in an 
>> objectionable
>> way,
>>   hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
>>   hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be 
>> reached after undefined behavior 
>> [-Werror=aggressive-loop-optimizations]
>>                    if (i++ >= MICROCODE_WORDS) {
>>                       ^
>>   hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement 
>> is here
>>        uint32_t insn = s->microcode[pc];
>>                 ^
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  hw/misc/milkymist-pfpu.c | 18 ++++++++----------
>>  1 file changed, 8 insertions(+), 10 deletions(-)
>> 
>> diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
>> index 609f33f9cd14..133f5b8c5153 100644
>> --- a/hw/misc/milkymist-pfpu.c
>> +++ b/hw/misc/milkymist-pfpu.c
>> @@ -164,6 +164,13 @@ output_queue_advance(MilkymistPFPUState *s)
>>  static int pfpu_decode_insn(MilkymistPFPUState *s)
>>  {
>>      uint32_t pc = s->regs[R_PC];
>> +
>> +    if (pc > MICROCODE_WORDS) {
>> +        error_report("milkymist_pfpu: too many instructions "
>> +                     "executed in microcode. No VECTOUT?");
>> +        return 0;
>> +    }
>> +

I don't like this syntax, eg declaration, statements, declaration. Can 
you just declare the variable first and then assign them? Also the error 
message is then misleading. I'd prefer something like "milkymist_pfpu: 
program counter out of bounds. No VECTOUT?"

>>      uint32_t insn = s->microcode[pc];
>>      uint32_t reg_a = (insn >> 18) & 0x7f;
>>      uint32_t reg_b = (insn >> 11) & 0x7f;
>> @@ -348,7 +355,6 @@ static int pfpu_decode_insn(MilkymistPFPUState *s)
>>  static void pfpu_start(MilkymistPFPUState *s)
>>  {
>>      int x, y;
>> -    int i;
>> 
>>      for (y = 0; y <= s->regs[R_VMESHLAST]; y++) {
>>          for (x = 0; x <= s->regs[R_HMESHLAST]; x++) {
>> @@ -359,15 +365,7 @@ static void pfpu_start(MilkymistPFPUState *s)
>>              s->gp_regs[GPR_Y] = y;
>> 
>>              /* run microcode on this position */
>> -            i = 0;
>> -            while (pfpu_decode_insn(s)) {
>> -                /* decode at most MICROCODE_WORDS instructions */
>> -                if (i++ >= MICROCODE_WORDS) {
> 
> Isn't the fix just to say "++i" instead of "i++"?

In the first run, s->regs[R_PC] may have any value, therefore the "insn 
= s->microcode[pc]" from above may access out of bounds.

-michael

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Paolo Bonzini]

On 20/02/2015 15:52, Michael Walle wrote:
>>>
>>> -            i = 0;
>>> -            while (pfpu_decode_insn(s)) {
>>> -                /* decode at most MICROCODE_WORDS instructions */
>>> -                if (i++ >= MICROCODE_WORDS) {
>>
>> Isn't the fix just to say "++i" instead of "i++"?
> 
> In the first run, s->regs[R_PC] may have any value, therefore the "insn
> = s->microcode[pc]" from above may access out of bounds.

Then should pfpu_decode_insn access s->microcode[pc & (MICROCODE_WORDS -
1)]?  That's likely what happens in hardware, and the purpose of the
error is just to avoid an infinite loop in device code.

Paolo

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Radim Krčmář]

2015-02-20 15:55+0100, Paolo Bonzini:
> 
> 
> On 20/02/2015 15:52, Michael Walle wrote:
> >>>
> >>> -            i = 0;
> >>> -            while (pfpu_decode_insn(s)) {
> >>> -                /* decode at most MICROCODE_WORDS instructions */
> >>> -                if (i++ >= MICROCODE_WORDS) {
> >>
> >> Isn't the fix just to say "++i" instead of "i++"?
> > 
> > In the first run, s->regs[R_PC] may have any value, therefore the "insn
> > = s->microcode[pc]" from above may access out of bounds.
> 
> Then should pfpu_decode_insn access s->microcode[pc & (MICROCODE_WORDS -
> 1)]?  That's likely what happens in hardware, and the purpose of the
> error is just to avoid an infinite loop in device code.

http://www.milkymist.org/socdoc/pfpu.pdf is dead, but the source isn't:
https://github.com/m-labs/milkymist/blob/master/cores/pfpu/doc/pfpu.tex

I don't see the PC register mentioned in interface, so hiding it would
probably be a good start.

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Peter Maydell]

On 20 February 2015 at 23:18, Radim Krčmář <rkrcmar@redhat.com> wrote:
> man gcc:
>   Warn if in a loop with constant number of iterations the compiler
>   detects undefined behavior in some statement during one or more of
>   the iterations.
>
> Refactored the code a bit to avoid the GCC warning, in an objectionable
> way,
>   hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
>   hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be reached after undefined behavior [-Werror=aggressive-loop-optimizations]
>                    if (i++ >= MICROCODE_WORDS) {
>                       ^
>   hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement is here
>        uint32_t insn = s->microcode[pc];
>                 ^

Why can't we just fix this by fixing the loop boundary
condition, which is the actual bug here? There should
be no need to move the check into the function (where it
does not belong).

(We try to stop before overflowing the s->microcode[]
array, but because 'i++' is a post increment and we do a >=
comparison, the last loop round will try to write to
s->microcode[MICROCODE_WORDS]. Easiest fix is to use
"++i" I suppose, though it might be better to just
separate the increment and the conditional instead.)

There is probably some actual hardware behaviour we're
failing to model correctly here, since it's a safe bet
the h/w doesn't print an error message in this situation.
However we should just fix the bogus array handling for now.

-- PMM

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Radim Krčmář]

2015-02-20 23:40+0900, Peter Maydell:
> On 20 February 2015 at 23:18, Radim Krčmář <rkrcmar@redhat.com> wrote:
> > man gcc:
> >   Warn if in a loop with constant number of iterations the compiler
> >   detects undefined behavior in some statement during one or more of
> >   the iterations.
> >
> > Refactored the code a bit to avoid the GCC warning, in an objectionable
> > way,
> >   hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
> >   hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be reached after undefined behavior [-Werror=aggressive-loop-optimizations]
> >                    if (i++ >= MICROCODE_WORDS) {
> >                       ^
> >   hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement is here
> >        uint32_t insn = s->microcode[pc];
> >                 ^
> 
> Why can't we just fix this by fixing the loop boundary
> condition, which is the actual bug here? There should
> be no need to move the check into the function (where it
> does not belong).

It would work now, but GCC could get more intelligent with time and
realize that there still can be undefined behavior ...

> (We try to stop before overflowing the s->microcode[]
> array, but because 'i++' is a post increment and we do a >=
> comparison, the last loop round will try to write to
> s->microcode[MICROCODE_WORDS]. Easiest fix is to use
> "++i" I suppose, though it might be better to just
> separate the increment and the conditional instead.)
> 
> There is probably some actual hardware behaviour we're
> failing to model correctly here, since it's a safe bet
> the h/w doesn't print an error message in this situation.
> However we should just fix the bogus array handling for now.

Ok, I will do just ++i for v2 and keep the other bug for later.

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Michael Walle]

Am 2015-02-20 16:37, schrieb Radim Krčmář:
> 2015-02-20 23:40+0900, Peter Maydell:
>> On 20 February 2015 at 23:18, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> > man gcc:
>> >   Warn if in a loop with constant number of iterations the compiler
>> >   detects undefined behavior in some statement during one or more of
>> >   the iterations.
>> >
>> > Refactored the code a bit to avoid the GCC warning, in an objectionable
>> > way,
>> >   hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
>> >   hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be reached after undefined behavior [-Werror=aggressive-loop-optimizations]
>> >                    if (i++ >= MICROCODE_WORDS) {
>> >                       ^
>> >   hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement is here
>> >        uint32_t insn = s->microcode[pc];
>> >                 ^
>> 
>> Why can't we just fix this by fixing the loop boundary
>> condition, which is the actual bug here? There should
>> be no need to move the check into the function (where it
>> does not belong).
> 
> It would work now, but GCC could get more intelligent with time and
> realize that there still can be undefined behavior ...
> 
>> (We try to stop before overflowing the s->microcode[]
>> array, but because 'i++' is a post increment and we do a >=
>> comparison, the last loop round will try to write to
>> s->microcode[MICROCODE_WORDS]. Easiest fix is to use
>> "++i" I suppose, though it might be better to just
>> separate the increment and the conditional instead.)
>> 
>> There is probably some actual hardware behaviour we're
>> failing to model correctly here, since it's a safe bet
>> the h/w doesn't print an error message in this situation.
>> However we should just fix the bogus array handling for now.
> 
> Ok, I will do just ++i for v2 and keep the other bug for later.

Ok, i'll post a patch later.

-michael