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(-)

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);

-- 
2.13.5

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/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?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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/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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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 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

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