26.12.11

Introducing mozilla/Assertions.h to mfbt

Recently I landed changes to the Mozilla Framework Based on Templates (mfbt) implementing Assertions.h, the new canonical assertions implementation in C/C++ Mozilla code.

Runtime assertions

Using assertions, a developer can efficiently detect when his code goes awry because internal invariants were broken. Mozilla has many assertion facilities. NS_ASSERTION is the oldest, but unfortunately it can be ignored, and therefore historically has been. We later introduced NS_ABORT_IF_FALSE as as an actual assertion that fails hard, and it’s now widely used. But it’s quite unwieldy, and it can’t be used by code that doesn’t want to depend on XPCOM. (Who would?)

mfbt addresses latent concerns with existing runtime assertions by introducing MOZ_ASSERT, MOZ_ASSERT_IF, MOZ_ALWAYS_TRUE, MOZ_ALWAYS_FALSE, and MOZ_NOT_REACHED macros to make performing true assertions dead simple.

MOZ_ASSERT(expr) and MOZ_ASSERT_IF(ifexpr, expr)

MOZ_ASSERT(expr) is straightforward: pass an expression as its sole argument, and in debug builds, if that expression is falsy, the assertion fails and execution halts in a debuggable way.

#include "mozilla/Assertions.h"

void frobnicate(Thing* thing)
{
  MOZ_ASSERT(thing);
  thing->frob();
}

MOZ_ASSERT_IF(ifexpr, expr) addresses the case when you want to assert something, where the check to decide whether to assert isn’t otherwise needed. You’d rather not muddy up your code by adding an #ifdef and if statement around your assertion. (MOZ_ASSERT(!ifexpr || expr) is a workaround, but it’s not very readable.) SpiderMonkey’s experience suggests Mozilla code will get good mileage from MOZ_ASSERT_IF.

#include "mozilla/Assertions.h"

class Error
{
    const char* optionalDescription;

  public:
    /* If specified, desc must not be empty. */
    Error(const char* desc = NULL)
    {
      MOZ_ASSERT_IF(desc != NULL, desc[0] != '\0');
      optionalDescription = desc;
    }
};

MOZ_ALWAYS_TRUE(expr) and MOZ_ALWAYS_FALSE(expr)

Sometimes the expression for an assertion must always be executed for its side effects, and it can’t just be executed in debug builds. MOZ_ALWAYS_TRUE(expr) and MOZ_ALWAYS_FALSE(expr) support this idiom. These macros always evaluate their argument, and in debug builds that argument is asserted truthy or falsy.

#include "mozilla/Assertions.h"

/* JS_ValueToBoolean was fallible but no longer is. */
MOZ_ALWAYS_TRUE(JS_ValueToBoolean(cx, v, &b));

MOZ_NOT_REACHED(reason)

MOZ_NOT_REACHED(reason) indicates that the given point can’t be reached during execution: simply hitting it is a bug. (Think of it as a more-explicit form of asserting false.) It takes as an argument an explanation of why that point shouldn’t have been reachable.

#include "mozilla/Assertions.h"

// ...in a language parser...
void handle(BooleanLiteralNode node)
{
  if (node.isTrue())
    handleTrueLiteral();
  else if (node.isFalse())
    handleFalseLiteral();
  else
    MOZ_NOT_REACHED("boolean literal that's not true or false?");
}

Compile-time assertions

Most assertions must happen at runtime. But some assertions are static, depending only on constants, and could be checked during compilation. A compile time check is better than a runtime check: the developer need not ensure a test exercises that code, because the compiler itself enforces the assertion. Properly crafted static assertions can never be unwittingly broken.

MOZ_STATIC_ASSERT(cond, reason)

MOZ_STATIC_ASSERT(cond, reason) asserts a condition at compile time. In newer compilers, if the assertion fails, the compiler will also include reason in diagnostics.

#include "mozilla/Assertions.h"

