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 <address@hidden> wrote:
> Stefan Hajnoczi <address@hidden> writes:
>
>> On Fri, Oct 28, 2011 at 8:40 AM, Markus Armbruster <address@hidden> wrote:
>>> Stefan Hajnoczi <address@hidden> 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 <address@hidden>
>>>>> ---
>>>>>  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