On Fri, Oct 28, 2011 at 12:07 PM, Markus Armbruster <arm...@redhat.com> wrote: > Stefan Hajnoczi <stefa...@gmail.com> writes: > >> On Fri, Oct 28, 2011 at 9:59 AM, Markus Armbruster <arm...@redhat.com> wrote: >>> Stefan Hajnoczi <stefa...@gmail.com> writes: >>> >>>> On Fri, Oct 28, 2011 at 8:40 AM, Markus Armbruster <arm...@redhat.com> >>>> wrote: >>>>> Stefan Hajnoczi <stefa...@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 <s...@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