[Spice-devel] [PATCH v3 04/11] Rephrase assertion section
Christophe de Dinechin
cdupontd at redhat.com
Mon Feb 12 11:28:23 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.
> 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 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.
> 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.
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.
I claim that it’s also totally useless. I’m willing to bet on it. If you can 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) {
>
>>>
>>>> +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.
>
>>> 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.
>
>>>
>>>> +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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list