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

[Qemu-devel] [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-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-devel] [PATCH] Fix compiler warning (always return a value), introduce qemu_abort?

[Stefan Weil]

Am 26.10.2011 14:54, schrieb 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

Alex, do you agree on replacing assert() by abort()?

I personally don't like abort() because it does not show the
reason for the failure.

Most users don't know how to get a core dump or how to
use gdb. And even for those who know, a crash caused
by an abort() which cannot be reproduced usually happens
on a system were ulimit disables core dumps...

I'd like to have a qemu_abort() macro in qemu-common.h which
replaces all abort() calls used today:

#define qemu_abort() \
   do { \
     fprintf(stderr, "QEMU aborted in %s, %s:%u\n", __func__, __FILE__, 
__LINE__);
     abort();
   } while (0)

(The macro could also call a function which handles fprintf and abort).

Cheers,
Stefan W.

Re: [Qemu-devel] [PATCH] Fix compiler warning (always return a value), introduce qemu_abort?

[Blue Swirl]

On Wed, Oct 26, 2011 at 16:35, Stefan Weil <sw@weilnetz.de> wrote:
> Am 26.10.2011 14:54, schrieb 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
>
> Alex, do you agree on replacing assert() by abort()?
>
> I personally don't like abort() because it does not show the
> reason for the failure.
>
> Most users don't know how to get a core dump or how to
> use gdb. And even for those who know, a crash caused
> by an abort() which cannot be reproduced usually happens
> on a system were ulimit disables core dumps...
>
> I'd like to have a qemu_abort() macro in qemu-common.h which
> replaces all abort() calls used today:

Also assert(0) calls.

> #define qemu_abort() \
>  do { \
>    fprintf(stderr, "QEMU aborted in %s, %s:%u\n", __func__, __FILE__,
> __LINE__);
>    abort();
>  } while (0)
>
> (The macro could also call a function which handles fprintf and abort).

There could be also a version with additional error message parameter.

[Qemu-devel] [RFC] Introduce qemu_abort? (was: Fix compiler warning (always return a value))

[Stefan Weil]

Am 26.10.2011 19:49, schrieb Blue Swirl:
> On Wed, Oct 26, 2011 at 16:35, Stefan Weil <sw@weilnetz.de> wrote:
>> ...
>> I personally don't like abort() because it does not show the
>> reason for the failure.
>>
>> Most users don't know how to get a core dump or how to
>> use gdb. And even for those who know, a crash caused
>> by an abort() which cannot be reproduced usually happens
>> on a system were ulimit disables core dumps...
>>
>> I'd like to have a qemu_abort() macro in qemu-common.h which
>> replaces all abort() calls used today:
>
> Also assert(0) calls.
>
>> #define qemu_abort() \
>>  do { \
>>    fprintf(stderr, "QEMU aborted in %s, %s:%u\n", __func__, __FILE__,
>> __LINE__);
>>    abort();
>>  } while (0)
>>
>> (The macro could also call a function which handles fprintf and abort).
>
> There could be also a version with additional error message parameter.

Replacing abort() and assert(0) by qemu_abort() touches a lot of files.
Do you think this can be a change for QEMU 1.0, or is it better
to wait?

Adding the infrastructure (macros / implementation) could be done
faster. I suggest these interfaces in qemu-common.h:

qemu_abort() - abort QEMU with a message containing function name,
file name and line (macro, see message text in my previous mail cited above)

qemu_fatal(formatstring, ...) - abort QEMU with a printf like message
(function, prints "QEMU aborted, " and the text according to the parameters)

Regards,
Stefan W.

Re: [Qemu-devel] [RFC] Introduce qemu_abort? (was: Fix compiler warning (always return a value))

[Blue Swirl]

