[Spice-devel] [PATCH v3 04/11] Rephrase assertion section
Frediano Ziglio
fziglio at redhat.com
Mon Feb 12 12:34:22 UTC 2018
>
> > On 12 Feb 2018, at 11:22, Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> >>>
> >>> 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”
>
> 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
> contract-like stuff, with “expect” for assertions that check pre-conditions,
> etc.
>
I.6 also spoke about the debug/production behaviour.
>
> >
> >>
> >>> 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 likely to mark statically as not
> taken because the assert macros nowadays have hints to that effect. Such a
> branch is effectively a no-op in most cases. So the effect on the
> surrounding code is complicated. In some cases, it may be null, e.g. if the
> value is already in a register, since the jump itself is going to be
> essentially a no-op.
>
I think we can agree the time is negligible in the cold path case
and also in these cases code cost is not so high.
>
> > 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.
>
> No, the boundaries are not personal opinion, that is precisely my point.
>
The fact that Windows setting are so different than Unix is IMO
a prove that these boundaries are opinions.
> If the code in question is a code where you are carefully watching for load
> latencies, cache utilization, and where an additional load or jump will
> impact performance, then by all means, avoid asserts in that code. That is
> *never* the case in spice code that I am aware of.
>
> Again, if that code is so performance-sensitive that you are pondering
> whether it’s OK, performance-wise, to add this expression to your code:
>
> *p ? “Hello” : “World"
>
> then OK, you have a point. I’m saying that because the expression above does
> exactly the same thing as the assert: a load, a test and a branch.
>
> What I am arguing is that we are, by far, not in code that is anywhere close
> to this situation, especially not in the code that has the if
> (ENABLE_EXTRA_CHECKS). I regret to not have seen that when the patches were
> submitted.
>
> >
> >> 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.
>
> Of course I know it’s a compile-time constant, thank you very much ;-)
>
> What I am disputing is that using a compile time constant here matters at
> all. I am also disputing that it’s a good idea. It carries the wrong
> impression that either:
> 1) Performance is so sensitive that we can’t even afford an assert here, or
> 2) That the asserts fail for the moment, and are temporarily disabled (which
> is how I read that code initially)
>
> So that code is not just ill-advised, it carries the wrong impression to
> someone reading it.
>
Not for me.
> I claim that it’s also totally useless. I’m willing to bet on it. If you can
Is not useless. Just not worth in production.
> build a test case, no matter how contrived, where you can measure the
> performance impact on the spice server of the following patch using an
> external tool, and you can actually isolate that performance impact out of
> the overall measurement noise reliably, then I’m willing to pay a round of
> beers to the whole team next time we meet:
>
>
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 4eaa959b..13ca3d1a 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -168,10 +168,8 @@ handle_msg_format(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
> {
> SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
>
> - if (ENABLE_EXTRA_CHECKS) {
> - spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> - spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> - }
> + spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> + spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
>
> int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> sizeof(StreamMsgFormat) - dev->msg_pos);
> if (n < 0) {
>
>
as said the style does not affect a single piece of code
but present and future code.
The initial discussion started about the "liberally" definition
that for me are "we never care about costs".
>
>
> >
> >>>
> >>>> +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.
>
> Ah, that’s a good point. But that’s an obvious misuse of asserts. I hope we
> don’t rely on asserts for functionality in our code. If that’s the case, it
> must be fixed.
>
> In any case, if that’s what we do, then the asserts are not a very good
> protection as far as security is concerned, since all they do is print
> something. It won’t abort your program, unlike the C library assert.
>
I hope you are wrong here and spice_assert/g_assert does not print
and continue. If not we have a problem.
I agree, asserts should not be used as needed runtime checks.
>
> >
> >>> 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
>
> Well, if we do rely on them for security, then maybe we should abort on
> failure using the C library assert for that.
>
Hope I'm wrong and I confuse other tests.
> >
> >>>
> >>>> +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
More information about the Spice-devel
mailing list