struct S { ... };
MOZ_STATIC_ASSERT(sizeof(S) % sizeof(size_t) == 0,
                  "S should be a multiple of word size for efficiency");

MOZ_STATIC_ASSERT is implemented with an impressive pile of hacks which should work perfectly everywhere — except, rarely, gcc 4.2 (the current OS X compiler) when compiling C++ code. The failure mode requires MOZ_STATIC_ASSERT on line N not in an extern "C" code block and a second MOZ_STATIC_ASSERT on the same line N (in a different file) in an extern "C" block. And those two files have to be used when compiling a single file, with the extern "C"‘d assertion second. This seems improbable, so we’ll risk it til we drop gcc 4.2 support.

Possible improvements

Assertions.h is reasonably complete, but I have a few ideas I’ve been considering for improvements. Let me know what you think of them in comments.

Add an optional reason argument to MOZ_ASSERT, and maybe to MOZ_ALWAYS_TRUE and MOZ_ALWAYS_FALSE

MOZ_ASSERT takes only the condition to test as an argument. In contrast NS_ASSERTION and NS_ABORT_IF_FALSE take the condition and an explanatory string. MOZ_ASSERT‘s lack of explanation derives purely from its ancestry in the JS_ASSERT macro: it wasn’t deliberate.

Would it be helpful if MOZ_ASSERT, MOZ_ALWAYS_TRUE, and MOZ_ALWAYS_FALSE optionally took a reason? (Optional because some assertions, e.g. many non-null assertions, are self-explanatory.) We’d have to disable assertions for compilers not implementing variadic macros (I think our supported compilers implement them), or possibly lose the condition expression in the assertion failure message. A reason would make it easier to convert existing NS_ABORT_IF_FALSEs to MOZ_ASSERTs. Should we add an optional second argument to MOZ_ASSERT and the others?

Include __assume(0) or __builtin_unreachable() in MOZ_NOT_REACHED

__builtin_unreachable() and __assume(0) inform the compiler that subsequent code can’t be executed, providing optimization opportunities. It’s unclear how this affects debugging feedback like stack traces. If the optimizations destroy Breakpad-ability, that may be too big a loss. More research is needed here.

Another possibility might be to use __builtin_trap(). This may not communicate an optimization opportunity comparable to that provided by the other two options. (It can’t be equally informative because execution must be able to continue past a trap. Thus the two have different impacts on variable lifetimes. Whether __builtin_trap otherwise communicates “unlikelihood” well enough isn’t clear.) Perhaps __builtin_trap could be used in debug builds, and __builtin_unreachable could be used in optimized builds. Again: more research needed.

Use C11’s _Static_assert in MOZ_STATIC_ASSERT

New editions of C and C++ include built-in static assertion syntax. MOZ_STATIC_ASSERT expands to C++11’s static_assert(2 + 2 == 4, "ya rly") syntax when possible. It could be made to expand to C11’s _Static_assert('A' == 'A', "no wai") syntax in some other cases, but frankly I don’t hack enough C code to care as long as the static assertion actually happens. :-) This is bug 713531 if you’re interested in investigating.

Want more information?

Read Assertions.h. mfbt code has a high standard for code comments in interface descriptions, and for file names (the current Util.h being a notable exception which will be fixed). We want it to be reasonably obvious where to find what you need and how to use it by skimming mfbt/‘s contents and then skimming a few files’ contents. Good comments are key to that. You should find Assertions.h quite readable; please file a bug if you have improvements to suggest.

