Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Christophe Fergeau]

On Thu, Feb 08, 2018 at 12:25:24PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  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


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Christophe Fergeau]

On Tue, Feb 13, 2018 at 02:40:38PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 13 Feb 2018, at 14:39, Frediano Ziglio  wrote:
> > 
> >>> 
> >>> On 12 Feb 2018, at 17:58, Frediano Ziglio  wrote:
> >>> 
> >>> In SPICE, as in Glib, ERROR is more "strong" than CRITICAL
> >>> (I don't know where this came, even in Solaris CRITICAL is more
> >>> strong like Linux, maybe an initial mistake).
> >>> By default CRITICAL is fatal (it calls abort) so ERROR is too.
> >> 
> >> I’ve not tested with the agent, but I know that for spicy, I regularly get
> >> “CRITICAL” about assertions without an abort:
> >> 
> >> (spicy:53020): GLib-GObject-CRITICAL **: g_object_get: assertion 
> >> 'G_IS_OBJECT
> >> (object)' failed
> >> (spicy:53020): GLib-GObject-CRITICAL **: g_object_unref: assertion
> >> 'G_IS_OBJECT (object)' failed
> >> 
> >> If the desired behavior is an abort, we are not configuring it properly.
> >> 
> >> 
> >> Christophe
> >> 
> > 
> > But this is a CRITICAL in spicy, not a ERROR in the server.
> > By default g_critical is not fatal, spice_critical yes.
> > Both g_error and spice_error are fatal.
> 
> My point was that an assertion failure was not aborting.
> 
> Do you feel that it’s logical and consistent? For me, it’s remarkably 
> confusing ;-)

g_assert() failures are aborting the program, g_{warn,return}_if_fail()
are triggering the kind of messages you quoted will not abort the
program unless you set G_DEBUG=fatal-criticals (there's also
fatal-warnings).

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Christophe de Dinechin]

> On 13 Feb 2018, at 14:39, Frediano Ziglio  wrote:
> 
>>> 
>>> On 12 Feb 2018, at 17:58, Frediano Ziglio  wrote:
>>> 
>>> In SPICE, as in Glib, ERROR is more "strong" than CRITICAL
>>> (I don't know where this came, even in Solaris CRITICAL is more
>>> strong like Linux, maybe an initial mistake).
>>> By default CRITICAL is fatal (it calls abort) so ERROR is too.
>> 
>> I’ve not tested with the agent, but I know that for spicy, I regularly get
>> “CRITICAL” about assertions without an abort:
>> 
>> (spicy:53020): GLib-GObject-CRITICAL **: g_object_get: assertion 'G_IS_OBJECT
>> (object)' failed
>> (spicy:53020): GLib-GObject-CRITICAL **: g_object_unref: assertion
>> 'G_IS_OBJECT (object)' failed
>> 
>> If the desired behavior is an abort, we are not configuring it properly.
>> 
>> 
>> Christophe
>> 
> 
> But this is a CRITICAL in spicy, not a ERROR in the server.
> By default g_critical is not fatal, spice_critical yes.
> Both g_error and spice_error are fatal.

My point was that an assertion failure was not aborting.

Do you feel that it’s logical and consistent? For me, it’s remarkably confusing 
;-)


Christophe

> 
> Frediano
> 
>>> 
> I agree, asserts should not be used as needed runtime checks.
 
 I’ll add that to the next writeup.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Christophe de Dinechin]

> On 12 Feb 2018, at 17:58, Frediano Ziglio  wrote:
> 
> In SPICE, as in Glib, ERROR is more "strong" than CRITICAL
> (I don't know where this came, even in Solaris CRITICAL is more
> strong like Linux, maybe an initial mistake).
> By default CRITICAL is fatal (it calls abort) so ERROR is too.

I’ve not tested with the agent, but I know that for spicy, I regularly get 
“CRITICAL” about assertions without an abort:

