[Qemu-trivial] [PATCH] Fix compiler warning (always return a value)

[Qemu-trivial] [PATCH] Fix compiler warning (always return a value)

[Stefan Weil]

For compilations with -DNDEBUG, the default case did not return
a value which caused a compiler warning.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/ppce500_spin.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
index cccd940..5b5ffe0 100644
--- a/hw/ppce500_spin.c
+++ b/hw/ppce500_spin.c
@@ -168,17 +168,22 @@ static uint64_t spin_read(void *opaque, target_phys_addr_t addr, unsigned len)
 {
     SpinState *s = opaque;
     uint8_t *spin_p = &((uint8_t*)s->spin)[addr];
+    uint64_t result = 0;
 
     switch (len) {
     case 1:
-        return ldub_p(spin_p);
+        result = ldub_p(spin_p);
+        break;
     case 2:
-        return lduw_p(spin_p);
+        result = lduw_p(spin_p);
+        break;
     case 4:
-        return ldl_p(spin_p);
+        result = ldl_p(spin_p);
+        break;
     default:
         assert(0);
     }
+    return result;
 }
 
 const MemoryRegionOps spin_rw_ops = {
-- 
1.7.2.5

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix compiler warning (always return a value)

[Stefan Hajnoczi]

On Mon, Oct 24, 2011 at 10:18:43PM +0200, Stefan Weil wrote:
> For compilations with -DNDEBUG, the default case did not return
> a value which caused a compiler warning.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  hw/ppce500_spin.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
> index cccd940..5b5ffe0 100644
> --- a/hw/ppce500_spin.c
> +++ b/hw/ppce500_spin.c
> @@ -168,17 +168,22 @@ static uint64_t spin_read(void *opaque, target_phys_addr_t addr, unsigned len)
>  {
>      SpinState *s = opaque;
>      uint8_t *spin_p = &((uint8_t*)s->spin)[addr];
> +    uint64_t result = 0;
>  
>      switch (len) {
>      case 1:
> -        return ldub_p(spin_p);
> +        result = ldub_p(spin_p);
> +        break;
>      case 2:
> -        return lduw_p(spin_p);
> +        result = lduw_p(spin_p);
> +        break;
>      case 4:
> -        return ldl_p(spin_p);
> +        result = ldl_p(spin_p);
> +        break;
>      default:
>          assert(0);

I would replace assert(3) with abort(3).  If this ever happens the
program is broken - returning 0 instead of an undefined value doesn't
help.

Stefan

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix compiler warning (always return a value)

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Mon, Oct 24, 2011 at 10:18:43PM +0200, Stefan Weil wrote:
>> For compilations with -DNDEBUG, the default case did not return
>> a value which caused a compiler warning.
>> 
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>  hw/ppce500_spin.c |   11 ++++++++---
>>  1 files changed, 8 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>> index cccd940..5b5ffe0 100644
>> --- a/hw/ppce500_spin.c
>> +++ b/hw/ppce500_spin.c
>> @@ -168,17 +168,22 @@ static uint64_t spin_read(void *opaque, target_phys_addr_t addr, unsigned len)
>>  {
>>      SpinState *s = opaque;
>>      uint8_t *spin_p = &((uint8_t*)s->spin)[addr];
>> +    uint64_t result = 0;
>>  
>>      switch (len) {
>>      case 1:
>> -        return ldub_p(spin_p);
>> +        result = ldub_p(spin_p);
>> +        break;
>>      case 2:
>> -        return lduw_p(spin_p);
>> +        result = lduw_p(spin_p);
>> +        break;
>>      case 4:
>> -        return ldl_p(spin_p);
>> +        result = ldl_p(spin_p);
>> +        break;
>>      default:
>>          assert(0);
>
> I would replace assert(3) with abort(3).  If this ever happens the
> program is broken - returning 0 instead of an undefined value doesn't
> help.

Why is it useful to make failed assertions stop the program regardless
of NDEBUG only when the assertion happens to be "can't reach"?

If you worry about assertions failing, don't compile with -DNDEBUG.

"Doctor, it hurts when I disable assertions!"
"Don't disable them, then."
;-P

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix compiler warning (always return a value)

[Stefan Hajnoczi]

On Fri, Oct 28, 2011 at 8:40 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Mon, Oct 24, 2011 at 10:18:43PM +0200, Stefan Weil wrote:
>>> For compilations with -DNDEBUG, the default case did not return
>>> a value which caused a compiler warning.
>>>
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>  hw/ppce500_spin.c |   11 ++++++++---
>>>  1 files changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>>> index cccd940..5b5ffe0 100644
>>> --- a/hw/ppce500_spin.c
>>> +++ b/hw/ppce500_spin.c
>>> @@ -168,17 +168,22 @@ static uint64_t spin_read(void *opaque, target_phys_addr_t addr, unsigned len)
>>>  {
>>>      SpinState *s = opaque;
>>>      uint8_t *spin_p = &((uint8_t*)s->spin)[addr];
>>> +    uint64_t result = 0;
>>>
>>>      switch (len) {
>>>      case 1:
>>> -        return ldub_p(spin_p);
>>> +        result = ldub_p(spin_p);
>>> +        break;
>>>      case 2:
>>> -        return lduw_p(spin_p);
>>> +        result = lduw_p(spin_p);
>>> +        break;
>>>      case 4:
>>> -        return ldl_p(spin_p);
>>> +        result = ldl_p(spin_p);
>>> +        break;
>>>      default:
>>>          assert(0);
>>
>> I would replace assert(3) with abort(3).  If this ever happens the
>> program is broken - returning 0 instead of an undefined value doesn't
>> help.
>
> Why is it useful to make failed assertions stop the program regardless
> of NDEBUG only when the assertion happens to be "can't reach"?

My point is that it should not be an assertion.  The program has a
control flow path which should never be taken.  In any "safe"
programming environment the program will terminate with a diagnostic.

That's exactly what we need to do here too.  assert(3) is the wrong
tool for this; we're not even asserting anything.

Stefan

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix compiler warning (always return a value)

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Fri, Oct 28, 2011 at 8:40 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>
>>> On Mon, Oct 24, 2011 at 10:18:43PM +0200, Stefan Weil wrote:
>>>> For compilations with -DNDEBUG, the default case did not return
>>>> a value which caused a compiler warning.
>>>>
>>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>>> ---
>>>>  hw/ppce500_spin.c |   11 ++++++++---
>>>>  1 files changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>>>> index cccd940..5b5ffe0 100644
>>>> --- a/hw/ppce500_spin.c
>>>> +++ b/hw/ppce500_spin.c
>>>> @@ -168,17 +168,22 @@ static uint64_t spin_read(void *opaque, target_phys_addr_t addr, unsigned len)
>>>>  {
>>>>      SpinState *s = opaque;
>>>>      uint8_t *spin_p = &((uint8_t*)s->spin)[addr];
>>>> +    uint64_t result = 0;
>>>>
>>>>      switch (len) {
>>>>      case 1:
>>>> -        return ldub_p(spin_p);
>>>> +        result = ldub_p(spin_p);
>>>> +        break;
>>>>      case 2:
>>>> -        return lduw_p(spin_p);
>>>> +        result = lduw_p(spin_p);
>>>> +        break;
>>>>      case 4:
>>>> -        return ldl_p(spin_p);
>>>> +        result = ldl_p(spin_p);
>>>> +        break;
>>>>      default:
>>>>          assert(0);
>>>
>>> I would replace assert(3) with abort(3).  If this ever happens the
>>> program is broken - returning 0 instead of an undefined value doesn't
>>> help.
>>
>> Why is it useful to make failed assertions stop the program regardless
>> of NDEBUG only when the assertion happens to be "can't reach"?
>
> My point is that it should not be an assertion.  The program has a
> control flow path which should never be taken.  In any "safe"
> programming environment the program will terminate with a diagnostic.

In the not-so-safe C programming environment, we can disable that safety
check by defining NDEBUG.

Disabling safety checks is almost always a bad idea.

> That's exactly what we need to do here too.  assert(3) is the wrong
> tool for this; we're not even asserting anything.

We do, actually: "can't reach".  That's as good an assertion as any.

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix compiler warning (always return a value)

[Stefan Hajnoczi]

On Fri, Oct 28, 2011 at 9:59 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Fri, Oct 28, 2011 at 8:40 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>
>>>> On Mon, Oct 24, 2011 at 10:18:43PM +0200, Stefan Weil wrote:
>>>>> For compilations with -DNDEBUG, the default case did not return
>>>>> a value which caused a compiler warning.
>>>>>
>>>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>>>> ---
>>>>>  hw/ppce500_spin.c |   11 ++++++++---
>>>>>  1 files changed, 8 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>>>>> index cccd940..5b5ffe0 100644
>>>>> --- a/hw/ppce500_spin.c
>>>>> +++ b/hw/ppce500_spin.c
>>>>> @@ -168,17 +168,22 @@ static uint64_t spin_read(void *opaque, target_phys_addr_t addr, unsigned len)
>>>>>  {
>>>>>      SpinState *s = opaque;
>>>>>      uint8_t *spin_p = &((uint8_t*)s->spin)[addr];
>>>>> +    uint64_t result = 0;
>>>>>
>>>>>      switch (len) {
>>>>>      case 1:
>>>>> -        return ldub_p(spin_p);
>>>>> +        result = ldub_p(spin_p);
>>>>> +        break;
>>>>>      case 2:
>>>>> -        return lduw_p(spin_p);
>>>>> +        result = lduw_p(spin_p);
>>>>> +        break;
>>>>>      case 4:
>>>>> -        return ldl_p(spin_p);
>>>>> +        result = ldl_p(spin_p);
>>>>> +        break;
>>>>>      default:
>>>>>          assert(0);
>>>>
>>>> I would replace assert(3) with abort(3).  If this ever happens the
>>>> program is broken - returning 0 instead of an undefined value doesn't
>>>> help.
>>>
>>> Why is it useful to make failed assertions stop the program regardless
>>> of NDEBUG only when the assertion happens to be "can't reach"?
>>
>> My point is that it should not be an assertion.  The program has a
>> control flow path which should never be taken.  In any "safe"
>> programming environment the program will terminate with a diagnostic.
>
> In the not-so-safe C programming environment, we can disable that safety
> check by defining NDEBUG.
>
> Disabling safety checks is almost always a bad idea.

There's a difference in a safety check that slows down the system and
a failure condition where the program must terminate.

assert(3) is for safety checks that can be disabled because they may
slow down the system.

I've been saying that assert(3) isn't appropriate here because the
program can't continue and there's no check we can skip here.

Stefan

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix compiler warning (always return a value)

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Fri, Oct 28, 2011 at 9:59 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>
>>> On Fri, Oct 28, 2011 at 8:40 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>>
>>>>> On Mon, Oct 24, 2011 at 10:18:43PM +0200, Stefan Weil wrote:
>>>>>> For compilations with -DNDEBUG, the default case did not return
>>>>>> a value which caused a compiler warning.
>>>>>>
>>>>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>>>>> ---
>>>>>>  hw/ppce500_spin.c |   11 ++++++++---
>>>>>>  1 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>>>>>> index cccd940..5b5ffe0 100644
>>>>>> --- a/hw/ppce500_spin.c
>>>>>> +++ b/hw/ppce500_spin.c
>>>>>> @@ -168,17 +168,22 @@ static uint64_t spin_read(void *opaque, target_phys_addr_t addr, unsigned len)
>>>>>>  {
>>>>>>      SpinState *s = opaque;
>>>>>>      uint8_t *spin_p = &((uint8_t*)s->spin)[addr];
>>>>>> +    uint64_t result = 0;
>>>>>>
>>>>>>      switch (len) {
>>>>>>      case 1:
>>>>>> -        return ldub_p(spin_p);
>>>>>> +        result = ldub_p(spin_p);
>>>>>> +        break;
>>>>>>      case 2:
>>>>>> -        return lduw_p(spin_p);
>>>>>> +        result = lduw_p(spin_p);
>>>>>> +        break;
>>>>>>      case 4:
>>>>>> -        return ldl_p(spin_p);
>>>>>> +        result = ldl_p(spin_p);
>>>>>> +        break;
>>>>>>      default:
>>>>>>          assert(0);
>>>>>
>>>>> I would replace assert(3) with abort(3).  If this ever happens the
>>>>> program is broken - returning 0 instead of an undefined value doesn't
>>>>> help.
>>>>
>>>> Why is it useful to make failed assertions stop the program regardless
>>>> of NDEBUG only when the assertion happens to be "can't reach"?
>>>
>>> My point is that it should not be an assertion.  The program has a
>>> control flow path which should never be taken.  In any "safe"
>>> programming environment the program will terminate with a diagnostic.
>>
>> In the not-so-safe C programming environment, we can disable that safety
>> check by defining NDEBUG.
>>
>> Disabling safety checks is almost always a bad idea.
>
> There's a difference in a safety check that slows down the system and
> a failure condition where the program must terminate.
>
> assert(3) is for safety checks that can be disabled because they may
> slow down the system.
>
> I've been saying that assert(3) isn't appropriate here because the
> program can't continue and there's no check we can skip here.

a. Program can't continue: same for pretty much any assertion anywhere.

b. There's no code we can skip here: calling abort() is code.

What I've been saying is

1. Attempting to distinguish between safety checks that are safe to skip
and failure conditions that aren't is a fool's errand.  If a check is
safe to skip it's not a safety check by definition.  It's debugging
code, perhaps.

2. Optionally disabling "expensive" safety checks should be done based
on measurements, if at all.  Arbitrarily declaring all "can't reach"
checks "inexpensive" and all other assertions "expensive" isn't
measuring, it's guesswork.

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix compiler warning (always return a value)

[Stefan Hajnoczi]

On Fri, Oct 28, 2011 at 12:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Fri, Oct 28, 2011 at 9:59 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>
>>>> On Fri, Oct 28, 2011 at 8:40 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>>>
>>>>>> On Mon, Oct 24, 2011 at 10:18:43PM +0200, Stefan Weil wrote:
>>>>>>> For compilations with -DNDEBUG, the default case did not return
>>>>>>> a value which caused a compiler warning.
>>>>>>>
>>>>>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>>>>>> ---
>>>>>>>  hw/ppce500_spin.c |   11 ++++++++---
>>>>>>>  1 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>>>>>>> index cccd940..5b5ffe0 100644
>>>>>>> --- a/hw/ppce500_spin.c
>>>>>>> +++ b/hw/ppce500_spin.c
>>>>>>> @@ -168,17 +168,22 @@ static uint64_t spin_read(void *opaque, target_phys_addr_t addr, unsigned len)
>>>>>>>  {
>>>>>>>      SpinState *s = opaque;
>>>>>>>      uint8_t *spin_p = &((uint8_t*)s->spin)[addr];
>>>>>>> +    uint64_t result = 0;
>>>>>>>
>>>>>>>      switch (len) {
>>>>>>>      case 1:
>>>>>>> -        return ldub_p(spin_p);
>>>>>>> +        result = ldub_p(spin_p);
>>>>>>> +        break;
>>>>>>>      case 2:
>>>>>>> -        return lduw_p(spin_p);
>>>>>>> +        result = lduw_p(spin_p);
>>>>>>> +        break;
>>>>>>>      case 4:
>>>>>>> -        return ldl_p(spin_p);
>>>>>>> +        result = ldl_p(spin_p);
>>>>>>> +        break;
>>>>>>>      default:
>>>>>>>          assert(0);
>>>>>>
>>>>>> I would replace assert(3) with abort(3).  If this ever happens the
>>>>>> program is broken - returning 0 instead of an undefined value doesn't
>>>>>> help.
>>>>>
>>>>> Why is it useful to make failed assertions stop the program regardless
>>>>> of NDEBUG only when the assertion happens to be "can't reach"?
>>>>
>>>> My point is that it should not be an assertion.  The program has a
>>>> control flow path which should never be taken.  In any "safe"
>>>> programming environment the program will terminate with a diagnostic.
>>>
>>> In the not-so-safe C programming environment, we can disable that safety
>>> check by defining NDEBUG.
>>>
>>> Disabling safety checks is almost always a bad idea.
>>
>> There's a difference in a safety check that slows down the system and
>> a failure condition where the program must terminate.
>>
>> assert(3) is for safety checks that can be disabled because they may
>> slow down the system.
>>
>> I've been saying that assert(3) isn't appropriate here because the
>> program can't continue and there's no check we can skip here.
>
> a. Program can't continue: same for pretty much any assertion anywhere.
>
> b. There's no code we can skip here: calling abort() is code.
>
> What I've been saying is
>
> 1. Attempting to distinguish between safety checks that are safe to skip
> and failure conditions that aren't is a fool's errand.  If a check is
> safe to skip it's not a safety check by definition.  It's debugging
> code, perhaps.
>
> 2. Optionally disabling "expensive" safety checks should be done based
> on measurements, if at all.  Arbitrarily declaring all "can't reach"
> checks "inexpensive" and all other assertions "expensive" isn't
> measuring, it's guesswork.

I'm tempted to continue the thread but at the end of the day we just
need to make the function compile with -NDEBUG.  Using abort(3) or
qemu_abort() would be the smallest and IMO sanest change, but if it's
something else that's fine.

Stefan

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix compiler warning (always return a value)

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Fri, Oct 28, 2011 at 12:07 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>
>>> On Fri, Oct 28, 2011 at 9:59 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>>
>>>>> On Fri, Oct 28, 2011 at 8:40 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
[...]
>>>>>>> I would replace assert(3) with abort(3).  If this ever happens the
>>>>>>> program is broken - returning 0 instead of an undefined value doesn't
>>>>>>> help.
>>>>>>
>>>>>> Why is it useful to make failed assertions stop the program regardless
>>>>>> of NDEBUG only when the assertion happens to be "can't reach"?
>>>>>
>>>>> My point is that it should not be an assertion.  The program has a
>>>>> control flow path which should never be taken.  In any "safe"
>>>>> programming environment the program will terminate with a diagnostic.
>>>>
>>>> In the not-so-safe C programming environment, we can disable that safety
>>>> check by defining NDEBUG.
>>>>
>>>> Disabling safety checks is almost always a bad idea.
>>>
>>> There's a difference in a safety check that slows down the system and
>>> a failure condition where the program must terminate.
>>>
>>> assert(3) is for safety checks that can be disabled because they may
>>> slow down the system.
>>>
>>> I've been saying that assert(3) isn't appropriate here because the
>>> program can't continue and there's no check we can skip here.
>>
>> a. Program can't continue: same for pretty much any assertion anywhere.
>>
>> b. There's no code we can skip here: calling abort() is code.
>>
>> What I've been saying is
>>
>> 1. Attempting to distinguish between safety checks that are safe to skip
>> and failure conditions that aren't is a fool's errand.  If a check is
>> safe to skip it's not a safety check by definition.  It's debugging
>> code, perhaps.
>>
>> 2. Optionally disabling "expensive" safety checks should be done based
>> on measurements, if at all.  Arbitrarily declaring all "can't reach"
>> checks "inexpensive" and all other assertions "expensive" isn't
>> measuring, it's guesswork.
>
> I'm tempted to continue the thread but at the end of the day we just
> need to make the function compile with -NDEBUG.  Using abort(3) or

Do we?

Anyone crazy^Wadventurous enough to compile with -DNDEBUG should be
capable of configure --disable-werror.

> qemu_abort() would be the smallest and IMO sanest change, but if it's
> something else that's fine.

Re: [Qemu-trivial] [PATCH] Fix compiler warning (always return a value)

[Paolo Bonzini]

On 10/28/2011 04:39 PM, Markus Armbruster wrote:
>> >
>> >  I'm tempted to continue the thread but at the end of the day we just
>> >  need to make the function compile with -NDEBUG.  Using abort(3) or
> Do we?
>
> Anyone crazy^Wadventurous enough to compile with -DNDEBUG should be
> capable of configure --disable-werror.

Also, I'm not really sure of the benefit of NDEBUG.  For something like 
QEMU, hardware emulation is not going to be your bottleneck and for KVM 
you badly want those asserts to protect against guest-to-host 
escalation.  If you have a really expensive check, you should wrap it 
inside #ifdef DEBUG.

And if you use NDEBUG because you want to live on the edge and ignore 
possible problems, remember there's an "assert (p != NULL)" hidden 
before any pointer dereference, and you cannot disable that with NDEBUG. :)

Paolo