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

[Frediano Ziglio]

> 
> > On 12 Feb 2018, at 09:21, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >> 
> >> From: Christophe de Dinechin <dinechin at redhat.com>
> >> 
> >> Signed-off-by: Christophe de Dinechin <dinechin at 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