[v3,04/11] Rephrase assertion section

Submitted by Christophe de Dinechin on Feb. 8, 2018, 11:25 a.m.

Details

Message ID 20180208112531.19601-5-christophe@dinechin.org
State New
Headers show
Series "Updates to the style guide" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 8, 2018, 11:25 a.m.
From: Christophe de Dinechin <dinechin@redhat.com>

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 docs/spice_style.txt | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index 13032df6..eef4880f 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -99,10 +99,19 @@  FIXME and TODO
 
 Comments that are prefixed with `FIXME` describe a bug that need to be fixed. Generally, it is not allowed to commit new code having `FIXME` comment. Committing  `FIXME` is allowed only for existing bugs. Comments that are prefixed with `TODO` describe further features, optimization or code improvements, and they are allowed to be committed along with the relevant code.
 
-ASSERT
-------
+Assertions
+----------
+
+Use assertions liberally. Assertions help testing function arguments and function results validity. As a result, they make it easier to detect bugs. Also, by expressing the intent of the developer, they make the code easier to read.
+
+Several form of assertion exist. SPICE does not use the language `assert` much, but instead relies on the following variants:
+
+- `spice_assert`, defined in `common/log.h`, is never disabled at run-time and prints an error if the assertion fails.
+
+- `g_assert` and other variants defined in glib such as `g_assert_null`, can be diabled by setting `G_DISABLE_ASSERT` (which is never done in practice for SPICE code), and emits a message through the g_assertion_message function and its variants.
+
+The guidelines for selecting one or the other are not very clear from the existing code. The `spice_assert` variant may be preferred for SPICE-only code, as it makes it clearer that this assertion is coming from SPICE. The `g_assert` and variants may be preferred for assertions related to the use of glib.
 
-Use it freely. ASSERT helps testing function arguments and function results validity.  ASSERT helps detecting bugs and improve code readability and stability.
 
 sizeof
 ------

Comments

> 
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  docs/spice_style.txt | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 13032df6..eef4880f 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -99,10 +99,19 @@ FIXME and TODO
>  
>  Comments that are prefixed with `FIXME` describe a bug that need to be
>  fixed. Generally, it is not allowed to commit new code having `FIXME`
>  comment. Committing  `FIXME` is allowed only for existing bugs. Comments
>  that are prefixed with `TODO` describe further features, optimization or
>  code improvements, and they are allowed to be committed along with the
>  relevant code.
>  
> -ASSERT
> -------
> +Assertions
> +----------
> +
> +Use assertions liberally. Assertions help testing function arguments and
> function results validity. As a result, they make it easier to detect bugs.
> Also, by expressing the intent of the developer, they make the code easier
> to read.
> +

I come from the Windows world and I found here in our case the "Use assertions
liberally" a bit stronger if they are always used. Kind of always using
address sanitizer compiled in even in production. Recently in spice-server
we added code like:

    if (ENABLE_EXTRA_CHECKS) {
        spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
        spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
    }

the ENABLE_EXTRA_CHECKS is always true/false and is a compiled in constant
and is true only if explicitly enabled so the compiler will strip the entire
checks if not enabled (but still checking for syntax).

> +Several form of assertion exist. SPICE does not use the language `assert`
> much, but instead relies on the following variants:
> +
> +- `spice_assert`, defined in `common/log.h`, is never disabled at run-time
> and prints an error if the assertion fails.
> +
> +- `g_assert` and other variants defined in glib such as `g_assert_null`, can
> be diabled by setting `G_DISABLE_ASSERT` (which is never done in practice
> for SPICE code), and emits a message through the g_assertion_message
> function and its variants.
> +

typo: "diabled" -> "disabled"
No, our code as far as I remember should NEVER be compiled and used with
G_DISABLE_ASSERT enabled. Or maybe I'm confused by G_DISABLE_CHECKS ?
OT: maybe we should check this in the code (I think better place is common/log.c).

> +The guidelines for selecting one or the other are not very clear from the
> existing code. The `spice_assert` variant may be preferred for SPICE-only
> code, as it makes it clearer that this assertion is coming from SPICE. The
> `g_assert` and variants may be preferred for assertions related to the use
> of glib.
>  
> -Use it freely. ASSERT helps testing function arguments and function results
> validity.  ASSERT helps detecting bugs and improve code readability and
> stability.
>  
>  sizeof
>  ------