On Wed, Oct 26, 2011 at 18:34, Stefan Weil <sw@weilnetz.de> wrote:
> Am 26.10.2011 19:49, schrieb Blue Swirl:
>>
>> On Wed, Oct 26, 2011 at 16:35, Stefan Weil <sw@weilnetz.de> wrote:
>>>
>>> ...
>>> I personally don't like abort() because it does not show the
>>> reason for the failure.
>>>
>>> Most users don't know how to get a core dump or how to
>>> use gdb. And even for those who know, a crash caused
>>> by an abort() which cannot be reproduced usually happens
>>> on a system were ulimit disables core dumps...
>>>
>>> I'd like to have a qemu_abort() macro in qemu-common.h which
>>> replaces all abort() calls used today:
>>
>> Also assert(0) calls.
>>
>>> #define qemu_abort() \
>>>  do { \
>>>   fprintf(stderr, "QEMU aborted in %s, %s:%u\n", __func__, __FILE__,
>>> __LINE__);
>>>   abort();
>>>  } while (0)
>>>
>>> (The macro could also call a function which handles fprintf and abort).
>>
>> There could be also a version with additional error message parameter.
>
> Replacing abort() and assert(0) by qemu_abort() touches a lot of files.
> Do you think this can be a change for QEMU 1.0, or is it better
> to wait?

It shouldn't destabilize anything since we are going to abort anyway.

> Adding the infrastructure (macros / implementation) could be done
> faster. I suggest these interfaces in qemu-common.h:
>
> qemu_abort() - abort QEMU with a message containing function name,
> file name and line (macro, see message text in my previous mail cited above)
>
> qemu_fatal(formatstring, ...) - abort QEMU with a printf like message
> (function, prints "QEMU aborted, " and the text according to the parameters)

This function could also be added, though I'd leave actual conversions
to 1.1, in case printfs turn out dangerous.

Re: [Qemu-devel] [RFC] Introduce qemu_abort? (was: Fix compiler warning (always return a value))

[Peter Maydell]

On 26 October 2011 19:34, Stefan Weil <sw@weilnetz.de> wrote:
> Adding the infrastructure (macros / implementation) could be done
> faster. I suggest these interfaces in qemu-common.h:
>
> qemu_abort() - abort QEMU with a message containing function name,
> file name and line (macro, see message text in my previous mail cited above)
>
> qemu_fatal(formatstring, ...) - abort QEMU with a printf like message
> (function, prints "QEMU aborted, " and the text according to the parameters)

We already have a couple of "print a message and abort" functions:
  hw_error()  (prints "qemu: hardware error" message, dumps cpu state
               for all cores and abort()s)
  cpu_abort() (prints "qemu: fatal:" message, dumps cpu state for one
               core and abort()s)
  tcg_abort() (takes no arguments, acts like your proposed qemu_abort())

...perhaps we should try to straighten out the naming inconsistencies
here and document which ones to use when.

