Performing assertions in Mozilla code: use #include "mozilla/Assertions.h" to do it

[Jeff Walden]

NS_ASSERTION and NS_ABORT_IF_FALSE both have obvious, well-recognized flaws that make them unsuitable for use in all Mozilla code -- non-fatality, excessive length, a mandatory explanation even for the most obvious assertions, and excess XPCOM_DebugBreak complexity among them.

Last week I landed changes to centralize assertion support in "mozilla/Assertions.h" in mfbt. This header includes assertion macros akin to the ones in the JS engine. Feel free to use them in any Mozilla code, with the unfortunate exceptions of NSPR and NSS at present. (Maybe, hopefully we can fix that at some point.) The macros defined there are:

* MOZ_ASSERT(expr)
* MOZ_ASSERT_IF(ifexpr, expr) (assert expr if ifexpr)
* MOZ_ALWAYS_TRUE(expr) (evaluate expr in debug and release, assert the result)
* MOZ_ALWAYS_FALSE(expr) (you figure it out)
* MOZ_NOT_REACHED(reason) (mark control flow paths which will never execute, e.g. defaults in under-full switches)
* MOZ_STATIC_ASSERT(expr, reason) -- assertions depending on compile-time constants

More details in this blog post:

http://whereswalden.com/2011/12/26/introducing-mozillaassertions-h-to-mfbt/

And here's Assertions.h:

http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h

One question: MOZ_ASSERT takes a single argument, the expression to test. For ease in transitioning from the existing assertions, and to better explain non-obvious assertions, should we add an optional second argument, a string literal reason for the assertion? Note that we'd have to semi-disable assertions in compilers not implementing variadic macros to do it. MSVC 2005 supports variadic macros, so I don't think we care about any such compilers, but I could be mistaken. Thoughts appreciated.

Jeff

[Brian Smith]

Please excuse me for being annoying, but...

I am very curious why we need new MOZ_* names for existing NS_* functionality, especially if we do the variadic macro thing for MOZ/NS_ASSERTION so that the string argument to NS_ASSERTION would become optional? Adding a sprinkling of new MOZ_* things that are the same as NS_* things--except maybe a little different sometimes--seems counter to the goals of making the code easier to understand. Or are you planning to rewrite (soon) all the uses of NS_* to the MOZ_* equivalents?

> * MOZ_STATIC_ASSERT(expr, reason) -- assertions depending on
> compile-time constants

Considering the work done to guarantee the availability of stdint *_t types, and also considering the switch from PR_Bool to bool, why not just call this static_assert, and define it as a macro when static_assert isn't available?

Also, I saw that you use JS_Assert instead of PR_Assert, presumably because PR_Assert doesn't work well all the time on Windows as it uses DebugBreak. Should we fix PR_Assert so that it works like JS_Assert, or change both PR_Assert and JS_Assert to call the better __debugbreak() instead [1], and then use PR_Assert instead? That would be better than making everything depend on Spidermonkey.

Cheers,
brian PR_BRIAN PORT_Brian NS_BRIANION JS_BRIAN MOZ_BRIAN Smith

[1] http://msdn.microsoft.com/en-us/library/ea9yy3ey.aspx

[Bobby Holley]

On Mon, Dec 26, 2011 at 10:14 PM, Brian Smith <bsm...@mozilla.com> wrote:

> I am very curious why we need new MOZ_* names for existing NS_*
> functionality, especially if we do the variadic macro thing for
> MOZ/NS_ASSERTION so that the string argument to NS_ASSERTION would become
> optional? Adding a sprinkling of new MOZ_* things that are the same as NS_*
> things--except maybe a little different sometimes--seems counter to the
> goals of making the code easier to understand.

Outside the JS engine, the only mechanism I know of for fatal assertions is
NS_ABORT_IF_FALSE(expr, str). This is a terrible name, and I always find
myself doing mental gymnastics to get the predicate right.

NS_ASSERTION can't be made fatal for obvious reasons, so we decided to
switch to NS_ABORT_IF_FALSE for new assertions [1]. We've only been doing
this for a few years, so I think there aren't so many of them. In light of
this, I think the switch to MOZ_ASSERT is great for understandability.

> Or are you planning to rewrite (soon) all the uses of NS_* to the MOZ_*
> equivalents?
>

I think this would be trivial to do for NS_ABORT_IF_FALSE -> MOZ_ASSERT,
once we figure out what to do about |str|. Instances of NS_ASSERTION need
to be gradually removed over time (since they're non-fatal and we hit them
quite often), but that issue is orthogonal to this one.

-bholley

[1] http://www.squarefree.com/2008/07/17/abort/ ,
http://whereswalden.com/2009/01/04/a-reminder-ns_abort_if_false-is-the-new-ns_assertion/

[Jeff Walden]

On 12/27/2011 12:14 AM, Brian Smith wrote:
> I am very curious why we need new MOZ_* names for existing NS_* functionality,
> especially if we do the variadic macro thing for MOZ/NS_ASSERTION so that the
> string argument to NS_ASSERTION would become optional? Adding a sprinkling of
> new MOZ_* things that are the same as NS_* things--except maybe a little
> different sometimes--seems counter to the goals of making the code easier to
> understand. Or are you planning to rewrite (soon) all the uses of NS_* to the
> MOZ_* equivalents?

The goal is to rewrite the others, over time. NS_ABORT_IF_FALSE is easily and quickly rewritten, being identically fatal; we should do this. NS_ASSERTION, being non-fatal and ignorable (pace Robert O'Callahan), would be changed incrementally as existing code becomes better-understood such that we have confidence in those assertions holding.

But most immediately, the goal was to get these macros out of the JS engine (where they previously lived) into core Mozilla code, so there's one thing everyone can use that eventually won't require the JS engine as well. (The crash reporter binary may be one such beast.) This also makes it easier to fix bugs in assertion support -- fewer places to change. See e.g. bug 381236, which had to be fixed in multiple instances in the tree (and which someone had to know to do, and point it out, for it to happen).

https://bugzilla.mozilla.org/show_bug.cgi?id=381236#c15

I also think we benefit greatly from having One Way To Perform An Assertion. New contributors shouldn't have to ask how to perform assertions in each module they touch. They don't in WebKit -- it's just ASSERT everywhere. (Port-specific code may differ, but that's much more third-party than any non-imported code we have.) And I can't see the value in having custom ways to say, "if this condition wasn't true, throw up a debugger".

> why not just call this static_assert, and define it as a macro when
> static_assert isn't available?

static_assert's a C++11 keyword and a new C11 (next-gen C) <assert.h> macro. Not defining it in C++11 compilers, and tiptoeing around <assert.h>'s potential definition, seems pretty messy. (Especially given compiler support won't advance uniformly.) It seems easiest to sidestep the complexity entirely.

> Also, I saw that you use JS_Assert instead of PR_Assert, presumably
> because PR_Assert doesn't work well all the time on Windows as it uses
> DebugBreak.

MOZ_ASSERT predates my landings by quite a bit; it just lived in Util.h, which was/is a bit grab-baggy. It's always depended on JS_Assert. I didn't change that.

> Should we fix PR_Assert so that it works like JS_Assert, or change
> both PR_Assert and JS_Assert to call the better __debugbreak()
> instead [1], and then use PR_Assert instead?

I'm not too picky about any of the various ways to crash being better than the others, so long as they throw up a debugger and don't mess up Breakpad, which I thought they all did competently. I'd leave what we have alone and not trouble trouble, but if you want to convince someone who understands these win32 nuances, go ahead. (The comment suggests JS may have originally done what NSPR did, then encountered the DebugBreak issue, then fixed JS without fixing NSPR. More evidence for fewer implementations == better.)

http://hg.mozilla.org/mozilla-central/file/6f4f2e53694b/js/src/jsutil.cpp#l83

SpiderMonkey doesn't depend on NSPR in some configurations, so it's not possible to use PR_Assert. For NSPR being very ancient and hard-to-read C, and for it being a true third-party import that's pretty hard to get changed (and don't forget system-nspr :-\ ), I think we'd rather not depend on it when possible.

> That would be better than making everything depend on Spidermonkey.

Someday we'll just create a micro-library for mfbt and have JS/Gecko link against that (in whatever the most efficient way is -- I really don't know much about linking) and have mfbt not depend on JS at all. I'm no build-system person to know how to straightforwardly implement this, plus I'm juggling infrequent mfbt hacking with JS responsibilities. And it's arguably not super-important, except as an implementation detail. The main goal for me was to move JS-specific functionality out of JS so that it's shared by more code, benefiting more than just SpiderMonkey.

> Cheers,
> brian PR_BRIAN PORT_Brian NS_BRIANION JS_BRIAN MOZ_BRIAN Smith

Har. The point of this is to get rid of some of those redundant implementations, believe it or not. And the MOZ_* prefix mostly indicates that something's newer and better thought out than the old NS_* stuff. I refer again to the XPCOM_DebugBreak complexity, and to XPCOM more generally. But this goal is not immediate: it'll take time.

Jeff

[Mike Hommey]