Frediano
> On 12 Feb 2018, at 09:21, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> docs/spice_style.txt | 15 ++++++++++++---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index 13032df6..eef4880f 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -99,10 +99,19 @@ FIXME and TODO
>> 
>> Comments that are prefixed with `FIXME` describe a bug that need to be
>> fixed. Generally, it is not allowed to commit new code having `FIXME`
>> comment. Committing  `FIXME` is allowed only for existing bugs. Comments
>> that are prefixed with `TODO` describe further features, optimization or
>> code improvements, and they are allowed to be committed along with the
>> relevant code.
>> 
>> -ASSERT
>> -------
>> +Assertions
>> +----------
>> +
>> +Use assertions liberally. Assertions help testing function arguments and
>> function results validity. As a result, they make it easier to detect bugs.
>> Also, by expressing the intent of the developer, they make the code easier
>> to read.
>> +
> 
> I come from the Windows world and I found here in our case the "Use assertions
> liberally" a bit stronger if they are always used.

I’m sorry, I can’t make sense of this sentence. I believe you mean that the recommendation is too strong (not stronger) because we never disable them?

Do you have any performance numbers on the extra cost of assertions?


> Kind of always using
> address sanitizer compiled in even in production. Recently in spice-server
> we added code like:
> 
>    if (ENABLE_EXTRA_CHECKS) {
>        spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>        spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
>    }
> 
> the ENABLE_EXTRA_CHECKS is always true/false and is a compiled in constant
> and is true only if explicitly enabled so the compiler will strip the entire
> checks if not enabled (but still checking for syntax).

Yes. This kind of code is the obvious result of being a bit confused on the usage of assertions.

That specific example is somewhat ridiculous, by the way. The two tests are one machine instruction each, I doubt they are even noticeable on any performance benchmark.

ENABLE_EXTRA_CHECKS is also used for additional checks that are somewhat expensive at run-time, like in display_channel_finalize. OK, but then why not make that a run-time flag instead of a compile-time flag? Unless the additional test code makes your code larger by 50%, but I doubt this is the case here…

> 
>> +Several form of assertion exist. SPICE does not use the language `assert`
>> much, but instead relies on the following variants:
>> +
>> +- `spice_assert`, defined in `common/log.h`, is never disabled at run-time
>> and prints an error if the assertion fails.
>> +
>> +- `g_assert` and other variants defined in glib such as `g_assert_null`, can
>> be diabled by setting `G_DISABLE_ASSERT` (which is never done in practice
>> for SPICE code), and emits a message through the g_assertion_message
>> function and its variants.
>> +
> 
> typo: "diabled" -> "disabled"
> No, our code as far as I remember should NEVER be compiled and used with
> G_DISABLE_ASSERT enabled.

I mention in the text that we don’t disable them in practice.

But why should that NEVER be done? (in uppercase, no less)? If some distro has a policy of building with assertions disabled, that’s their choice.

> Or maybe I'm confused by G_DISABLE_CHECKS ?
> OT: maybe we should check this in the code (I think better place is common/log.c).

Check what? That G_DISABLE_ASSERT is not set?

> 
>> +The guidelines for selecting one or the other are not very clear from the
>> existing code. The `spice_assert` variant may be preferred for SPICE-only
>> code, as it makes it clearer that this assertion is coming from SPICE. The
>> `g_assert` and variants may be preferred for assertions related to the use
>> of glib.
>> 
>> -Use it freely. ASSERT helps testing function arguments and function results
>> validity.  ASSERT helps detecting bugs and improve code readability and
>> stability.
>> 
>> sizeof
>> ------
> 
> Frediano
> 
> > On 12 Feb 2018, at 09:21, Frediano Ziglio <fziglio@redhat.com> wrote:
> > 
> >> 
> >> From: Christophe de Dinechin <dinechin@redhat.com>
> >> 
> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> >> ---
> >> docs/spice_style.txt | 15 ++++++++++++---
> >> 1 file changed, 12 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> >> index 13032df6..eef4880f 100644
> >> --- a/docs/spice_style.txt
> >> +++ b/docs/spice_style.txt
> >> @@ -99,10 +99,19 @@ FIXME and TODO
> >> 
> >> Comments that are prefixed with `FIXME` describe a bug that need to be
> >> fixed. Generally, it is not allowed to commit new code having `FIXME`
> >> comment. Committing  `FIXME` is allowed only for existing bugs. Comments
> >> that are prefixed with `TODO` describe further features, optimization or
> >> code improvements, and they are allowed to be committed along with the
> >> relevant code.
> >> 
> >> -ASSERT
> >> -------
> >> +Assertions
> >> +----------
> >> +
> >> +Use assertions liberally. Assertions help testing function arguments and
> >> function results validity. As a result, they make it easier to detect
> >> bugs.
> >> Also, by expressing the intent of the developer, they make the code easier
> >> to read.
> >> +
> > 
> > I come from the Windows world and I found here in our case the "Use
> > assertions
> > liberally" a bit stronger if they are always used.
> 
> I’m sorry, I can’t make sense of this sentence. I believe you mean that the
> recommendation is too strong (not stronger) because we never disable them?
> 

yes, "too strong"

> Do you have any performance numbers on the extra cost of assertions?
> 

Does it matter? Depends on many thing, but the "use liberally" seems to
indicate that we never care about the costs.

> 
> > Kind of always using
> > address sanitizer compiled in even in production. Recently in spice-server
> > we added code like:
> > 
> >    if (ENABLE_EXTRA_CHECKS) {
> >        spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> >        spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> >    }
> > 
> > the ENABLE_EXTRA_CHECKS is always true/false and is a compiled in constant
> > and is true only if explicitly enabled so the compiler will strip the
> > entire
> > checks if not enabled (but still checking for syntax).
> 
> Yes. This kind of code is the obvious result of being a bit confused on the
> usage of assertions.
> 
> That specific example is somewhat ridiculous, by the way. The two tests are
> one machine instruction each, I doubt they are even noticeable on any
> performance benchmark.
> 

One machine instruction? In which architecture?
Yes, this is not really hot path but I think there's a balance between
probability that the event happens, cost of not having the check and
costs having it. The boundaries are personal opinions. 

> ENABLE_EXTRA_CHECKS is also used for additional checks that are somewhat
> expensive at run-time, like in display_channel_finalize. OK, but then why
> not make that a run-time flag instead of a compile-time flag? Unless the
> additional test code makes your code larger by 50%, but I doubt this is the
> case here…
> 

Yes, as I said are opinions. And as I said ENABLE_EXTRA_CHECKS is a compile
time constant true or false so has no costs at runtime.

> > 
> >> +Several form of assertion exist. SPICE does not use the language `assert`
> >> much, but instead relies on the following variants:
> >> +
> >> +- `spice_assert`, defined in `common/log.h`, is never disabled at
> >> run-time
> >> and prints an error if the assertion fails.
> >> +
> >> +- `g_assert` and other variants defined in glib such as `g_assert_null`,
> >> can
> >> be diabled by setting `G_DISABLE_ASSERT` (which is never done in practice
> >> for SPICE code), and emits a message through the g_assertion_message
> >> function and its variants.
> >> +
> > 
> > typo: "diabled" -> "disabled"
> > No, our code as far as I remember should NEVER be compiled and used with
> > G_DISABLE_ASSERT enabled.
> 
> I mention in the text that we don’t disable them in practice.
> 
> But why should that NEVER be done? (in uppercase, no less)? If some distro
> has a policy of building with assertions disabled, that’s their choice.
> 

If you call g_assert to check runtime thing that can happen instead
of developer error and you remove the check at runtime this lead to
potential security issues so the NEVER that is we should ignore the
policy and build with assertions enabled even if the build policies
disable them. At this point is not their choice anymore.

> > Or maybe I'm confused by G_DISABLE_CHECKS ?
> > OT: maybe we should check this in the code (I think better place is
> > common/log.c).
> 
> Check what? That G_DISABLE_ASSERT is not set?
> 

Yes

> > 
> >> +The guidelines for selecting one or the other are not very clear from the
> >> existing code. The `spice_assert` variant may be preferred for SPICE-only
> >> code, as it makes it clearer that this assertion is coming from SPICE. The
> >> `g_assert` and variants may be preferred for assertions related to the use
> >> of glib.
> >> 
> >> -Use it freely. ASSERT helps testing function arguments and function
> >> results
> >> validity.  ASSERT helps detecting bugs and improve code readability and
> >> stability.
> >> 
> >> sizeof
> >> ------
> > 

Frediano
On Thu, Feb 08, 2018 at 12:25:24PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  docs/spice_style.txt | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 13032df6..eef4880f 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -99,10 +99,19 @@ FIXME and TODO
>  
>  Comments that are prefixed with `FIXME` describe a bug that need to be fixed. Generally, it is not allowed to commit new code having `FIXME` comment. Committing  `FIXME` is allowed only for existing bugs. Comments that are prefixed with `TODO` describe further features, optimization or code improvements, and they are allowed to be committed along with the relevant code.
>  
> -ASSERT
> -------
> +Assertions
> +----------
> +
> +Use assertions liberally. Assertions help testing function arguments
> and function results validity. As a result, they make it easier to
> detect bugs. Also, by expressing the intent of the developer, they
> make the code easier to read.  +

Given the rest of the thread, it's not clear if you mean "assertions
which abort the program", or runtime checks such as
g_warn_if_fail()/g_return_if_fail()/... I'm very much in favour of using
the latter functions liberally. I think we should avoid as much as
possible the use of anything calling abort(), especially in library
code. If a user can get the abort() to trigger, that means they can just
kill their VM, which is not a nice thing to do ;) So one should really
think hard before adding a g_assert() in the code.

Christophe