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

[Stefan Hajnoczi]

On Fri, Oct 28, 2011 at 12:07 PM, Markus Armbruster <address@hidden> wrote:
> Stefan Hajnoczi <address@hidden> writes:
>
>> 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.
>
> 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