(spicy:53020): GLib-GObject-CRITICAL **: g_object_get: assertion 'G_IS_OBJECT 
(object)' failed
(spicy:53020): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT 
(object)' failed

If the desired behavior is an abort, we are not configuring it properly.


Christophe

> 
>>> I agree, asserts should not be used as needed runtime checks.
>> 
>> I’ll add that to the next writeup.
>> 
> 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Frediano Ziglio]

> 
> > On 12 Feb 2018, at 13:34, Frediano Ziglio  wrote:
> > 
> >>> 
> >>> On 12 Feb 2018, at 11:22, Frediano Ziglio  wrote:
> >>> 
> > 
> > On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
> > 
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> Signed-off-by: Christophe de Dinechin 
> >> ---
> >> 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”
> >> 
> >> OK. Now I understand what you mean.
> >> 
> >>> 
>  Do you have any performance numbers on the extra cost of assertions?
>  
> >>> 
> >>> Does it matter?
> >> 
> >> Yes it does. When someone refrains from writing an assert because they are
> >> afraid of the extra cost, it generally mean they have no clue and switched
> >> to FUD mode. A typical assert on a modern CPU is a test and a branch, so
> >> we
> >> are talking nanoseconds.
> >> 
> > 
> > If the rule is "never care" that include any path in the code.
> 
> OK. “Liberally” IMO does not mean “don’t care”.
> 
> Liberally means “feel free to add them as you identify conditions that are
> worth checking”. But based on the discussion, I think we need to add things
> to pay attention to, namely:
> 
> - Avoid side effects
> - Don’t use assert as an alternative for error checking
> - Be mindful of timing
> 
> If we add that, does that address your concerns?
> 

Yes.

> > As a practical example take into account Quic code. In encode function
> > there are 4 spice_assert. encode is called for each pixel of the image
> > to million of times for a big image.
> 
> Well, I decided to resort to hard data to figure out how much this case
> really mattered.
> 
> With a reduced test case like this:
> 
> https://pastebin.com/inwJnAmd
> 
> Running 1 iterations with assertions gives: “real 0m11.507s"
> Running without assertions gives: "real   0m0.128s"
> 
> So this is one case where assertions represent 100x the payload!!! Definitely
> worth looking into. If you ever want to use ENABLE_EXTRA_CHECKS anywhere,
> that would be a good place ;-)
> 

Ouch... was not expecting so bad either!

> Also, note that the loss is 10s for 10 billion pixels encoded, so that’s 1s
> per billion pixels, or 1s for 113 full 4K frames. At 60Hz, that suggests
> that the asserts could eat 50% of the CPU. Clearly, if I did not goof up in
> my evaluation, that needs some attention.
> 
> I filed https://spice-redmine.usersys.redhat.com/issues/62.
> 
> Finally, for the record, do not underestimate the compiler either. If you
> leave only the assertions, i.e run https://pastebin.com/DzPY7B1h, then you
> get a time of real0m0.005s, and you will notice that the compiler just
> removed the assertions entirely. It also took me three attempts before
> getting arguments to “encode” where the compiler would not statically remove
> the assertions. My first experiments gave absolutely no effect of
> assertions, because assertions were just optimized away at compile-time.
> 
> 
> > Yes, the test is taking nanoseconds
> > but multiplied by millions turn it to milliseconds multiplied by the images
> > and frames and VMs in a machine they starts to count.
> 
> To me, the only way to decide is to offer hard evidence, as I did above.
> 
> Otherwise, it’s really speculation. In the example above, with variations in
> just the call arguments, I can either have assertions wiped out statically
> or having 

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Christophe de Dinechin]

> On 12 Feb 2018, at 13:34, Frediano Ziglio  wrote:
> 
>>> 
>>> On 12 Feb 2018, at 11:22, Frediano Ziglio  wrote:
>>> 
> 
> On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> 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”
>> 
>> OK. Now I understand what you mean.
>> 
>>> 
 Do you have any performance numbers on the extra cost of assertions?
 