On Tue, Dec 27, 2011 at 01:39:08AM -0600, Jeff Walden wrote:
> >That would be better than making everything depend on Spidermonkey.
>
> Someday we'll just create a micro-library for mfbt and have JS/Gecko link against that (in whatever the most efficient way is -- I really don't know much about linking) and have mfbt not depend on JS at all. I'm no build-system person to know how to straightforwardly implement this, plus I'm juggling infrequent mfbt hacking with JS responsibilities. And it's arguably not super-important, except as an implementation detail. The main goal for me was to move JS-specific functionality out of JS so that it's shared by more code, benefiting more than just SpiderMonkey.

It seems to me the right place for this would be what currently is
mozutils (soon to be renamed mozglue and moved around). Adding
something there will be pretty straightforward.

Mike

[Neil]

Jeff Walden wrote:

> One question: MOZ_ASSERT takes a single argument, the expression to
> test. For ease in transitioning from the existing assertions, and to
> better explain non-obvious assertions, should we add an optional
> second argument, a string literal reason for the assertion?

Does MOZ_ASSERT_IF(!expr, !"reason") work?

--
Warning: May contain traces of nuts.

[Brian Smith]

Jeff Walden wrote:
> I also think we benefit greatly from having One Way To Perform An
> Assertion. New contributors shouldn't have to ask how to perform
> assertions in each module they touch. They don't in WebKit -- it's
> just ASSERT everywhere. (Port-specific code may differ, but that's
> much more third-party than any non-imported code we have.) And I can't
> see the value in having custom ways to say, "if this condition wasn't
> true, throw up a debugger".

I agree, which is why I think it must be PR_ASSERT, because I don't see NSPR or NSS (C libraries) changing to depend on mfbt (a C++ library), ever. (Not due to my preferences, but due to others').

> static_assert's a C++11 keyword and a new C11 (next-gen C) <assert.h>
> macro. Not defining it in C++11 compilers, and tiptoeing around
> <assert.h>'s potential definition, seems pretty messy. (Especially
> given compiler support won't advance uniformly.) It seems easiest to
> sidestep the complexity entirely.

Isn't this exactly the same kind of problem we had for stdint.h? And, isn't the solution the same or similar? Plus, why do we stab ourselves in the faces every day with the horribleness of autoconf, if not for this type of problem?

* For C++, you already do the messy work of detecting the availability of static_assert.
* In C11, static_assert is always a macro, so you can:

#if !defined(__cplusplus)
# include <assert.h>
# ifndef static_assert
# define static_assert ...
# endif
#else
# <do what you are already doing, but
# #define static_assert instead of MOZ_STATIC_ASSERT>
#endif

Also, I have found PR_STATIC_ASSERT's signature to be *MUCH* more convenient than static_assert's signature, for the same reason that MOZ_ASSERT's signature is usually more convenient than NS_ASSERTION: the second "reason" string parameter is usually unhelpful. Rewriting code that uses PR_STATIC_ASSERT to use MOZ_STATIC_ASSERT, as proposed in bug 712936, would often just make the code harder to read.

> someone who understands these win32 nuances, go ahead. (The comment
> suggests JS may have originally done what NSPR did, then encountered
> the DebugBreak issue, then fixed JS without fixing NSPR. More evidence
> for fewer implementations == better.)

Yes, this is my exactly my point.

> SpiderMonkey doesn't depend on NSPR in some configurations, so it's
> not possible to use PR_Assert.

Interesting, I did not know that. So we have the case where Spidermonkey can't use PR_Assert, and NSPR and NSS cannot use JS_Assert, even though they do the exact same thing.

Or, rather, they *should* be doing the same thing. I filed bug 713631 to improve the NSPR PR_Assert on Windows.