17 Comments »

  1. I previously tried adding __builtin_unreachable, and quit for a few reasons, listed here: https://bugzilla.mozilla.org/show_bug.cgi?id=585986. The latter two are probably no longer valid, so it’s probably worth looking into again, if the first can be fixed.

    Comment by Paul Biggar — 26.12.11 @ 11:00

  2. In theory assertions could be changed to evaluate their condition in release builds. In practice it seems unlikely to happen. No assertion has ever evaluated its condition, so that’d be a substantial change impeding adoption. I did originally think I could mark JS_Assert as not returning, which would have the same effect…but that’s a lie, it can return if you’re in a debugger and you quell the trap. I think MOZ_NOT_REACHED is the only macro really amenable to unreachability. But maybe I’m missing something.

    Comment by Jeff — 26.12.11 @ 11:25

  3. > NS_ASSERTION is the oldest, but unfortunately it can be ignored, and therefore
    > historically has been.

    This is very untrue.

    Jesse files bugs about layout assertions, and they often get fixed.

    Better still, reftests automatically go orange if they trigger unexpected NS_ASSERTIONS. And unlike fatal assertions, that doesn’t wipe out the results of the rest of the test suite. We should do this for mochitests too but for some reason or another that never eventuated.

    Back when I used to dogfood debug builds, I would regularly hit JS_ASSERTs. That’s one of the reasons I stopped dogfooding debug builds. So JS_ASSERT/MOZ_ASSERT are being ignored; we ignore them by not using debug builds.

    I have never appreciated the point of view that non-fatal assertions are worthless and fatal assertions are always better.

    Comment by Robert O'Callahan — 26.12.11 @ 21:27

  4. I believe I’ve seen bugs in the last few months where dbaron, in reviewing style system patches, has mentioned that the people writing them need to fix the style system assertions they’ve triggered and not noticed. I could be mistaken; I’m doing a lot of skimming of my bugmail these days, especially outside JS. NS_ASSERTION isn’t always ignored, and yes, Jesse is awesome. But it is often enough that the extra discretion doesn’t strike me as an advantage.

    I could as easily argue that fail-fast is better for not hoarding tinderbox time when a changeset has been weighed and found wanting in initial results. In truth, I think the merits and demerits cut about the same both ways, so I don’t see early crashes on tinderbox as categorically worse than delayed, maybe-complete results. (Especially since most people’s patches can and should be smokescreened against a small subset of Mochitests, often weeding out 99% of problems, before testing on tinderboxen.)

    I have never appreciated the converse view. :-) I think we’re going to have to agree to disagree on this point. But I think most people are happier with assertions that break like assert does than the other way.

    As final data points, WebKit’s and V8’s ASSERT macros are fatal.

    Comment by Jeff — 27.12.11 @ 00:03

  5. Why does the clang version of MOZ_STATIC_ASSERT use reason but the others all use #cond ?

    The last time I hit an assertion it was a false positive. If it had been a fatal assertion, I would have been even more annoyed than I already was.

    I also occasionally appear to hit a false positive assertion in JS in an obscure bug requiring a computer that is so slow that the slow script timeout triggers while trying to show the slow script timeout dialog. (I actually get about four of those dialogs in all, but some of them may be caused by the slow script timeout triggering because of the time it takes me to use the debugger to ignore the assertion.)

    Comment by Neil Rashbrook — 27.12.11 @ 03:33

  6. But I think most people are happier with assertions that break like assert does than the other way.

    As long as you give me something that interrupts as NS_ASSERTION currently does without shooting the build down, I am with you. I am currently very eager to place NS_ASSERTION as a mark, that I personally care about it, when it goes wrong. If I look at table webkit code the asserts prevent mainly things that would crash anyway. A lot of the NS_ASSERTION in layout will not be followed by a crash but by wrong rendering because basic assumptions are violated. I like to get the disruptive information but I would not want to tear the browser down because a table is at the wrong place. This would be NS_WARN… but they are so frequent that they got silenced (I see 19 of those when my debug build starts and obviously some of them are very old and nobody cares.)

    Comment by Bernd — 27.12.11 @ 07:48

  7. The clang version does the right thing, the others do the wrong. Zack noticed this and filed a bug; I’ll fix it today.

    What’s the bug for that JS assertion? If it’s wrong, as it sounds like it is, it shouldn’t be there.

    Comment by Jeff — 27.12.11 @ 09:55

  8. Sounds like you want something log-gy, then, not an assertion. We have various logging stuff already, so I guess you’re concerned that it’s too obtuse to work with, compared to just a simple do-this, spew-to-console. Perhaps we should add logging stuff to mfbt. I don’t know much about logging APIs, so I should probably not be the person to design and implement one.

    I often add assertions to my code that I know won’t work, while I’m writing code. Then I either fix them before landing, or I remove them and indicate any remaining deficiency in a comment, and usually a bug too. This tends to work pretty well for me.

    Comment by Jeff — 27.12.11 @ 10:04

  9. I believe I’ve seen bugs in the last few months where dbaron, in reviewing style system patches, has mentioned that the people writing them need to fix the style system assertions they’ve triggered and not noticed.

    That alone doesn’t mean anything. David knows the assertion will fire in some situation, but maybe the person writing the code hasn’t hit that path yet.

    I could as easily argue that fail-fast is better for not hoarding tinderbox time when a changeset has been weighed and found wanting in initial results.

    Which is better: identifying 10 test failures in a single Tinderbox run and fixing all of them, or doing 10 Tinderbox runs fixing one failure at a time? The former of course, both for Tinderbox/tryserver usage and more importantly for the developer.

    In practice I run the patch(es) through Tinderbox, either try or on inbound, and if there are failures I know which subset of tests I need to rerun locally after I think I might have fixed the failures. Unless test failures are fatal.

    As final data points, WebKit’s and V8′s ASSERT macros are fatal.

    Webkit layout code hardly has any assertions compared to ours. That’s not good for them.

    Bernd is exactly right. Fatal assertions probably make sense in the JS engine where almost any kind of bug will lead to a crash anyway. They often do not make sense in layout, where many bugs result in nothing more harmful than incorrect page rendering.

    Sounds like you want something log-gy, then, not an assertion.

    Not at all. NS_ASSERTION has clear semantics: when it fires, we have a bug. None of our log levels mean that.

    Comment by Robert O'Callahan — 27.12.11 @ 14:53

  10. See http://blog.mozilla.com/nnethercote/2011/06/07/what-is-the-point-of-non-fatal-assertions/ of course.

    Comment by Robert O'Callahan — 27.12.11 @ 14:59

  11. I couldn’t resist:
    http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html

    Comment by Robert O'Callahan — 27.12.11 @ 15:53

  12. So, to double-check, these are fatal-in-debug, no-op-in-release assertions? Not arguing for whether it should be fatal (at the moment), but due to the history of NS_ASSERTION these things should be spelled out very clearly… and somehow this post manages to not be explicit. (“execution halts in a debuggable way” is, sadly, too easily lost in the other text IMHO.)

    I believe – I wasn’t around at the time – that there used to be something that was NS_ASSERTION-without-the-message, and they were mass-converted to NS_ASSERTION-with-the-message at some point; seems reasonable to guess then that making sure the new things have a message would be a good idea. There seems to still be a few examples lying around in comm-central.

    Comment by Mook — 27.12.11 @ 18:12

  13. David knows the assertion will fire in some situation, but maybe the person writing the code hasn’t hit that path yet.

    It’s an assumption that’s doubtless wrong at least sometimes, but I assume people have run at least a fair subset of the relevant tests when posting patches for review, and the tests either pass or any remaining failures are noted so that I can evaluate without respect to those known problems. To do otherwise is to potentially waste the reviewer’s time. Everyone does that on occasion, sure. But there’s a difference between mistakenness and inattentiveness to potential test failure.

    Which is better: identifying 10 test failures in a single Tinderbox run and fixing all of them, or doing 10 Tinderbox runs fixing one failure at a time?

    I think this is the exceptional case, made more so by proper preliminary local testing to smoke out baseline test failures.

    What is your position on compile errors halting the build? I’ve almost never encountered so many failures piling atop each other in a single tinderbox run, except a couple times in patches that fell afoul of Windows-specific compile errors. But I wouldn’t change the fatality of errors (compile or runtime) just to suit those edge cases.

    Fatal assertions probably make sense in the JS engine where almost any kind of bug will lead to a crash anyway.

    Yet this doesn’t explain why we use fatal assertions even when execution might well proceed without notable problem should the assertion fail. Indeed, in rare instances we use a LOCAL_ASSERT macro which includes backstop code (return NULL leading to script execution halting without throwing an exception) for the case where the assertion fails in a release build, because the corresponding code is so hairy. We use fatal assertions even when there’s no particular dangerous consequence to doing so, but merely as matter-of-course verification of an assumption.

    There’s also substantial value to not requiring the developer to decide whether his assertion is one verifying an important requirement or uncovering mostly harmless error.

    A new log level, or something, might be a fair alternative. But I worry about such a thing being overused when the test is well-understood, and where a fatal failure would get attention a logging message would simply be ignored.

    Comment by Jeff — 28.12.11 @ 08:15

  14. Yes, fatal in debug, no-op in release. I don’t actually intend this post to be long-term documentation of all this; that’s what the comments in Assertions.h are for. And I believe those comments clearly spell out the semantics. If you disagree, I’m happy to amend them in whatever way makes them clear to you, of course. But this post is mostly announcement, with few enough extra details so people who won’t immediately look at the header will know what’s available to use.

    Comment by Jeff — 28.12.11 @ 08:20

  15. Hm, I see you noted the compiler point in your separate post, which I’m reading now. :-)

    Comment by Jeff — 28.12.11 @ 08:24

  16. Sometimes assertions can give you compiler warnings in optimized builds due to unused variables, e.g.:

    int x = widget->frob();
    MOZ_ASSERT(x != 0);

    where x is otherwise unused. When MOZ_ASSERT expands to a nop in optimized builds, this may trigger a warning, depending on your compiler flags. One way to avoid this is to use this clever sizeof trick:

    #ifdef DEBUG
    # define MOZ_ASSERT(expr_) /* as before */
    #else
    # define MOZ_ASSERT(expr_) ((void)sizeof(expr_))
    #endif /* DEBUG */

    The sizeof ensures that the code doesn’t get evaluated at runtime, but the compiler will now believe that any variable that appears in the expression is used.

    The only potential problem I can think of is C99’s VLAs, where taking the size of a VLA is a runtime operation, not a compile-time one. But you would never assert on a VLA (it would always decay into a non-null pointer). I also don’t know if C++11 added any new features that could potentially make sizeof a runtime operation.

    Comment by Adam Rosenfield — 30.12.11 @ 10:58

  17. The sizeof trick is clever. Mozilla’s assertions quite often depend upon DEBUG-only variables, however, so using sizeof wouldn’t work for us. One trick we’ve recently started using is this:

    template <typename T>
    class DebugOnly
    {
    #ifdef DEBUG
    T t;
    #endif
    public:
    #ifdef DEBUG
    T& operator=(const T& t) { this->t = t; return *this; }
    operator T() { return t; }
    // …and whatever other operators become needed
    #else
    T& operator=(const T& t) { }
    // …other corresponding no-op versions
    #endif
    };

    void foo(Widget* widget)
    {
    DebugOnly<int> x = widget->frob();
    MOZ_ASSERT(x != 0);
    }

    Because this only elides the store for the variable and doesn’t necessarily elide computation of the right-hand side, I am somewhat ambivalent about its preferability to just enclosing the entire thing in an #ifdef. EIBTI and all that. But if used carefully, it does look a little cleaner.

    sizeof is a runtime operation on C99’s variable-length arrays? Ugh. Mozilla’s pretty much never used them as C++ has (or we can create) better, and safer (against stack overflow) alternatives, thankfully. C++11 doesn’t include variable-length arrays, and its sizeof remains a compile-time construct.

    Comment by Jeff — 31.12.11 @ 11:53

RSS feed for comments on this post. TrackBack URI

Leave a comment

HTML: You can use these tags: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>