| Message ID | 20170818222313.13391-1-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/18/2017 07:23 PM, Eric Blake wrote: > We already have several files that knowingly require assert() > to work. While we do NOT want to encourage the use of > 'assert(side-effects)' (that is a bad practice that prevents > copy-and-paste of code to other projects that CAN disable > assertions; plus it costs unnecessary reviewer mental cycles > to remember our project policy on crippling asserts), we DO > want to send a message that anyone that disables assertions > has to tweak code in order to compile, making it obvious that > we are not going to support their efforts. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > First mentioned as an idea here: > http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html > but I'm titling this RFC as I'm not 100% convinced we want to make > it a project-wide, rather than a per-file decision. I think project-wide is the correct way. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > include/qemu/osdep.h | 12 ++++++++++++ > hw/scsi/mptsas.c | 4 ---- > hw/virtio/virtio.c | 4 ---- > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index 6855b94bbf..9e745a8af9 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -107,6 +107,18 @@ extern int daemon(int, int); > #include "glib-compat.h" > #include "qemu/typedefs.h" > > +/* > + * We have a lot of unaudited code that will fail in strange ways if > + * you disable assertions at compile-time. You are on your own if > + * you cripple these safety-checks. > + */ > +#ifdef NDEBUG > +#error building with NDEBUG is not supported > +#endif > +#ifdef G_DISABLE_ASSERT > +#error building with G_DISABLE_ASSERT is not supported > +#endif > + > #ifndef O_LARGEFILE > #define O_LARGEFILE 0 > #endif > diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c > index 765ab53c34..3b93f12cdb 100644 > --- a/hw/scsi/mptsas.c > +++ b/hw/scsi/mptsas.c > @@ -1236,11 +1236,7 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq) > n = qemu_get_be32(f); > /* TODO: add a way for SCSIBusInfo's load_request to fail, > * and fail migration instead of asserting here. > - * When we do, we might be able to re-enable NDEBUG below. > */ > -#ifdef NDEBUG > -#error building with NDEBUG is not supported > -#endif > assert(n >= 0); > > pci_dma_sglist_init(&req->qsg, pci, n); > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 464947f76d..2778adabcc 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1025,11 +1025,7 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz) > > /* TODO: teach all callers that this can fail, and return failure instead > * of asserting here. > - * When we do, we might be able to re-enable NDEBUG below. > */ > -#ifdef NDEBUG > -#error building with NDEBUG is not supported > -#endif > assert(ARRAY_SIZE(data.in_addr) >= data.in_num); > assert(ARRAY_SIZE(data.out_addr) >= data.out_num); >
On 19.08.2017 00:23, Eric Blake wrote: > We already have several files that knowingly require assert() > to work. While we do NOT want to encourage the use of > 'assert(side-effects)' (that is a bad practice that prevents > copy-and-paste of code to other projects that CAN disable > assertions; plus it costs unnecessary reviewer mental cycles > to remember our project policy on crippling asserts), we DO > want to send a message that anyone that disables assertions > has to tweak code in order to compile, making it obvious that > we are not going to support their efforts. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > First mentioned as an idea here: > http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html > but I'm titling this RFC as I'm not 100% convinced we want to make > it a project-wide, rather than a per-file decision. I think we should make this project-wide. Otherwise we will have the discussion again and again whether it is ok to compile with NDEBUG or not. Reviewed-by: Thomas Huth <thuth@redhat.com>
Eric Blake <eblake@redhat.com> writes: > We already have several files that knowingly require assert() > to work. While we do NOT want to encourage the use of > 'assert(side-effects)' (that is a bad practice that prevents > copy-and-paste of code to other projects that CAN disable > assertions; plus it costs unnecessary reviewer mental cycles > to remember our project policy on crippling asserts), we DO > want to send a message that anyone that disables assertions > has to tweak code in order to compile, making it obvious that > we are not going to support their efforts. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > First mentioned as an idea here: > http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html > but I'm titling this RFC as I'm not 100% convinced we want to make > it a project-wide, rather than a per-file decision. I'm fine with project-wide, but does it have to be a hard error? Can we make it a warning? Same effect by default, but it lets people experiment more easily with --disable-werror.
On 21 August 2017 at 10:31, Markus Armbruster <armbru@redhat.com> wrote: > I'm fine with project-wide, but does it have to be a hard error? Can we > make it a warning? Same effect by default, but it lets people > experiment more easily with --disable-werror. It won't be the same effect by default for our releases, which don't use -Werror. I don't think we want to make it easy for people to turn this off -- and it's only commenting out a single #error line if anybody really does want to mess about with it. thanks -- PMM
On 08/19/2017 12:23 AM, Eric Blake wrote: > We already have several files that knowingly require assert() > to work. While we do NOT want to encourage the use of > 'assert(side-effects)' (that is a bad practice that prevents > copy-and-paste of code to other projects that CAN disable > assertions; plus it costs unnecessary reviewer mental cycles > to remember our project policy on crippling asserts), we DO > want to send a message that anyone that disables assertions > has to tweak code in order to compile, making it obvious that > we are not going to support their efforts. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > First mentioned as an idea here: > http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html > but I'm titling this RFC as I'm not 100% convinced we want to make > it a project-wide, rather than a per-file decision. I think project wide is just right for what you describe: tell the people before disabling assertions you have to change the code (and preferably at least examine each usage of assert project-wide). OTOH I do think this is to some degree institutionalizing a bad practice (you say we do not want to do that, but IMHO refusing to build with NDEBUG makes only sense if we want to alter the semantic of assert so that once bad becomes acceptable). I can live with that, but I'm not happy about it. Have we considered rolling our own construct which is designed to exhibit the properties we desire? I mean, if it's about the side effects we could create something like q_assert(cond) and state that cond is evaluate exactly once (and depending on what we want to have, make the actions on !cond (calling abort(), producing a diagnostic message) compile time tweak-able or not). I assume we could then convert each usage of assert to the safe q_assert programmatically. Regards, Halil
On 08/22/2017 06:19 AM, Halil Pasic wrote: > OTOH I do think this is to some degree institutionalizing a bad practice > (you say we do not want to do that, but IMHO refusing to build with > NDEBUG makes only sense if we want to alter the semantic of assert so > that once bad becomes acceptable). I can live with that, but I'm not > happy about it. Have we considered rolling our own construct which is > designed to exhibit the properties we desire? I've thought about it, and it may have even been discussed on the list perhaps (although I didn't go searching to verify). > > I mean, if it's about the side effects we could create something like > q_assert(cond) and state that cond is evaluate exactly once (and > depending on what we want to have, make the actions on !cond (calling > abort(), producing a diagnostic message) compile time tweak-able or > not). I assume we could then convert each usage of assert to the safe > q_assert programmatically. I'd prefer that if we are going to introduce our own construct that always evaluates side effects, and only has a compile-time switch on whether to abort() or (foolishly) plow on, that we name it something without 'assert' in the name, so that reviewers don't have to be confused about remembering which variant evaluates side effects. Maybe: q_verify(cond) is a good name, which performs the side effects of 'cond' no matter what, but also allows us to abort() if cond fails vs. ignore the failure; perhaps where we make a compile-time decision between the two behaviors as a configure --enable-FOO flag. Converting all existing assert() and g_assert() to q_verify() should be fairly simple, if we like the idea.
Eric Blake <eblake@redhat.com> writes: > On 08/22/2017 06:19 AM, Halil Pasic wrote: > >> OTOH I do think this is to some degree institutionalizing a bad practice >> (you say we do not want to do that, but IMHO refusing to build with >> NDEBUG makes only sense if we want to alter the semantic of assert so >> that once bad becomes acceptable). I can live with that, but I'm not >> happy about it. Have we considered rolling our own construct which is >> designed to exhibit the properties we desire? > > I've thought about it, and it may have even been discussed on the list > perhaps (although I didn't go searching to verify). > >> >> I mean, if it's about the side effects we could create something like >> q_assert(cond) and state that cond is evaluate exactly once (and >> depending on what we want to have, make the actions on !cond (calling >> abort(), producing a diagnostic message) compile time tweak-able or >> not). I assume we could then convert each usage of assert to the safe >> q_assert programmatically. > > I'd prefer that if we are going to introduce our own construct that > always evaluates side effects, and only has a compile-time switch on > whether to abort() or (foolishly) plow on, that we name it something > without 'assert' in the name, so that reviewers don't have to be > confused about remembering which variant evaluates side effects. Maybe: > > q_verify(cond) > > is a good name, which performs the side effects of 'cond' no matter > what, but also allows us to abort() if cond fails vs. ignore the > failure; perhaps where we make a compile-time decision between the two > behaviors as a configure --enable-FOO flag. > > Converting all existing assert() and g_assert() to q_verify() should be > fairly simple, if we like the idea. About as simple as expanding tabs. CODING_STYLE demands spaces rather than tabs since 2009. 455 out of 3542 C source files have tabs. Only a minority of them is imported from other projects and thus exempt from CODING_STYLE. I vote for frying bigger fish. I also vote for using standard C when standard C is servicable.
On 08/21/2017 05:08 AM, Peter Maydell wrote: > On 21 August 2017 at 10:31, Markus Armbruster <armbru@redhat.com> wrote: >> I'm fine with project-wide, but does it have to be a hard error? Can we >> make it a warning? Same effect by default, but it lets people >> experiment more easily with --disable-werror. > > It won't be the same effect by default for our releases, which > don't use -Werror. > > I don't think we want to make it easy for people to turn this > off -- and it's only commenting out a single #error line if > anybody really does want to mess about with it. Two lines, actually, since we are also prohibiting disabling g_assert(). Anyways, as the topic came up again on the list, should I promote this patch from RFC to actual patch? Whose tree would it go in through?
On 08/24/2017 02:51 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 08/22/2017 06:19 AM, Halil Pasic wrote: >> >>> OTOH I do think this is to some degree institutionalizing a bad practice >>> (you say we do not want to do that, but IMHO refusing to build with >>> NDEBUG makes only sense if we want to alter the semantic of assert so >>> that once bad becomes acceptable). I can live with that, but I'm not >>> happy about it. Have we considered rolling our own construct which is >>> designed to exhibit the properties we desire? >> >> >> I'd prefer that if we are going to introduce our own construct that >> always evaluates side effects, and only has a compile-time switch on >> whether to abort() or (foolishly) plow on, that we name it something >> without 'assert' in the name, so that reviewers don't have to be >> confused about remembering which variant evaluates side effects. Maybe: >> >> q_verify(cond) >> > > I vote for frying bigger fish. > > I also vote for using standard C when standard C is servicable. So if it were up to me alone, the answer is: I'm NOT going to add any new construct (whether spelled q_verify() or otherwise), and will merely document in the commit message that we discussed this as an alternative (so someone who wants to disable #error can get a git history of what went into the decision). Also, it sounds like we want to keep it #error, not #warn. But if anyone else has strong opinions before we promote this from RFC to actual patch, I'm still interested in your arguments.
On 05.09.2017 21:50, Eric Blake wrote: > On 08/24/2017 02:51 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 08/22/2017 06:19 AM, Halil Pasic wrote: [...] >>> I'd prefer that if we are going to introduce our own construct that >>> always evaluates side effects, and only has a compile-time switch on >>> whether to abort() or (foolishly) plow on, that we name it something >>> without 'assert' in the name, so that reviewers don't have to be >>> confused about remembering which variant evaluates side effects. Maybe: >>> >>> q_verify(cond) >> >> I vote for frying bigger fish. >> >> I also vote for using standard C when standard C is servicable. > > So if it were up to me alone, the answer is: > > I'm NOT going to add any new construct (whether spelled q_verify() or > otherwise), and will merely document in the commit message that we > discussed this as an alternative (so someone who wants to disable #error > can get a git history of what went into the decision). > > Also, it sounds like we want to keep it #error, not #warn. > > But if anyone else has strong opinions before we promote this from RFC > to actual patch, I'm still interested in your arguments. You asked for opinions, so here's mine: I agree with you, please do *not* add a new QEMU-specific construct here. assert() should be a well-known C construct that every programmer should have understood. You also need it for other projects. If you haven't understood that it's a macro and has side-effects, you should learn it (e.g. during patch review), not avoid it, otherwise you'll run into problems in another project that is using it again. But IMHO we should still try to get rid of wrong usage of assert() in the QEMU sources. So maybe we could allow building with NDEBUG one day for the brave people who need the extra percent of additional speed. But as long as we're not there, I think this patch is a good thing to avoid wrong expectations. Thomas
On 09/05/2017 09:50 PM, Eric Blake wrote: > On 08/24/2017 02:51 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 08/22/2017 06:19 AM, Halil Pasic wrote: >>> >>>> OTOH I do think this is to some degree institutionalizing a bad practice >>>> (you say we do not want to do that, but IMHO refusing to build with >>>> NDEBUG makes only sense if we want to alter the semantic of assert so >>>> that once bad becomes acceptable). I can live with that, but I'm not >>>> happy about it. Have we considered rolling our own construct which is >>>> designed to exhibit the properties we desire? >>> > >>> >>> I'd prefer that if we are going to introduce our own construct that >>> always evaluates side effects, and only has a compile-time switch on >>> whether to abort() or (foolishly) plow on, that we name it something >>> without 'assert' in the name, so that reviewers don't have to be >>> confused about remembering which variant evaluates side effects. Maybe: >>> >>> q_verify(cond) >>> > >> >> I vote for frying bigger fish. >> >> I also vote for using standard C when standard C is servicable. > > So if it were up to me alone, the answer is: > > I'm NOT going to add any new construct (whether spelled q_verify() or > otherwise), and will merely document in the commit message that we > discussed this as an alternative (so someone who wants to disable #error > can get a git history of what went into the decision). > > Also, it sounds like we want to keep it #error, not #warn. > > But if anyone else has strong opinions before we promote this from RFC > to actual patch, I'm still interested in your arguments. > I'm fine with this outcome: it is a minimal solution to a real problem. My initiative was a kind of bike-shedding: any devel with enough exposure to qemu to matter will learn soon enough that the assert macro is special in this project. Regards, Halil
On 06/09/2017 07:26, Thomas Huth wrote: > You asked for opinions, so here's mine: I agree with you, please do > *not* add a new QEMU-specific construct here. assert() should be a > well-known C construct that every programmer should have understood. You > also need it for other projects. If you haven't understood that it's a > macro and has side-effects, you should learn it (e.g. during patch > review), not avoid it, otherwise you'll run into problems in another > project that is using it again. > > But IMHO we should still try to get rid of wrong usage of assert() in > the QEMU sources. So maybe we could allow building with NDEBUG one day > for the brave people who need the extra percent of additional speed. It's not only about the side effects. There are a couple cases in migration (IIRC) where our error propagation is not up to the task, and failing assertions are used to validate untrusted input. NDEBUG in that case can introduce a known vulnerability. > But as long as we're not there, I think this patch is a good thing to > avoid wrong expectations. Agreed. Paolo
On 11 September 2017 at 11:30, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 06/09/2017 07:26, Thomas Huth wrote: >> You asked for opinions, so here's mine: I agree with you, please do >> *not* add a new QEMU-specific construct here. assert() should be a >> well-known C construct that every programmer should have understood. You >> also need it for other projects. If you haven't understood that it's a >> macro and has side-effects, you should learn it (e.g. during patch >> review), not avoid it, otherwise you'll run into problems in another >> project that is using it again. >> >> But IMHO we should still try to get rid of wrong usage of assert() in >> the QEMU sources. So maybe we could allow building with NDEBUG one day >> for the brave people who need the extra percent of additional speed. > > It's not only about the side effects. There are a couple cases in > migration (IIRC) where our error propagation is not up to the task, and > failing assertions are used to validate untrusted input. NDEBUG in that > case can introduce a known vulnerability. > >> But as long as we're not there, I think this patch is a good thing to >> avoid wrong expectations. > > Agreed. Mmm. The only thing that makes me hesitate is that this is moving the flagging up that we rely on assert() causing a failure away from the places where we do that -- if we want in future to fix these migration issues then they'll get harder to find. (If we already think we have more of those that aren't marked by testing NDEBUG then we'd already need to do a wider code audit anyway, though...) thanks -- PMM
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 6855b94bbf..9e745a8af9 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -107,6 +107,18 @@ extern int daemon(int, int); #include "glib-compat.h" #include "qemu/typedefs.h" +/* + * We have a lot of unaudited code that will fail in strange ways if + * you disable assertions at compile-time. You are on your own if + * you cripple these safety-checks. + */ +#ifdef NDEBUG +#error building with NDEBUG is not supported +#endif +#ifdef G_DISABLE_ASSERT +#error building with G_DISABLE_ASSERT is not supported +#endif + #ifndef O_LARGEFILE #define O_LARGEFILE 0 #endif diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index 765ab53c34..3b93f12cdb 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -1236,11 +1236,7 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq) n = qemu_get_be32(f); /* TODO: add a way for SCSIBusInfo's load_request to fail, * and fail migration instead of asserting here. - * When we do, we might be able to re-enable NDEBUG below. */ -#ifdef NDEBUG -#error building with NDEBUG is not supported -#endif assert(n >= 0); pci_dma_sglist_init(&req->qsg, pci, n); diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 464947f76d..2778adabcc 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1025,11 +1025,7 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz) /* TODO: teach all callers that this can fail, and return failure instead * of asserting here. - * When we do, we might be able to re-enable NDEBUG below. */ -#ifdef NDEBUG -#error building with NDEBUG is not supported -#endif assert(ARRAY_SIZE(data.in_addr) >= data.in_num); assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
We already have several files that knowingly require assert() to work. While we do NOT want to encourage the use of 'assert(side-effects)' (that is a bad practice that prevents copy-and-paste of code to other projects that CAN disable assertions; plus it costs unnecessary reviewer mental cycles to remember our project policy on crippling asserts), we DO want to send a message that anyone that disables assertions has to tweak code in order to compile, making it obvious that we are not going to support their efforts. Signed-off-by: Eric Blake <eblake@redhat.com> --- First mentioned as an idea here: http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html but I'm titling this RFC as I'm not 100% convinced we want to make it a project-wide, rather than a per-file decision. include/qemu/osdep.h | 12 ++++++++++++ hw/scsi/mptsas.c | 4 ---- hw/virtio/virtio.c | 4 ---- 3 files changed, 12 insertions(+), 8 deletions(-)