> Someday we'll just create a micro-library for mfbt and have JS/Gecko
> link against that (in whatever the most efficient way is -- I really
> don't know much about linking) and have mfbt not depend on JS at all.

We already have such a micro-library: libplc, which is part of NSPR, and which is only 22kb (on Windows). I think it would be good to move the parts of mfbt that also apply to C (e.g. static_assert, a good implementation of JS_Assert/PR_Assert, and the stdint support) to libplc, so that NSS and NSPR can benefit from them. Otherwise, work like this needs to be duplicated between mfbt and NSPR.

- Brian

[Mike Hommey]

On Tue, Dec 27, 2011 at 04:25:30AM -0800, Brian Smith wrote:
> > Someday we'll just create a micro-library for mfbt and have JS/Gecko
> > link against that (in whatever the most efficient way is -- I really
> > don't know much about linking) and have mfbt not depend on JS at all.
>

> We already have such a micro-library: libplc, which is part of NSPR, and which is only 22kb (on Windows). I think it would be good to move the parts of mfbt that also apply to C (e.g. static_assert, a good implementation of JS_Assert/PR_Assert, and the stdint support) to libplc, so that NSS and NSPR can benefit from them. Otherwise, work like this needs to be duplicated between mfbt and NSPR.

I don't agree here, mainly because I want to use mfbt in code that needs
not to depend on nspr. (BTW, your data is biased, PLC is small, sure,
but it also depends on NSPR)

Another thing is that NSPR is mainly third-party software, and we'll
have things duplicated between mozilla code and third-party code all
over the place anyways, not only with NSPR.

Mike

[Jeff Walden]

On 12/27/2011 04:49 AM, Neil wrote:
> Does MOZ_ASSERT_IF(!expr, !"reason") work?

I guess that might work as a hackaround. But honestly I find that bizarre to read and extremely difficult to parse and understand. Waaaay too many negatives...

Jeff

[Jeff Walden]

On 12/27/2011 06:25 AM, Brian Smith wrote:
> * For C++, you already do the messy work of detecting the availability of static_assert.

I *conservatively* detect its availability. You could remove the entire detection section from the current implementation and just do the fall-back-to-typedef/extern and it'd still work, I think. If you're detecting static_assert, tho, you have to get it right for every compiler from the start. And you have to get it right for compilers that haven't been released yet, or possibly don't even exist yet.

Fine, that's supposedly autoconf's purview. Except that it can't be, because C++11 is opt-in with many compilers, requiring -std=c++11 or similar. You can't autoconf your way to guessing what that compiler flag is, or will be.

Perhaps this all could be made to work even still. Given that most of what I wanted to do was move things out of the JS engine for broader use, it was tangential. And I'm still pretty sure it'd be a rathole to implement.

(stdint.h types' elaborate solution was necessary where static_assert's wasn't because that problem was significantly older. If all factions weren't pleased, it wouldn't happen. It *hadn't* happened. There was that much stop energy. In contrast, people have used PR_STATIC_ASSERT and MOZ_STATIC_ASSERT for a long time and been reasonably satisfied with it, so a solution along those lines remained feasible.)

> Also, I have found PR_STATIC_ASSERT's signature to be *MUCH* more convenient than static_assert's signature, for the same reason that MOZ_ASSERT's signature is usually more convenient than NS_ASSERTION: the second "reason" string parameter is usually unhelpful. Rewriting code that uses PR_STATIC_ASSERT to use MOZ_STATIC_ASSERT, as proposed in bug 712936, would often just make the code harder to read.

Originally M_S_A's reason string didn't exist in earlier patches. It was suggested to have it to make it consistent with static_assert from the start. There's something to say for that. At the time I was thinking maybe it made sense to maybe make it optional in future work. But I'm also a don't-rock-the-boat sort of person as far as minor decisions go, so I'm inclined to stick with requiring a reason. It does have the benefit of making people think about why it's there, and hopefully nudge them further toward supplying a decent explanation. And it makes a search-and-replace when static_assert is ubiquitous a bit easier, as a distant-future benefit.

> We already have such a micro-library: libplc, which is part of NSPR, and which is only 22kb (on Windows).

Honestly I think there's little love for NSPR, and there wouldn't be much love for chaining ourselves to it further.

> I think it would be good to move the parts of mfbt that also apply to C (e.g. static_assert, a good implementation of JS_Assert/PR_Assert, and the stdint support) to libplc, so that NSS and NSPR can benefit from them.

I'm not particularly a fan of an artificial separation here by C/C++ applicability. Really we'd be better off just changing the few bits of C code we have that aren't third-party imports to C++ and not having to think about the C/C++ distinction. Quick find/acking suggests there are fewer non-third-party .c files in the tree than there were in all SpiderMonkey before it made the jump internally (there's one API header that's C-compatible) a few years back -- about 200 now, and I'm pretty sure there were more in SpiderMonkey then, although I'm going off impressions to say that.

Jeff

[Jeff Walden]

On 12/27/2011 11:12 AM, Jeff Walden wrote:
> In contrast, people have used PR_STATIC_ASSERT and MOZ_STATIC_ASSERT for a long time

Er, PR_STATIC_ASSERT and JS_STATIC_ASSERT, of course.

Jeff

[Mike Hommey]

On Tue, Dec 27, 2011 at 11:12:43AM -0600, Jeff Walden wrote:
> On 12/27/2011 06:25 AM, Brian Smith wrote:
> >* For C++, you already do the messy work of detecting the availability of static_assert.
>
> I *conservatively* detect its availability. You could remove the entire detection section from the current implementation and just do the fall-back-to-typedef/extern and it'd still work, I think. If you're detecting static_assert, tho, you have to get it right for every compiler from the start. And you have to get it right for compilers that haven't been released yet, or possibly don't even exist yet.
>
> Fine, that's supposedly autoconf's purview. Except that it can't be, because C++11 is opt-in with many compilers, requiring -std=c++11 or similar. You can't autoconf your way to guessing what that compiler flag is, or will be.

Configure would just have to check if static_assert is supported with
whatever flags we're going to use for the build. Which means it will use
the opt-in flags if we're going to use them for the build (which we do
on linux builds, for instance).

However, as part of the SDK, you can't rely on variables coming out of
configure. At least, you can't rely on the fact that the build flags
used to build something against the SDK are the same as those used when
the SDK itself was built. Likewise for the compiler.
So all in all, it's good the way it is.

Mike

[Robert O'Callahan]

On Tue, Dec 27, 2011 at 8:39 PM, Jeff Walden <jwald...@mit.edu> wrote:

> NS_ASSERTION, being non-fatal and ignorable (pace Robert O'Callahan),
> would be changed incrementally as existing code becomes better-understood
> such that we have confidence in those assertions holding.
>

"pace" isn't a silver bullet :-).

I have written down why I think non-fatal assertions are valuable:
http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html
I'm opening to renaming them so they're not called "assertions" (providing
someone comes up with an adequate alternative). I think making them all
fatal would be a serious mistake, removing them an even worse mistake.

Rob
--
"If we claim to be without sin, we deceive ourselves and the truth is not
in us. If we confess our sins, he is faithful and just and will forgive us
our sins and purify us from all unrighteousness. If we claim we have not
sinned, we make him out to be a liar and his word is not in us." [1 John
1:8-10]

[Mike Hommey]

On Wed, Dec 28, 2011 at 02:31:54PM +1300, Robert O'Callahan wrote:
> On Tue, Dec 27, 2011 at 8:39 PM, Jeff Walden <jwald...@mit.edu> wrote:
>

> > NS_ASSERTION, being non-fatal and ignorable (pace Robert O'Callahan),
> > would be changed incrementally as existing code becomes better-understood
> > such that we have confidence in those assertions holding.
> >
>

> "pace" isn't a silver bullet :-).
>
> I have written down why I think non-fatal assertions are valuable:
> http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html
> I'm opening to renaming them so they're not called "assertions" (providing
> someone comes up with an adequate alternative).

"expectations"?

Mike

[Nicholas Nethercote]

On Wed, Dec 28, 2011 at 6:20 PM, Mike Hommey <m...@glandium.org> wrote:
>> I'm opening to renaming them so they're not called "assertions" (providing
>> someone comes up with an adequate alternative).
>
> "expectations"?

MOZ_WEAK_ASSERT ?

Nick

[Robert O'Callahan]

On Wed, Dec 28, 2011 at 8:20 PM, Mike Hommey <m...@glandium.org> wrote:

> "expectations"?
>

Hmm.

A fatal assertion should mean "this condition is expected to be true; if it
isn't, we have encountered a bug so severe we may as well crash now."

A non-fatal assertion should mean "this condition is expected to be true;
if it isn't, we have encountered a bug, but it's worth continuing."

There's no necessary difference in our confidence in the condition. The
difference is in our guess at the severity of the underlying bug and the
consequences of continuing execution.

[Robert O'Callahan]

On Wed, Dec 28, 2011 at 9:36 PM, Nicholas Nethercote <n.neth...@gmail.com
> wrote:

> MOZ_WEAK_ASSERT ?
>

Too vague.

In classical computer science, in the words of Wikipedia an assertion is
simply "a predicate placed in a program to indicate that the developer *
thinks* that the predicate is always true at that place". A checked
assertion that doesn't terminate the program is still definitely an
assertion, and I'd like to use the accurate term because I don't know of a
better one. So how about MOZ_NONFATAL_ASSERT? MOZ_BENIGN_ASSERT?

If not, maybe MOZ_CHECK.

[Zack Weinberg]

On 2011-12-27 8:39 AM, Jeff Walden wrote:

>> why not just call this static_assert, and define it as a macro when
>> static_assert isn't available?
>
> static_assert's a C++11 keyword and a new C11 (next-gen C) <assert.h>
> macro. Not defining it in C++11 compilers, and tiptoeing around
> <assert.h>'s potential definition, seems pretty messy. (Especially given
> compiler support won't advance uniformly.) It seems easiest to sidestep
> the complexity entirely.

Does C++11 forbid "#define static_assert" ? (I am not in a position to
check for myself, just now.)

If it doesn't, a conservative check would be fine, something like

#undef static_assert
#if /* attempt to detect C++11 static_assert support */
/* intrinsic is available, no macro definition required */
#elif /* attempt to detect C11 _Static_assert support */
#define static_assert(c, r) _Static_assert(c, r)
#else
#define static_assert(c, r) /* fallback construct */
#endif

> SpiderMonkey doesn't depend on NSPR in some configurations, so it's not
> possible to use PR_Assert. For NSPR being very ancient and hard-to-read
> C, and for it being a true third-party import that's pretty hard to get
> changed (and don't forget system-nspr :-\ ), I think we'd rather not
> depend on it when possible.

Remember also that some of us have "get rid of NSPR entirely" as a
long-term goal <.<

zw

[Ted Mielczarek]

On Wed, Dec 28, 2011 at 3:38 AM, Robert O'Callahan <rob...@ocallahan.org> wrote:
> On Wed, Dec 28, 2011 at 8:20 PM, Mike Hommey <m...@glandium.org> wrote:
>
>> "expectations"?
>>
>
> Hmm.
>
> A fatal assertion should mean "this condition is expected to be true; if it
> isn't, we have encountered a bug so severe we may as well crash now."
>
> A non-fatal assertion should mean "this condition is expected to be true;
> if it isn't, we have encountered a bug, but it's worth continuing."
>
> There's no necessary difference in our confidence in the condition. The
> difference is in our guess at the severity of the underlying bug and the
> consequences of continuing execution.

I don't know that I see the point in adding something like this back.
We already have NS_ASSERTION, and it's practically useless. Our unit
tests hit multiple assertions, and I'm sure our users would too if
they were running debug builds. If the assertions aren't fatal, then
they're unlikely to ever get fixed. In which case, why bother having
them?

In addition, if you have fatal assertions, then you can use them as
your error handling for disastrous conditions (because you know
program execution will terminate at the assertion). If you put in
non-fatal assertions, then you still need an error handling branch
anyway, so it doesn't really buy you much.

-Ted

[L. David Baron]

On Wednesday 2011-12-28 07:53 -0500, Ted Mielczarek wrote:
> On Wed, Dec 28, 2011 at 3:38 AM, Robert O'Callahan <rob...@ocallahan.org> wrote:
> > On Wed, Dec 28, 2011 at 8:20 PM, Mike Hommey <m...@glandium.org> wrote:
> >
> >> "expectations"?
> >>
> >
> > Hmm.
> >
> > A fatal assertion should mean "this condition is expected to be true; if it
> > isn't, we have encountered a bug so severe we may as well crash now."
> >
> > A non-fatal assertion should mean "this condition is expected to be true;
> > if it isn't, we have encountered a bug, but it's worth continuing."
> >
> > There's no necessary difference in our confidence in the condition. The
> > difference is in our guess at the severity of the underlying bug and the
> > consequences of continuing execution.
>
> I don't know that I see the point in adding something like this back.
> We already have NS_ASSERTION, and it's practically useless. Our unit
> tests hit multiple assertions, and I'm sure our users would too if
> they were running debug builds. If the assertions aren't fatal, then
> they're unlikely to ever get fixed. In which case, why bother having
> them?

They're actually quite useful in reftest, since reftest reports an
unexpected failure (but continues) if tests cause unexpected
assertions.

I've been planning to do the same for mochitest, and have a patch
written, but haven't had the time to debug the issues with the
patch. (There used to be issues with mochitest-browser-chrome (?)
that have now gone away, but there are new issues with
mochitest-a11y.) This work is
https://bugzilla.mozilla.org/show_bug.cgi?id=404077

> In addition, if you have fatal assertions, then you can use them as
> your error handling for disastrous conditions (because you know
> program execution will terminate at the assertion). If you put in

Not really, since they're debug-only. And we rely on that sometimes
and put somewhat expensive things in assertions.

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla http://www.mozilla.org/ 𝄂

[Chris Jones]

----- Original Message -----
> From: "Robert O'Callahan" <rob...@ocallahan.org>
> To: "Nicholas Nethercote" <n.neth...@gmail.com>
> Cc: "Mike Hommey" <m...@glandium.org>, dev-pl...@lists.mozilla.org, "Jeff Walden" <jwald...@mit.edu>
> Sent: Wednesday, December 28, 2011 12:56:33 AM
> Subject: Re: Performing assertions in Mozilla code: use #include "mozilla/Assertions.h" to do it
> On Wed, Dec 28, 2011 at 9:36 PM, Nicholas Nethercote
> <n.neth...@gmail.com
> > wrote:
>
> In classical computer science, in the words of Wikipedia an assertion
> is
> simply "a predicate placed in a program to indicate that the developer
> *
> thinks* that the predicate is always true at that place". A checked
> assertion that doesn't terminate the program is still definitely an
> assertion, and I'd like to use the accurate term because I don't know
> of a
> better one. So how about MOZ_NONFATAL_ASSERT? MOZ_BENIGN_ASSERT?
>
> If not, maybe MOZ_CHECK.
>

MOZ_CHECK seems reasonable to me.

In model checking, "assert" is usually used to declare a condition that *must* be true of program state, usually something that the program has computed. But there's a companion statement, "assume", which means "if this condition doesn't hold of the current state, the program states resulting from it aren't worth exploring". "assume" is usually used on inputs to computations. (This is oversimplifying things.)

I'm not terribly familiar with the current uses of NS_ASSERTION because I try to avoid it, but if they're semantically closer to "assume"s, a MOZ_ASSUME helper might make sense. I would expect a failed MOZ_ASSUME to be non-fatal and say something like, "assumptions %s are invalid, unknown program state will result". This would still be worth filing a bug on, since it likely points at a bug in the module providing inputs to the MOZ_ASSUME.

But folks here may really advocating for a weak assert over computed state ("my assumptions were valid, but my computation was wrong, though maybe not fatally/exploitably wrong"), in which case something like MOZ_CHECK makes sense, orthogonal to a MOZ_ASSUME.

Cheers,
Chris

[Jeff Walden]

On 12/28/2011 06:50 AM, Zack Weinberg wrote:
> Does C++11 forbid "#define static_assert" ? (I am not in a position to check for myself, just now.)

Redefining any keyword induces undefined behavior in both C and C++, so the answer is yes.

>> SpiderMonkey doesn't depend on NSPR in some configurations, so it's not
>> possible to use PR_Assert. For NSPR being very ancient and hard-to-read
>> C, and for it being a true third-party import that's pretty hard to get
>> changed (and don't forget system-nspr :-\ ), I think we'd rather not
>> depend on it when possible.
>
> Remember also that some of us have "get rid of NSPR entirely" as a long-term goal <.<

I was being polite. :-) I'd love to see that happen, myself, although I'm not sure how we'll get there.

Jeff

[Ehsan Akhgari]

NS_ASSERTION means "I expect this predicate to be true here, but if it's
not, it's OK to continue the execution of the program". C's assert() (and
our variants of it) mean "I expect this predicate to be true here, and if
it's not, the program is in an undefined state and should be terminated."
Therefore, I don't see a big similarity between these macros and the model
checking assert/assume concepts.

I'm ambivalent as to which name should be chosen. I think the biggest
chunk of the problem is what programmers familiar with C's assert() think
when they see any name containing "assert"/"assertion". So if we would
have to pick a new name, I'd vote for MOZ_NONFATAL_ASSERT. Although I have
to say that so far I haven't seen a very compelling reason for renaming
MOZ_ASSERTION, and MOZ_NONFATAL_ASSERT is definitely less readable and
harder to type.

Cheers,
Ehsan

[Chris Jones]

----- Original Message -----
> From: "Ehsan Akhgari" <ehsan....@gmail.com>
> To: "Chris Jones" <cjo...@mozilla.com>
> Cc: rob...@ocallahan.org, "Nicholas Nethercote" <n.neth...@gmail.com>, "Mike Hommey" <m...@glandium.org>,
> dev-pl...@lists.mozilla.org, "Jeff Walden" <jwald...@mit.edu>
> Sent: Wednesday, December 28, 2011 10:15:24 AM
> Subject: Re: Performing assertions in Mozilla code: use #include "mozilla/Assertions.h" to do it
> On Wed, Dec 28, 2011 at 12:04 PM, Chris Jones < cjo...@mozilla.com >
> wrote:
>
> NS_ASSERTION means "I expect this predicate to be true here, but if
> it's not, it's OK to continue the execution of the program". C's
> assert() (and our variants of it) mean "I expect this predicate to be
> true here, and if it's not, the program is in an undefined state and
> should be terminated." Therefore, I don't see a big similarity between
> these macros and the model checking assert/assume concepts.
>

I was asking about how NS_ASSERTION tends to be used in practice. (I don't know.)

> I'm ambivalent as to which name should be chosen. I think the biggest
> chunk of the problem is what programmers familiar with C's assert()
> think when they see any name containing "assert"/"assertion". So if we
> would have to pick a new name, I'd vote for MOZ_NONFATAL_ASSERT.
> Although I have to say that so far I haven't seen a very compelling
> reason for renaming MOZ_ASSERTION, and MOZ_NONFATAL_ASSERT is
> definitely less readable and harder to type.
>

MOZ_ASSERTION doesn't exist, do you mean NS_ASSERTION? NS_ASSERTION is a bad name because it's not a verb, and it's confusing semantically wrt C assert(). I think the addition of MOZ_ASSERT with C assert() semantics to fix existing confusion and historical baggage with NS_ASSERTION/NS_ABORT_IF_FALSE is fairly non-controversial.

I think there are two remaining questions
- Are we generally using NS_ASSERTION to mean "assume" in the model checking sense?
- If generally not, what's a good name for a nonfatal check of program state. I agree that MOZ_NONFATAL_ASSERT is not a great name, but I vote for MOZ_CHECK instead of adding a MOZ_ASSERTION with also a not-great name and the old confusion around NS_ASSERTION. The downside to that is existing gecko hackers will need to learn MOZ_CHECK.

Cheers,
Chris

[Ehsan Akhgari]

On Wed, Dec 28, 2011 at 1:40 PM, Chris Jones <cjo...@mozilla.com> wrote:

> ----- Original Message -----
> > From: "Ehsan Akhgari" <ehsan....@gmail.com>
> > To: "Chris Jones" <cjo...@mozilla.com>
> > Cc: rob...@ocallahan.org, "Nicholas Nethercote" <n.neth...@gmail.com>,
> "Mike Hommey" <m...@glandium.org>,
> > dev-pl...@lists.mozilla.org, "Jeff Walden" <jwald...@mit.edu>
> > Sent: Wednesday, December 28, 2011 10:15:24 AM
> > Subject: Re: Performing assertions in Mozilla code: use #include
> "mozilla/Assertions.h" to do it
> > On Wed, Dec 28, 2011 at 12:04 PM, Chris Jones < cjo...@mozilla.com >
> > wrote:
> >
> > NS_ASSERTION means "I expect this predicate to be true here, but if
> > it's not, it's OK to continue the execution of the program". C's
> > assert() (and our variants of it) mean "I expect this predicate to be
> > true here, and if it's not, the program is in an undefined state and
> > should be terminated." Therefore, I don't see a big similarity between
> > these macros and the model checking assert/assume concepts.
> >
>
> I was asking about how NS_ASSERTION tends to be used in practice. (I
> don't know.)
>

Yes, that is what I was talking about.

> > I'm ambivalent as to which name should be chosen. I think the biggest
> > chunk of the problem is what programmers familiar with C's assert()
> > think when they see any name containing "assert"/"assertion". So if we
> > would have to pick a new name, I'd vote for MOZ_NONFATAL_ASSERT.
> > Although I have to say that so far I haven't seen a very compelling
> > reason for renaming MOZ_ASSERTION, and MOZ_NONFATAL_ASSERT is
> > definitely less readable and harder to type.
> >
>
> MOZ_ASSERTION doesn't exist, do you mean NS_ASSERTION?

Argh, yes!

> NS_ASSERTION is a bad name because it's not a verb, and it's confusing
> semantically wrt C assert(). I think the addition of MOZ_ASSERT with C
> assert() semantics to fix existing confusion and historical baggage with
> NS_ASSERTION/NS_ABORT_IF_FALSE is fairly non-controversial.
>

We do have other macros with names in their names as opposed to verbs
(examples: NS_PRECONDITION, NS_ERROR, NS_WARNING, etc).

I agree completely with the confusion wrt C assert() piece, and I think
that MOZ_ASSERT is useful for people used to C's assert().

> I think there are two remaining questions
> - Are we generally using NS_ASSERTION to mean "assume" in the model
> checking sense?
>

Not prevalently.

> - If generally not, what's a good name for a nonfatal check of program
> state. I agree that MOZ_NONFATAL_ASSERT is not a great name, but I vote
> for MOZ_CHECK instead of adding a MOZ_ASSERTION with also a not-great name
> and the old confusion around NS_ASSERTION. The downside to that is
> existing gecko hackers will need to learn MOZ_CHECK.
>

Leaving the fatality issue aside for a second, I think NS_ASSERTION is a
better name than MOZ_CHECK, as it actually says what the macro is supposed
to be used for. As roc mentioned, there is nothing weaker about a
non-fatal assertion compared to a fatal assertion, where as "checking"
something is weaker than "asserting" its truthfulness.

The reason that I prefer MOZ_NONFATAL_ASSERT is that it keeps the sense in
which the macro is to be used, but also makes people used to C's assert()
happy.

Cheers,
--
Ehsan
<http://ehsanakhgari.org/>

[Chris Jones]

----- Original Message -----
> From: "Ehsan Akhgari" <ehsan....@gmail.com>
> To: "Chris Jones" <cjo...@mozilla.com>
> Cc: rob...@ocallahan.org, "Nicholas Nethercote" <n.neth...@gmail.com>, "Mike Hommey" <m...@glandium.org>,
> dev-pl...@lists.mozilla.org, "Jeff Walden" <jwald...@mit.edu>
> Sent: Wednesday, December 28, 2011 10:56:11 AM
> Subject: Re: Performing assertions in Mozilla code: use #include "mozilla/Assertions.h" to do it
> On Wed, Dec 28, 2011 at 1:40 PM, Chris Jones < cjo...@mozilla.com >
> wrote:
>
> NS_ASSERTION is a bad name because it's not a verb, and it's confusing
> semantically wrt C assert(). I think the addition of MOZ_ASSERT with C
> assert() semantics to fix existing confusion and historical baggage
> with NS_ASSERTION/NS_ABORT_IF_FALSE is fairly non-controversial.
>
>
> We do have other macros with names in their names as opposed to verbs
> (examples: NS_PRECONDITION, NS_ERROR, NS_WARNING, etc).
>

Those are also bad names ;).

>
> - If generally not, what's a good name for a nonfatal check of program
> state. I agree that MOZ_NONFATAL_ASSERT is not a great name, but I
> vote for MOZ_CHECK instead of adding a MOZ_ASSERTION with also a
> not-great name and the old confusion around NS_ASSERTION. The downside
> to that is existing gecko hackers will need to learn MOZ_CHECK.
>
> Leaving the fatality issue aside for a second, I think NS_ASSERTION is
> a better name than MOZ_CHECK, as it actually says what the macro is
> supposed to be used for. As roc mentioned, there is nothing weaker
> about a non-fatal assertion compared to a fatal assertion, where as
> "checking" something is weaker than "asserting" its truthfulness.
>

IMHO having both a MOZ_ASSERT and an NS_ASSERTION is the worst outcome for developer sanity.

I think you and roc are arguing from the English-language semantics of the word "assert", and you're correct. But in the programming-language usage (at least in my experience), "assert" really does have the strong meaning: there's no point in continuing execution if the condition on the program state doesn't hold. We really are discussing something weaker, which boils down to a warning about unexpected program state. Since we're inventing a new weaker concept (at wrt my background), importing a weaker verb from English seems perfectly fine. Why is it helpful to gecko hackers to overload the traditional PL meaning of "assert"?

> The reason that I prefer MOZ_NONFATAL_ASSERT is that it keeps the
> sense in which the macro is to be used, but also makes people used to
> C's assert() happy.
>

Between NS_ASSERTION and MOZ_NONFATAL_ASSERT, I prefer MOZ_NONFATAL_ASSERT. But per above, I would still prefer MOZ_CHECK, or something similar that doesn't overload "assert".

Cheers,
Chris

[Zack Weinberg]

On 2011-12-28 6:09 PM, Jeff Walden wrote:
> On 12/28/2011 06:50 AM, Zack Weinberg wrote:
>> Does C++11 forbid "#define static_assert" ? (I am not in a position to
>> check for myself, just now.)
>
> Redefining any keyword induces undefined behavior in both C and C++, so
> the answer is yes.

Are you certain? I, um, wrote a C preprocessor once, and at the time,
'#define keyword whatever' was totally legit for *most* keywords in both
C and C++; the only exceptions I can remember are 'defined' and
'__VA_ARGS__', on account of their being *preprocessor* keywords, and
the C++-only keywords that are defined by <iso646.h> in C, for no
apparent reason.

C11/C++11 may well have changed this, and I would look it up for myself
if I had access to the computer that has my copy of those, but I don't
until I get back to the USA, so would you mind finding the standardese?

>> Remember also that some of us have "get rid of NSPR entirely" as a
>> long-term goal <.<
>
> I was being polite. :-) I'd love to see that happen, myself, although
> I'm not sure how we'll get there.

It seems to me that we can get to "only used via NSS" pretty easily, by
adding stuff to mfbt as necessary and/or converting to standard library
features. Replacing NSS is of course going to be a lot harder.

zw

[Jeff Walden]

On 12/28/2011 02:14 PM, Zack Weinberg wrote:
> Are you certain? I, um, wrote a C preprocessor once, and at the time, '#define keyword whatever' was totally legit for *most* keywords in both C and C++; the only exceptions I can remember are 'defined' and '__VA_ARGS__', on account of their being *preprocessor* keywords, and the C++-only keywords that are defined by <iso646.h> in C, for no apparent reason.

C++98 17.4.3.1.1 [lib.macro.names]p2:

> A translation unit that includes a [C++ Standard Library] header shall not
> contain any macros that define names declared or defined in that header.
> Nor shall such a translation unit define macros for names lexically
> identical to keywords.

C++11 N3242 [macro.names]p2:

> A translation unit [again, using the C++ STL] shall not #define or #undef
> names lexically identical to keywords.

The requirements apply to programs which use the C++ STL. I'm not sure why it's not just applicable to every program. But as a practical matter it might as well be, since I'd expect most of our files (or at least a non-negligible portion) include <new>. And various gfx code includes <bitset>, and some of the IPC headers include <utility> and <map> (? on the latter). So as a practical matter, you can't define a keyword.

I might be mistaken about C89/C99/C11; I can't find similar language in C99 N1124 or C11 N1570.

>>> Remember also that some of us have "get rid of NSPR entirely" as a
>>> long-term goal <.<
>>
>> I was being polite. :-) I'd love to see that happen, myself, although
>> I'm not sure how we'll get there.
>
> It seems to me that we can get to "only used via NSS" pretty easily, by adding stuff to mfbt as necessary and/or converting to standard library features.

Yes. Technically it's "easy". As a practical matter it'll take awhile, with a fair bit of code to rewrite/reimplement in terms of a new API. And then there's the time to design the new API *right*. Easy? Yes. And no.

> Replacing NSS is of course going to be a lot harder.

Yes, again.

Jeff

[Nicholas Nethercote]

On Wed, Dec 28, 2011 at 11:43 AM, Chris Jones <cjo...@mozilla.com> wrote:
>
>> The reason that I prefer MOZ_NONFATAL_ASSERT is that it keeps the
>> sense in which the macro is to be used, but also makes people used to
>> C's assert() happy.
>
> Between NS_ASSERTION and MOZ_NONFATAL_ASSERT, I prefer MOZ_NONFATAL_ASSERT.  But per above, I would still prefer MOZ_CHECK, or something similar that doesn't overload "assert".

I like MOZ_NONFATAL_ASSERT. It's a bit unwieldy compared to
MOZ_ASSERT. But then, while I think having both non-fatal and fatal
assertions is reasonable, I suspect we should be using more fatal
assertions and fewer non-fatal assertions. So the unwieldiness might
help push things in that direction.

I don't like MOZ_CHECK at all! Much too vague.

Nick

[Robert O'Callahan]

On Thu, Dec 29, 2011 at 8:43 AM, Chris Jones <cjo...@mozilla.com> wrote:

> I think you and roc are arguing from the English-language semantics of the
> word "assert", and you're correct.

As I said before, I'm arguing from the classic computer science term.

> Why is it helpful to gecko hackers to overload the traditional PL meaning
> of "assert"?
>

The "traditional" PL meaning of assert goes at least as far back as Hoare's
paper "An Axiomatic Basis For Computer Programming" in 1969, when it meant,
as I said before, nothing more than "a predicate placed in a program to
indicate that the developer thinks that the predicate is always true at
that place." In Hoare logic assertions aren't even checked at runtime, let
alone fatal; they're just there to help you reason about the program.

I think we're converging on MOZ_NONFATAL_ASSERT. It sounds OK to me.

[Robert O'Callahan]

On Thu, Dec 29, 2011 at 1:53 AM, Ted Mielczarek <t...@mielczarek.org> wrote:

> If the assertions aren't fatal, then they're unlikely to ever get fixed.
>

Reftests go orange when new assertions fire, and that gets fixed.

[Robert O'Callahan]

On Thu, Dec 29, 2011 at 3:48 AM, L. David Baron <dba...@dbaron.org> wrote:

> I've been planning to do the same for mochitest, and have a patch
> written, but haven't had the time to debug the issues with the
> patch. (There used to be issues with mochitest-browser-chrome (?)
> that have now gone away, but there are new issues with
> mochitest-a11y.) This work is
> https://bugzilla.mozilla.org/show_bug.cgi?id=404077
>

Someone needs to finish that. It would be a nice big increase in regression
test coverage.

Ironic that that was blocked for a while on bug 631289, a reproducible
fatal JS_ASSERT that was never fixed.

[Brian Smith]

Zack Weinberg wrote:
> >> Remember also that some of us have "get rid of NSPR entirely" as a
> >> long-term goal <.<
> >
> > I was being polite. :-) I'd love to see that happen, myself,
> > although I'm not sure how we'll get there.

What is the motivation for eliminating the NSPR dependency in Gecko, assuming that we continue to depend on NSS and NSS continues to depend on NSPR?

> It seems to me that we can get to "only used via NSS" pretty easily,
> by adding stuff to mfbt as necessary and/or converting to standard
> library features.

Removing NSPR must be the means to some end. Again, what is that end?

Cheers,
Brian

[Brian Smith]

Mike Hommey wrote:

> On Tue, Dec 27, 2011 at 04:25:30AM -0800, Brian Smith wrote:
> > We already have such a micro-library: libplc, which is part of NSPR,

> > and which is only 22kb (on Windows). I think it would be good to

> > move the parts of mfbt that also apply to C (e.g. static_assert, a
> > good implementation of JS_Assert/PR_Assert, and the stdint support)

> > to libplc, so that NSS and NSPR can benefit from them. Otherwise,
> > work like this needs to be duplicated between mfbt and NSPR.
>
> I don't agree here, mainly because I want to use mfbt in code that
> needs not to depend on nspr.

What drives the "must not depend on NSPR" requirement? Is it a compile-time requirement or a runtime requirement?

> (BTW, your data is biased, PLC is small, sure,
> but it also depends on NSPR)

My bad; I thought the dependency was the other way. PLC is not the place for such a function then.

I am pretty apathetic to the existence of NSPR. It didn't call my mother any names and it never lent me money when I needed it. I think it is important to make our "crash in a way that we can debug at the point of the crash" functions correct so that we can effectively debug our software. It is appealing to me to have one such function and to put it in NSPR because then we can use it in NSPR and NSS, and we don't have to keep them in sync. It wouldn't be particularly hard problem to keep two implementations in sync (one in Gecko and one in NSPR), but it isn't encouraging that they're out of sync from the start.

- Brian

[Boris Zbarsky]

On 12/28/11 7:42 PM, Brian Smith wrote:
> It is appealing to me to have one such function and to put it in NSPR because then we can use it in NSPR and NSS, and we don't have to keep them in sync.

But then we can't use it in the JS engine.

So as long as NSPR is not willing to have a dependency on something
external to itself that the rest of Gecko can also depend on, we have
two options:

1) Have two functions, one for use by NSPR, NSS, and Gecko and one for
use by JS.

2) Have two functions, one for use by Gecko and JS and one for NSS and
NSPR.

Since we can sanely control what happens with Gecko and JS, but not
necessarily NSS and NSPR, option 2 (which is what Jeff implemented)
seems like the better one.

-Boris

[Jeff Walden]

This is all tangential, but it seems worth stating it at least once, so it's down in writing.

On 12/28/2011 06:23 PM, Brian Smith wrote:
> Removing NSPR must be the means to some end. Again, what is that end?

Generally I think some of the reasons people dislike NSPR are:

- it's painful making fixes to it, because
- its code is inscrutable beneath layers of over-abstracting macros for lots of platform-specific differences
- it's C usually without C++ conveniences like RAII, automatic destruction, true virtual methods, and so on, both in the API it exposes (with rare exceptions) and in its implementation
- since it exposes many structures through pointers (PRLock* and such), using such things means extra memory use/fragmentation over simply embedding lock data directly in the relevant function
- it duplicates a lot of standard library stuff with its own new API to learn
- the system-nspr problem for depending on new stuff one might want to add to it
- it doesn't always integrate cleanly with Gecko's event loop (although this is a hard problem generally, one which a non-NSPR API would find somewhat tricky too)

I'm sure other reasons could be added.

These are not all actual functionality deficiencies, although sometimes they are. Code cleanliness often doesn't matter as much as actual features. But it does matter. (And sometimes, I would argue, more than actual features, when that cleanliness makes such features much easier to implement.)

None of this addresses the problem of NSS. The usual logic here would be, "One problem at a time, one step at a time."

Jeff

[Chris Jones]

----- Original Message -----
> From: "Robert O'Callahan" <rob...@ocallahan.org>
> To: "Chris Jones" <cjo...@mozilla.com>
> Cc: "Ehsan Akhgari" <ehsan....@gmail.com>, "Nicholas Nethercote" <n.neth...@gmail.com>, "Mike Hommey"
> <m...@glandium.org>, dev-pl...@lists.mozilla.org, "Jeff Walden" <jwald...@mit.edu>
> Sent: Wednesday, December 28, 2011 3:46:59 PM
> Subject: Re: Performing assertions in Mozilla code: use #include "mozilla/Assertions.h" to do it

> On Thu, Dec 29, 2011 at 8:43 AM, Chris Jones < cjo...@mozilla.com >
> wrote:
>
> Why is it helpful to gecko hackers to overload the traditional PL
> meaning of "assert"?
>
> The "traditional" PL meaning of assert goes at least as far back as
> Hoare's paper "An Axiomatic Basis For Computer Programming" in 1969,
> when it meant, as I said before, nothing more than "a predicate placed
> in a program to indicate that the developer thinks that the predicate
> is always true at that place." In Hoare logic assertions aren't even
> checked at runtime, let alone fatal; they're just there to help you
> reason about the program.
>

Don't want to derail too much more, but formulas in Hoare logic are either true or not. So if your assertions in Hoare logic don't "hold", your proof/program is wrong. Or maybe I'm missing something?

Assertion statements, in programming languages, have had the side effect of terminating program execution when the asserted condition is false since at least Algol W (1972).

> I think we're converging on MOZ_NONFATAL_ASSERT. It sounds OK to me.
>

The grammar fascist in me just remembered that this should be MOZ_NONFATALLY_ASSERT.

Cheers,
Chris

[Robert O'Callahan]

On Thu, Dec 29, 2011 at 5:37 PM, Chris Jones <cjo...@mozilla.com> wrote:

> Don't want to derail too much more, but formulas in Hoare logic are either
> true or not. So if your assertions in Hoare logic don't "hold", your
> proof/program is wrong. Or maybe I'm missing something?
>

You're not missing anything. That is consistent with the usage of
NS_ASSERTION; when the condition is false, the program has a bug.

[Brian Smith]

Jeff Walden wrote:
> - its code is inscrutable beneath layers of over-abstracting macros
> for lots of platform-specific differences

> - it duplicates a lot of standard library stuff with its own new API
> to learn

This is exactly my criticism of mfbt. "#include <mozilla/StdInt.h>" vs "include <stdint.h>" and "MOZ_STATIC_ASSERT" vs "static_assert". IMO, we should providing fallback implementations of standard APIs when there are useful standard APIs defined, instead of creating a C++ NSPR that abstracts over the standard APIs and our fallback.

By the way, I am not saying that the mfbt stuff is un-useful. It is useful. But so are some of the NSPRisms are useful too. For example, there is code in our codebase that makes use of the fact that PL_strXXX(NULL) does not crash in places where std::strXXX(NULL) would crash, so we can't just blindly s/PL_strXXX/strXXX/. My point is that we should just leave the *already working* code alone unless/until rewriting it solves a real problem. And, PR_STATIC_ASSERT(x) is better than even the standard static_assert(x, "useless string").

> None of this addresses the problem of NSS. The usual logic here would
> be, "One problem at a time, one step at a time."

I am very interested in hearing what people see as being bad about NSS, besides the well-known linkage (bug 648407), and size issues (bug 611781), the build time slowness, and the common misunderstandngs about why nsprpub/ and security/nss are managed differently than the rest of the tree. AFAICT, fixing problems with patches to NSS is much, much easier than trying to replace it with something else. NSS isn't some immutable third-party software. It is a Mozilla project that Mozilla has chosen not to contribute to until recently.

Regarding system NSPR and system NSS: It is our choice whether to support using system NSPR and system NSS or not. Personally, I don't think we win very much by dropping that support, and I think we would risk losing a lot of the help we get from Red Hat if we did so. But, also, even for Red Hat, some parts of this system library support are more critical than others. Whatever the problems are, we should just solve them.

- Brian, "useless string"

[Jeff Walden]

On 12/28/2011 11:18 PM, Brian Smith wrote:
> Jeff Walden wrote:
>> - its code is inscrutable beneath layers of over-abstracting macros
>> for lots of platform-specific differences
>> - it duplicates a lot of standard library stuff with its own new API
>> to learn
>
> This is exactly my criticism of mfbt. "#include<mozilla/StdInt.h>" vs "include<stdint.h>"

Honestly, I hate "mozilla/StdInt.h" as much as the next person, compared to just using <stdint.h>. But it was the only way to have the stdint types right now. When we drop support for MSVC less than 2010, I'll be perfectly happy to do the mass search-and-replace.

> and "MOZ_STATIC_ASSERT" vs "static_assert". IMO, we should providing fallback implementations of standard APIs when there are useful standard APIs defined,

Note that MOZ_STATIC_ASSERT isn't as equally usable as the bona fide static_assert of C++11 or _Static_assert of C11. The standard constructs are usable in more places than MOZ_STATIC_ASSERT is usable: inside structs but not inside methods inside them, for example. (This would be more pleasant than having |static void staticAsserts()| methods to enclose them.) If we can't implement identically extensive functionality -- and we cannot using either the extern-with-sized-array or typedef-with-sized-array tricks, because externs aren't allowed in structs in C11 or C++11 and redundant typedefs are forbidden in C99 and in class scope in C++11 -- we shouldn't attempt to use the name.

Using the standard name is likely to lead someone astray who thinks our reimplementations are coterminous with the standard constructs. (Standard constructs which in C++ need no header at all, and in C need only a header often bootlegged from some other header, note.) Someone using modern gcc would eventually attempt to use static_assert directly inside a class definition, say, only to discover the error when tinderbox builds failed. A name clearly our own connotes exactly the semantics we document and define and is not a false cognate for other semantics.

> instead of creating a C++ NSPR that abstracts over the standard APIs and our fallback.

I'm reminded a little bit of how Prototype used to add methods to various prototype objects in the DOM, and to builtin prototype objects in the JS language. This was all prior to Object.defineProperty, so such additions could not replicate [[DontEnum]] semantics, most prominently. Eventually they stopped doing this because of its effects on unrelated code not aware of these prototypal mutations, which for better or worse assumed for-in loops would work on objects chaining to these modified prototypes.

Jeff

[Mike Hommey]

On Wed, Dec 28, 2011 at 04:42:04PM -0800, Brian Smith wrote:
> Mike Hommey wrote:
> > On Tue, Dec 27, 2011 at 04:25:30AM -0800, Brian Smith wrote:
> > > We already have such a micro-library: libplc, which is part of NSPR,
> > > and which is only 22kb (on Windows). I think it would be good to
> > > move the parts of mfbt that also apply to C (e.g. static_assert, a
> > > good implementation of JS_Assert/PR_Assert, and the stdint support)
> > > to libplc, so that NSS and NSPR can benefit from them. Otherwise,
> > > work like this needs to be duplicated between mfbt and NSPR.
> >
> > I don't agree here, mainly because I want to use mfbt in code that
> > needs not to depend on nspr.
>
> What drives the "must not depend on NSPR" requirement? Is it a compile-time requirement or a runtime requirement?

Runtime.

Mike

[Zack Weinberg]

On 2011-12-28 10:48 PM, Jeff Walden wrote:
> C++98 17.4.3.1.1 [lib.macro.names]p2:
>
>> A translation unit that includes a [C++ Standard Library] header shall
>> not
>> contain any macros that define names declared or defined in that header.
>> Nor shall such a translation unit define macros for names lexically
>> identical to keywords.

Oh, I get it now. This is part of the pattern where C++ licenses all
standard library headers to include each other and assume that all the
standard library's identifiers have their standard-library meaning. The
idea is that the library authors can put 'static_assert(...)' (for
instance) inside an STL template somewhere, without worrying about
whether the application has redefined it.

C does precisely the opposite: the standard C headers are required to
behave as if they *don't* include each other, and may not depend on
stdlib identifiers that they don't declare themselves. So a C library
has to have a bunch of __whatever aliases for things.

Anyway, yeah, I'm in agreement with you now: we can't practically define
'static_assert'.

>> It seems to me that we can get to "only used via NSS" pretty easily,
>> by adding stuff to mfbt as necessary and/or converting to standard
>> library features.
>
> Yes. Technically it's "easy". As a practical matter it'll take awhile,
> with a fair bit of code to rewrite/reimplement in terms of a new API.
> And then there's the time to design the new API *right*. Easy? Yes. And no.
>
>> Replacing NSS is of course going to be a lot harder.
>
> Yes, again.

I don't feel that there is *huge* urgency here -- if I were in charge of
the general codebase-modernization drive, I think I'd actually go after
some of XPCOM's worst excesses first. And I could make a case that NSS
is a bigger drag on our ability to innovate, honestly. It's certainly
the more *relevant* drag for what I do nowadays. Unfortunately, I'm not
aware of any other TLS library that can be said to be an *improvement*
on NSS.

zw

[Mike Hommey]

On Thu, Dec 29, 2011 at 05:08:35PM +0100, Zack Weinberg wrote:
> I don't feel that there is *huge* urgency here -- if I were in
> charge of the general codebase-modernization drive, I think I'd
> actually go after some of XPCOM's worst excesses first. And I could
> make a case that NSS is a bigger drag on our ability to innovate,
> honestly. It's certainly the more *relevant* drag for what I do
> nowadays. Unfortunately, I'm not aware of any other TLS library
> that can be said to be an *improvement* on NSS.

IMHO the drag is not really NSS itself, but the fact that it is a
third-party library that has binary compatibility as a core
functionality.

Mike

[Jeff Walden]

On 12/29/2011 10:08 AM, Zack Weinberg wrote:
>>> Replacing NSS is of course going to be a lot harder.
>>
>> Yes, again.
>
> I don't feel that there is *huge* urgency here -- if I were in charge of the general codebase-modernization drive, I think I'd actually go after some of XPCOM's worst excesses first.

Agreed. I'm pretty sure modernization efforts are mostly going to be on an as-needed basis for awhile anyway. (Although I think "as-needed" should to some extent include "when rewriting other things, where the existing building blocks unnecessarily obfuscate".) People do want to get stuff done without and having to architect constantly. But they should consider it nonetheless, for the multiplier effect in improvements such changes enable elsewhere.

> And I could make a case that NSS is a bigger drag on our ability to innovate, honestly. It's certainly the more *relevant* drag for what I do nowadays. Unfortunately, I'm not aware of any other TLS library that can be said to be an *improvement* on NSS.

I could believe that, regarding NSS.

I could also go the other way, tho. Security is hard. Rewriting and refactoring there seems quite a bit more dangerous than in other areas, so more caution is advisable. But if that's what's needed, and I suspect in the long run it will be needed to some extent, "hard" shouldn't stop us forever. I haven't had much reason to use NSS myself, tho, so it's hard for me to judge just how much of an issue it is.

Jeff

[Neil]

Chris Jones wrote:

>Ehsan Akhgari wrote:
>
>
>>We do have other macros with names in their names as opposed to verbs (examples: NS_PRECONDITION, NS_ERROR, NS_WARNING, etc).
>>
>Those are also bad names ;)
>

Aww, I wanted to suggest MOZ_ERROR_UNLESS as our replacement for
NS_ASSERTION; I get the feeling that an error is more severe than a
warning (which just logs to stdout), but not so severe that you can't
ignore it.

--
Warning: May contain traces of nuts.

[Benoit Jacob]

2011/12/28 Ehsan Akhgari <ehsan....@gmail.com>:

> I'm ambivalent as to which name should be chosen.  I think the biggest
> chunk of the problem is what programmers familiar with C's assert() think
> when they see any name containing "assert"/"assertion".  So if we would
> have to pick a new name, I'd vote for MOZ_NONFATAL_ASSERT.  Although I have
> to say that so far I haven't seen a very compelling reason for renaming
> MOZ_ASSERTION, and MOZ_NONFATAL_ASSERT is definitely less readable and
> harder to type.

Just for the record, even XPCOM code gets confused into using
NS_ASSERTION / NS_PRECONDITION where a fatal assertion is really
needed, for example:

http://hg.mozilla.org/mozilla-central/file/30161b298513/xpcom/glue/nsTArray.h#l550

This has been a cause of security bugs, as NS_ASSERTION failures are
often ignored, for example:

https://bugzilla.mozilla.org/show_bug.cgi?id=686398

Tracking bug for such issues:

https://bugzilla.mozilla.org/show_bug.cgi?id=NS_ASSERTION_SUX

So if nobody objects, I would like to change all array-addressing
accessors, like nsTArray::ElementAt(), to use a fatal assertion
instead of a NS_ASSERTION. More generally, every assertion that guards
against an illegal memory access.

Cheers,
Benoit

>
> Cheers,
> Ehsan
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Robert O'Callahan]

On Sat, Dec 31, 2011 at 12:48 PM, Benoit Jacob <jacob.b...@gmail.com>wrote:

> Just for the record, even XPCOM code gets confused into using
> NS_ASSERTION / NS_PRECONDITION where a fatal assertion is really
> needed, for example:
>
>
> http://hg.mozilla.org/mozilla-central/file/30161b298513/xpcom/glue/nsTArray.h#l550
>
> This has been a cause of security bugs, as NS_ASSERTION failures are
> often ignored, for example:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=686398
>

I'm not sure what the problem was there. Was it simply that you ignored
NS_ASSERTION failures while debugging the test?

So if nobody objects, I would like to change all array-addressing
> accessors, like nsTArray::ElementAt(), to use a fatal assertion
> instead of a NS_ASSERTION. More generally, every assertion that guards
> against an illegal memory access.
>

That sounds reasonable.

[Benoit Jacob]

2011/12/30 Robert O'Callahan <rob...@ocallahan.org>:

> On Sat, Dec 31, 2011 at 12:48 PM, Benoit Jacob <jacob.b...@gmail.com>
> wrote:
>>
>> Just for the record, even XPCOM code gets confused into using
>> NS_ASSERTION / NS_PRECONDITION where a fatal assertion is really
>> needed, for example:
>>
>>
>> http://hg.mozilla.org/mozilla-central/file/30161b298513/xpcom/glue/nsTArray.h#l550
>>
>> This has been a cause of security bugs, as NS_ASSERTION failures are
>> often ignored, for example:
>>
>> https://bugzilla.mozilla.org/show_bug.cgi?id=686398
>
>
> I'm not sure what the problem was there. Was it simply that you ignored
> NS_ASSERTION failures while debugging the test?

Yes, if I remember correctly.

Benoit

[Kyle Huey]

On Fri, Dec 30, 2011 at 6:48 PM, Benoit Jacob <jacob.b...@gmail.com>wrote:

> So if nobody objects, I would like to change all array-addressing
> accessors, like nsTArray::ElementAt(), to use a fatal assertion
> instead of a NS_ASSERTION. More generally, every assertion that guards
> against an illegal memory access.
>

Do you mean a debug only fatal assertion or a fatal assertion even in opt
builds? I don't know that the former helps much (though if you want to,
I'm not going to object), and I wonder if we can afford the perf hit of the
latter.

- Kyle

[Robert O'Callahan]

On Sat, Dec 31, 2011 at 1:01 PM, Benoit Jacob <jacob.b...@gmail.com>wrote:

> 2011/12/30 Robert O'Callahan <rob...@ocallahan.org>:

> > I'm not sure what the problem was there. Was it simply that you ignored
> > NS_ASSERTION failures while debugging the test?
>
> Yes, if I remember correctly.
>

Don't do that :-).

[Benoit Jacob]

2011/12/30 Kyle Huey <m...@kylehuey.com>:

> On Fri, Dec 30, 2011 at 6:48 PM, Benoit Jacob <jacob.b...@gmail.com>
> wrote:
>>
>> So if nobody objects, I would like to change all array-addressing
>> accessors, like nsTArray::ElementAt(), to use a fatal assertion
>> instead of a NS_ASSERTION. More generally, every assertion that guards
>> against an illegal memory access.
>
>
> Do you mean a debug only fatal assertion or a fatal assertion even in opt
> builds? I don't know that the former helps much (though if you want to, I'm
> not going to object), and I wonder if we can afford the perf hit of the
> latter.

We can't afford the perf hit of the latter, so I meant the former. I
think it helps a lot as it ensures that we can't ignore it when it
happens in unit tests, and personally I test a lot of WebGL pages in
debug builds all the time so I would catch a lot more bugs if they
crashed my debug builds.

Benoit

[Robert O'Callahan]

On Sat, Dec 31, 2011 at 1:14 PM, Benoit Jacob <jacob.b...@gmail.com>wrote:

> We can't afford the perf hit of the latter, so I meant the former. I
> think it helps a lot as it ensures that we can't ignore it when it
> happens in unit tests, and personally I test a lot of WebGL pages in
> debug builds all the time so I would catch a lot more bugs if they
> crashed my debug builds.
>

You can set XPCOM_DEBUG_BREAK=abort.

[Benoit Jacob]

2011/12/30 Robert O'Callahan <rob...@ocallahan.org>:

> On Sat, Dec 31, 2011 at 1:14 PM, Benoit Jacob <jacob.b...@gmail.com>
> wrote:
>>
>> We can't afford the perf hit of the latter, so I meant the former. I
>> think it helps a lot as it ensures that we can't ignore it when it
>> happens in unit tests, and personally I test a lot of WebGL pages in
>> debug builds all the time so I would catch a lot more bugs if they
>> crashed my debug builds.
>
>
> You can set XPCOM_DEBUG_BREAK=abort.

We have too many NS_ASSERTION failures for this to be practical in
most cases, e.g. browsing to some common WebGL demos.

Benoit

[Kyle Huey]

On Fri, Dec 30, 2011 at 8:22 PM, Benoit Jacob <jacob.b...@gmail.com>wrote:

>
> We have too many NS_ASSERTION failures for this to be practical in
> most cases, e.g. browsing to some common WebGL demos.

Are there bugs on file for these assertions? I browse the web with debug
builds for a few hours every week and hardly ever hit assertions (and
they're always on file).

- Kyle

[Benoit Jacob]

2011/12/30 Kyle Huey <m...@kylehuey.com>:

I just filed https://bugzilla.mozilla.org/show_bug.cgi?id=714398 for
the first one I could reproduce; the STR is just open an email in
GMail.

I've not been able to reproduce the ones I seemed to remember hitting
in WebGL demos.

Benoit

[Robert Kaiser]

Mike Hommey schrieb:

> IMHO the drag is not really NSS itself, but the fact that it is a
> third-party library that has binary compatibility as a core
> functionality.

I find it interesting that NSPR and NSS are both Mozilla projects and at
the same time considered "third-party" for Mozilla.

Robert Kaiser

[Jeff Walden]

On 12/31/2011 08:46 AM, Robert Kaiser wrote:
> I find it interesting that NSPR and NSS are both Mozilla projects and at the same time considered "third-party" for Mozilla.

They're in separate repositories. (Still CVS (!?!?) last I heard, too.) We only do infrequent source imports. We make few (or more likely no) changes to those imports. "Third-party" isn't in the sense of code ownership (which is not particularly relevant for freely licensed code). It's in the sense that we treat it the same way we treat all other code we add to our repository with the intent of changing it as little as possible except through wholesale updates.

Jeff

[Chris Peterson]

On 12/28/11 4:53 AM, Ted Mielczarek wrote:
> We already have NS_ASSERTION, and it's practically useless. Our unit
> tests hit multiple assertions, and I'm sure our users would too if
> they were running debug builds. If the assertions aren't fatal, then
> they're unlikely to ever get fixed. In which case, why bother having
> them?

A compromise between fatal asserts and log-a-warning-that-will-get-fixed
NS_ASSERTIONs might be to debugbreak IFF a debugger is attached. This
would allow developers to find non-fatal bugs without breaking unit
tests and without preventing other developers from hitting 'continue' in
the debugger to ignore "not my problem" asserts.

Windows pseudocode:

if (!(expr) && IsDebuggerPresent()) {
DebugBreak(); // or raise(SIGTRAP) on Unix
}


chris

[Neil]

Chris Peterson wrote:

> A compromise between fatal asserts and
> log-a-warning-that-will-get-fixed NS_ASSERTIONs might be to debugbreak
> IFF a debugger is attached.

You can achieve this by setting the value of
Software\mozilla.org\windbgdlg\ASSERTION: to REG_DWORD (5).

[Kyle Huey]

On Mon, Jan 2, 2012 at 3:51 AM, Chris Peterson <cpet...@mozilla.com>wrote:

> On 12/28/11 4:53 AM, Ted Mielczarek wrote:
>
>> We already have NS_ASSERTION, and it's practically useless. Our unit
>> tests hit multiple assertions, and I'm sure our users would too if
>> they were running debug builds. If the assertions aren't fatal, then
>> they're unlikely to ever get fixed. In which case, why bother having
>> them?
>>
>

> A compromise between fatal asserts and log-a-warning-that-will-get-**fixed

> NS_ASSERTIONs might be to debugbreak IFF a debugger is attached. This would
> allow developers to find non-fatal bugs without breaking unit tests and
> without preventing other developers from hitting 'continue' in the debugger
> to ignore "not my problem" asserts.
>
> Windows pseudocode:
>
> if (!(expr) && IsDebuggerPresent()) {
> DebugBreak(); // or raise(SIGTRAP) on Unix
> }
>

The XPCOM_DEBUG_BREAK environment variable allows you to configure the
NS_ASSERTION behavior along these lines.

- Kyle

[Gervase Markham]

On 28/12/11 08:56, Robert O'Callahan wrote:
> In classical computer science, in the words of Wikipedia an assertion is
> simply "a predicate placed in a program to indicate that the developer *
> thinks* that the predicate is always true at that place". A checked
> assertion that doesn't terminate the program is still definitely an
> assertion, and I'd like to use the accurate term because I don't know of a
> better one. So how about MOZ_NONFATAL_ASSERT? MOZ_BENIGN_ASSERT?
>
> If not, maybe MOZ_CHECK.

MOZ_HOPE?

MOZ_FAITH? ;-)

Gerv

[Chris Peterson]

On 12/28/11 4:53 AM, Ted Mielczarek wrote:
> We already have NS_ASSERTION, and it's practically useless. Our unit
> tests hit multiple assertions, and I'm sure our users would too if
> they were running debug builds. If the assertions aren't fatal, then
> they're unlikely to ever get fixed. In which case, why bother having
> them?

A compromise between fatal asserts and log-a-warning-that-will-get-fixed

NS_ASSERTIONs might be to debugbreak IFF a debugger is attached. This
would allow developers to find non-fatal bugs without breaking unit
tests and without preventing other developers from hitting 'continue' in
the debugger to ignore "not my problem" asserts.

Windows pseudocode:

if (!(expr) && IsDebuggerPresent()) {
DebugBreak(); // or raise(SIGTRAP) on Unix
}

chris