>>> 
>>> Does it matter?
>> 
>> Yes it does. When someone refrains from writing an assert because they are
>> afraid of the extra cost, it generally mean they have no clue and switched
>> to FUD mode. A typical assert on a modern CPU is a test and a branch, so we
>> are talking nanoseconds.
>> 
> 
> If the rule is "never care" that include any path in the code.

OK. “Liberally” IMO does not mean “don’t care”.

Liberally means “feel free to add them as you identify conditions that are 
worth checking”. But based on the discussion, I think we need to add things to 
pay attention to, namely:

- Avoid side effects
- Don’t use assert as an alternative for error checking
- Be mindful of timing

If we add that, does that address your concerns?

> As a practical example take into account Quic code. In encode function
> there are 4 spice_assert. encode is called for each pixel of the image
> to million of times for a big image.

Well, I decided to resort to hard data to figure out how much this case really 
mattered.

With a reduced test case like this:

https://pastebin.com/inwJnAmd

Running 1 iterations with assertions gives: “real   0m11.507s"
Running without assertions gives: "real 0m0.128s"

So this is one case where assertions represent 100x the payload!!! Definitely 
worth looking into. If you ever want to use ENABLE_EXTRA_CHECKS anywhere, that 
would be a good place ;-)

Also, note that the loss is 10s for 10 billion pixels encoded, so that’s 1s per 
billion pixels, or 1s for 113 full 4K frames. At 60Hz, that suggests that the 
asserts could eat 50% of the CPU. Clearly, if I did not goof up in my 
evaluation, that needs some attention.

I filed https://spice-redmine.usersys.redhat.com/issues/62.

Finally, for the record, do not underestimate the compiler either. If you leave 
only the assertions, i.e run https://pastebin.com/DzPY7B1h, then you get a time 
of real 0m0.005s, and you will notice that the compiler just removed the 
assertions entirely. It also took me three attempts before getting arguments to 
“encode” where the compiler would not statically remove the assertions. My 
first experiments gave absolutely no effect of assertions, because assertions 
were just optimized away at compile-time.


> Yes, the test is taking nanoseconds
> but multiplied by millions turn it to milliseconds multiplied by the images
> and frames and VMs in a machine they starts to count.

To me, the only way to decide is to offer hard evidence, as I did above.

Otherwise, it’s really speculation. In the example above, with variations in 
just the call arguments, I can either have assertions wiped out statically or 
having them count for 100x more than the body of the function…


> These spice_assert(s) came probably from a long history path where
> ASSERT/assert were used but not compiled in release code.
> Other example is functions like ring_remove which for performance reasons
> somebody decided to define inline but some with all the 

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Frediano Ziglio]

> 
> > On 12 Feb 2018, at 11:22, Frediano Ziglio  wrote:
> > 
> >>> 
> >>> On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
> >>> 
>  
>  From: Christophe de Dinechin 
>  
>  Signed-off-by: Christophe de Dinechin 
>  ---
>  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”
> 
> OK. Now I understand what you mean.
> 
> > 
> >> Do you have any performance numbers on the extra cost of assertions?
> >> 
> > 
> > Does it matter?
> 
> Yes it does. When someone refrains from writing an assert because they are
> afraid of the extra cost, it generally mean they have no clue and switched
> to FUD mode. A typical assert on a modern CPU is a test and a branch, so we
> are talking nanoseconds.
> 

If the rule is "never care" that include any path in the code.
As a practical example take into account Quic code. In encode function
there are 4 spice_assert. encode is called for each pixel of the image
to million of times for a big image. Yes, the test is taking nanoseconds
but multiplied by millions turn it to milliseconds multiplied by the images
and frames and VMs in a machine they starts to count.
These spice_assert(s) came probably from a long history path where
ASSERT/assert were used but not compiled in release code.
Other example is functions like ring_remove which for performance reasons
somebody decided to define inline but some with all the spice_assert in it
are enough big to take quite some bytes.

