| Message ID | 1319487523-19978-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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.
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.
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.
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
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
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. >
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
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.
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
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.
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
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.
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
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.
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
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 = {
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(-)