I think the qemu_fatal() might be better addressed as part of a set
of functions for handling fatal and other errors in a more consistent
way. At the moment it's a bit random whether a device model will
abort, warn but continue or silently continue if a guest does something
that tickles unimplemented behaviour or does something that's probably
a guest error (eg accessing nonexistent registers). It would be good
to have some of this be user configurable (some people want to see
"my guest OS is doing something that's probably a guest OS bug" warnings,
some don't, for instance).

Regarding whether to put this into qemu 1.0 or not -- my preference
would be for 'not' because any change touching a lot of files has
the potential to cause pending patches/pullreqs people are trying
to get in before the feature freeze deadline to no longer apply.

-- PMM

Re: [Qemu-devel] [PATCH] Fix compiler warning (always return a value), introduce qemu_abort?

[Stefan Hajnoczi]

On Wed, Oct 26, 2011 at 06:35:08PM +0200, Stefan Weil wrote:
> Am 26.10.2011 14:54, schrieb 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
> 
> Alex, do you agree on replacing assert() by abort()?
> 
> I personally don't like abort() because it does not show the
> reason for the failure.
> 
> Most users don't know how to get a core dump or how to
> use gdb. And even for those who know, a crash caused
> by an abort() which cannot be reproduced usually happens
> on a system were ulimit disables core dumps...
> 
> I'd like to have a qemu_abort() macro in qemu-common.h which
> replaces all abort() calls used today:

Sounds good.

Stefan

Re: [Qemu-devel] [PATCH] Fix compiler warning (always return a value), introduce qemu_abort?

[Alexander Graf]

On 26.10.2011, at 18:35, Stefan Weil <sw@weilnetz.de> wrote:

> Am 26.10.2011 14:54, schrieb 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
> 
> Alex, do you agree on replacing assert() by abort()?

I honestly am pretty indifferent. IIRC I used assert(0) because it does show you the line of code it failed in.

Alex

> 
> I personally don't like abort() because it does not show the
> reason for the failure.
> 
> Most users don't know how to get a core dump or how to
> use gdb. And even for those who know, a crash caused
> by an abort() which cannot be reproduced usually happens
> on a system were ulimit disables core dumps...
> 
> I'd like to have a qemu_abort() macro in qemu-common.h which
> replaces all abort() calls used today:
> 
> #define qemu_abort() \
>  do { \
>    fprintf(stderr, "QEMU aborted in %s, %s:%u\n", __func__, __FILE__, __LINE__);
>    abort();
>  } while (0)
> 
> (The macro could also call a function which handles fprintf and abort).
> 
> Cheers,
> Stefan W.
> 

Re: [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-devel] [RFC] Introduce qemu_abort?

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 26 October 2011 19:34, Stefan Weil <sw@weilnetz.de> wrote:
>> Adding the infrastructure (macros / implementation) could be done
>> faster. I suggest these interfaces in qemu-common.h:
>>
>> qemu_abort() - abort QEMU with a message containing function name,
>> file name and line (macro, see message text in my previous mail cited above)
>>
>> qemu_fatal(formatstring, ...) - abort QEMU with a printf like message
>> (function, prints "QEMU aborted, " and the text according to the parameters)
>
> We already have a couple of "print a message and abort" functions:
>   hw_error()  (prints "qemu: hardware error" message, dumps cpu state
>                for all cores and abort()s)
>   cpu_abort() (prints "qemu: fatal:" message, dumps cpu state for one
>                core and abort()s)
>   tcg_abort() (takes no arguments, acts like your proposed qemu_abort())
>
> ...perhaps we should try to straighten out the naming inconsistencies
> here and document which ones to use when.

Yes, that would be useful.

> I think the qemu_fatal() might be better addressed as part of a set
> of functions for handling fatal and other errors in a more consistent
> way. At the moment it's a bit random whether a device model will
> abort, warn but continue or silently continue if a guest does something
> that tickles unimplemented behaviour or does something that's probably
> a guest error (eg accessing nonexistent registers). It would be good
> to have some of this be user configurable (some people want to see
> "my guest OS is doing something that's probably a guest OS bug" warnings,
> some don't, for instance).

Yes.

assert() and abort() are strictly for programming errors: the programmer
screwed up, program is in disorder, and can't continue.  They're not
appropriate ways to handle environmental conditions that can be expected
(out of memory, network kaputt, ...), or bad input.  For QEMU, input
includes guest actions.

I generally don't bother to provide "nice" ways to abort on programming
error.  assert() is standard, and it's just fine.  To do anything about
it, I'll need a debugger anyway.  If you don't like abort() not telling
you function and line, use assert().

That said, if you want to wrap around yet another standard function, go
right ahead, one more won't make a difference.

> Regarding whether to put this into qemu 1.0 or not -- my preference
> would be for 'not' because any change touching a lot of files has
> the potential to cause pending patches/pullreqs people are trying
> to get in before the feature freeze deadline to no longer apply.

Agree.

Re: [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-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-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-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-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-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-devel] [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