> 
> > Depends on many thing, but the "use liberally" seems to
> > indicate that we never care about the costs.
> 
> No, it means that in general, we do not care about the cost of an assert.
> 
>  Of course, don’t be stupid and write code like:
> 
>   assert(recursive_fibonacci(50) > 0)
> 
> because then the assert might cost a little more than a few nanoseconds
> (about 36 seconds on my laptop).
> 
> But in reality, asserts are generally used to test very simple conditions.

In Windows people expect these checks to disappear in release so people
do more expensive checks.
This "small" setting change make a very big difference.

> Bad cases of an expensive assert in performance critical code are extremely
> rare. If this happens, I expect code review to catch it, and even so, I
> would not even consider the removal of an assert for that reason without
> hard numbers. Which is why I asked you about hard numbers above :-)
> 

But the style apply also to any future code so it catches also the addition
case, not only removal. And as code was moved from ASSERT to 
spice_assert/g_assert
implicitly (maybe without people realizing) turned out in many additions.

> Remember, I recently did some measurements to prove that a record entry would
> not be too expensive to be left in the code all the time. Assertions are
> generally even less expensive than a flight recorder entry.
> 
> If you want, what I could do is add a rule that in general, the expression
> inside an assert should make no function calls and have no side effects.
> That would have two benefits:
> 
> - By avoiding function calls, you restrict expressions to things that can be
> computed locally, which generally means a few CPU cycles at most

But is hard to enforce in case of inline functions.

> - By avoiding side effects, you guarantee that the assert has no impact on
> the behavior of the program.
> 

I think no side effects (beside timing) are mandatory for asserts.

> Also, for C++ code, consider C++G I.6, i.e. thinking about writing
> 

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Christophe de Dinechin]

> On 12 Feb 2018, at 11:22, Frediano Ziglio  wrote:
> 
>>> 
>>> On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
>>> 
 
 From: Christophe de Dinechin 
 
 Signed-off-by: Christophe de Dinechin 
 ---
 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”

OK. Now I understand what you mean.

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

Yes it does. When someone refrains from writing an assert because they are 
afraid of the extra cost, it generally mean they have no clue and switched to 
FUD mode. A typical assert on a modern CPU is a test and a branch, so we are 
talking nanoseconds.


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

No, it means that in general, we do not care about the cost of an assert.

 Of course, don’t be stupid and write code like:

assert(recursive_fibonacci(50) > 0)

because then the assert might cost a little more than a few nanoseconds (about 
36 seconds on my laptop).

But in reality, asserts are generally used to test very simple conditions. Bad 
cases of an expensive assert in performance critical code are extremely rare. 
If this happens, I expect code review to catch it, and even so, I would not 
even consider the removal of an assert for that reason without hard numbers. 
Which is why I asked you about hard numbers above :-)

Remember, I recently did some measurements to prove that a record entry would 
not be too expensive to be left in the code all the time. Assertions are 
generally even less expensive than a flight recorder entry.

If you want, what I could do is add a rule that in general, the expression 
inside an assert should make no function calls and have no side effects. That 
would have two benefits:

- By avoiding function calls, you restrict expressions to things that can be 
computed locally, which generally means a few CPU cycles at most
- By avoiding side effects, you guarantee that the assert has no impact on the 
behavior of the program.

Also, for C++ code, consider C++G I.6, i.e. thinking about writing 
contract-like stuff, with “expect” for assertions that check pre-conditions, 
etc.


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

We are not going to play that game again, are we? You know exactly what I mean.

Any architecture I know of can do an integer compare for equality or >= in one 
machine instruction. That’s what I’m talking about.

Then obviously, depending on the surrounding context, you will also probably 
have a load (likely to be from hot cache, unless you are asserting for 
something that is not related to surrounding code), and a never-taken 
conditional branch, which the compiler is 

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 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.
+
+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
 --
-- 
2.13.5 (Apple Git-94)

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel