820686 - Try to clear up confusion about MOZ_NOT_REACHED

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Justin Lebar (not reading bugmail)]

Lots of reviewers have asked me to substitute a closing MOZ_CRASH() with a MOZ_NOT_REACHED(), as though these two are equivalent.

They're not; MOZ_NOT_REACHED is not an assertion.  It's a flag which enables compiler optimizations.

Given that there's widespread confusion about this, I propose modifying the comments on MOZ_NOT_REACHED and sending a public service announcement to dev.planning.

I'll post a patch in a moment.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

I know the example is contrived, and you don't like that.  Suggestions for
improvement are welcome; I'm kind of at a loss for other occasions when when
it's appropriate to use this macro, to be honest.

Comment 2

[Nicholas Nethercote [inactive]]

Should we do away with MOZ_NOT_REACHED?  Bug 824840 shows another example, where bz (of all people) was confused about it even after I pointed him at this bug.  The fact that you can't come up with a good example of its use in a comment make me suspect that we don't need it at all.

(On a side note:  part of the confusion may be due to the fact that JS_NOT_REACHED used to be equivalent to JS_ASSERT(false).  That's how everybody seems to be using it.)

If you really believe we need this macro, can we rename it MOZ_OPTIMIZE_FOR_UNREACHABILITY or something equally descriptive and awkward, so that people think twice before using it?

Comment 3

[Nicholas Nethercote [inactive]]

Oh, and the existing not-quite-right uses should also be changed.

Comment 4

[Marco Bonardo [:mak]]

I suspect the confusion is increased by the existence of NS_NOTREACHED that is likely doing what people expect MOZ_NOT_REACHED to do (assert in debug, nothing in opt)

Comment 5

[Justin Lebar (not reading bugmail)]

> Should we do away with MOZ_NOT_REACHED?

I can fathom circumstances under which MOZ_NOT_REACHED would be appropriate.  Particularly in SM, I imagine there are places where we really do want the compiler to assume a particular point cannot be reached.

But perhaps, as mak suggests, it shouldn't be called "MOZ_NOT_REACHED".  Maybe MOZ_ASSUME_NOT_REACHED() would be more clear.  And I suspect that the majority of places where MOZ_NOT_REACHED is used it's not being used as an optimization.

Comment 6

[Jeff Walden [:Waldo]]

I have partial comments I really need to get around to polishing up and posting.  In the meantime, I'm not sure I agree with the premise of some of the comments here, although I'm certainly happy to adjust docs to make them more understandable or more precise or whatever regardless of any semantics issues.

An assertion indicates that a condition is impossible to encounter.  There should be no expectation about semantics when it is encountered and fails (however failure is defined -- by being reached, for MOZ_NOT_REACHED, for its condition failing with MOZ_ASSERT [or that it would have failed, if it were checked or could be checked in an opt build], or whatever).  By this I mean the *concept* of an assertion, of course.  (We could say we have our own thing which we call "assertion", but that just muddies the water.)

MOZ_NOT_REACHED() has no semantics at all, because it's impossible for it to be hit.  If you say it's impossible for this to happen, you can't very well say "...but if it does, do this".  You're off the rails already.  (It's like saying fluffy unicorns do not exist, but the fluffy unicorns that do exist are piiiiiiiiink.  It's self-contradictory.)  That it happens to let compilers optimize knowing a point is unreachable is gravy, not promised semantics.  MOZ_CRASH() is totally different -- it just crashes the program, with no implication that something bad happened, or that something bad didn't happen.  It's up to the user of it to decide what it implies, if anything.

Comment 7

[Nicholas Nethercote [inactive]]

> An assertion indicates that a condition is impossible to encounter.  There
> should be no expectation about semantics when it is encountered and fails

I sure hope it aborts the process in a debug build.

A concrete question that's been bugging me:  what should I use in the default case of a switch that shouldn't be hit?  My current practice is to put either MOZ_ASSERT(false) or MOZ_CRASH(), depending on where it occurs and how hard-assed I'm feeling at the time.  Is that reasonable?

Comment 8

[Justin Lebar (not reading bugmail)]

AIUI you're saying that MOZ_NOT_REACHED is in fact an "assertion", where an "assertion" is something that's impossible to reach.  Is that right?

If so, I contend that definition is not consistent with the usage of "assertion" in Mozilla code and indeed in code in general.

MOZ_ASSERT has well-defined semantics when it fails -- we don't say that it's impossible for a MOZ_ASSERT condition to be false.  But I hope you'll agree that MOZ_ASSERT is an "assertion".  Similarly for NS_ASSERT, and NS_ABORT_IF_FALSE.

C's assert() function is also well-defined.  I hope you'll agree that's reasonably called an "assertion".

By far the most common usage of "assertion" -- in both Mozilla code and in programming in general -- is a function which checks a condition and declares that there's an error if that condition fails.  If there is an error, it then takes some defined action.  I'd be happy to extend the notion of "assertion" to a function whose check may be trivial (always declare that there is an error).

You're of course free to disagree with this usage and to define the word "assertion" however you like, but for the purposes of documentation, I don't think we should call a function which behaves differently than all other assertions an "assertion", because that would cause some people to use MOZ_NOT_REACHED incorrectly.  Indeed it has.

> That it happens to let compilers optimize knowing a point is unreachable is gravy, not 
> promised semantics.

Understood.  But we agree that the only reason to use MOZ_NOT_REACHED is to enable these optimizations, right?  If you don't care about the speed of your code, you should never use MOZ_NOT_REACHED, as it is strictly less safe than MOZ_CRASH() or MOZ_ASSERT(false), right?

It's a simple result that it's very unlikely that you're able to prove anything non-trivial about the Mozilla codebase, so in particular it's unlikely that you can prove that a particular line of code is not reached.  As a programmer, you can "assert" without proof that a line is not reached (*), but that's all.  And even if you can prove that a particular line is not reached, you certainly can't prove that there's no modification one can make to Gecko which will cause it to be reached.

So since any line of code /may/ be reached -- either in this build or a future one -- MOZ_NOT_REACHED(), which triggers undefined behavior when it's reached, is less safe than the alternatives, which trigger defined behavior.

The chance of undefined behavior is necessary sometimes.  But you have to get something in return for the decrease in safety for it to be worthwhile.  Certainly if you got an increase in speed, that would be something.  Perhaps there's some other advantage we gain from MOZ_NOT_REACHED.

But the point is, MOZ_NOT_REACHED is not an "assertion" how most of us understand that word, and MOZ_NOT_REACHED decreases the safety of our code, so should not be used where you don't care about its corresponding benefits.  That's what I was trying to get across in this patch.

> what should I use in the default case of a switch that shouldn't be hit?

Because code can change, I think you should use something with defined behavior, unless you care about speed.  Someone could add a new value to the enum, or someone could pass in a bad int value to the function.

(*) Perhaps this is what you mean by "assertion", but note that this is different from asking the program to check a condition.

Comment 9

[Justin Lebar (not reading bugmail)]

Wow, sorry, that was a lot longer than I intended it to be.

Comment 10

[Nicholas Nethercote [inactive]]

> MOZ_NOT_REACHED decreases the safety of our code

This point is critical, and I didn't even understand it until just now.

Comment 11

[Chris Peterson [:cpeterson]]

Can we remove (the subtle and dangerous) MOZ_NOT_REACHED and fold its compiler-specific "not reached" optimization hint into the end of MOZ_CRASH, one of the few places in our code that we can guarantee is unreachable?

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 691220 [details] [diff] [review]
Patch, v1

Jeff, should I take the lack of action here as an r-?

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: MOZ_ASSUME_UNREACHABLE.patch

Here is an alternate patch:

1. Rename MOZ_NOT_REACHED_MARKER() to MOZ_ASSUME_UNREACHABLE() and remove fatal abort() calls.

2. Call MOZ_ASSUME_UNREACHABLE() at end of MOZ_CRASH() so MOZ_CRASH() can be used to provide optimization hints about unreachable code.

3. Temporarily call MOZ_CRASH() from MOZ_NOT_REACHED(), assuming MOZ_NOT_REACHED() calls could be replaced with MOZ_CRASH() or MOZ_ASSUME_UNREACHABLE().

Curiously, the try builds I ran are green except for "Win debug" failures. Perhaps "Win debug" tests are hitting a MOZ_NOT_REACHED(), which now crashes instead of asserting?

https://tbpl.mozilla.org/?tree=Try&rev=e505408211db
https://tbpl.mozilla.org/?tree=Try&rev=e0594e273609

Comment 14

[Justin Lebar (not reading bugmail)]

Jeff, I don't think it's fair to pocket veto these patches.

If you disagree with both approaches here, can you please say so?

Comment 15

[Jeff Walden [:Waldo]]

Not a pocket veto, just a harder question/lower priority than other reviews I have to deal with.  (Including a couple going back to October.  :-( )  I'll try to look at this later today.

Comment 16

[Justin Lebar (not reading bugmail)]

Can we perhaps add a peer to mfbt to take you off the critical path?

Comment 17

[Jeff Walden [:Waldo]]

There are several peers already, if unlisted ones (just, um, like the owner is, in letter if not spirit.  or something).  Fourteen-month-old mail has this as the set:

"cjones looks like the winner by hg annotate. Chris, are you willing to own? Luke, will you peer along with Jeff, mhommey, Ms2ger"

which admittedly is probably 20% out of date.

I'd just update the module ownership page with this, but for the initial change it feels a little weird doing it -- especially as, when I added the section to the page originally, it spawned the thread that led to that mail.

Comment 18

[Justin Lebar (not reading bugmail)]

fwiw that list might be more than 20% out of date; here's the tail of

$ git log --pretty='%an' mfbt | sort | uniq -c | sort -g

   4 Aryeh Gregor
   4 Boris Zbarsky
   4 Jacek Caban
   4 Jeff Muizelaar
   4 Ms2ger
   5 Chris Jones
   5 Chris Leary
   5 Christian Holler
   7 Ed Morley
   7 Terrence Cole
   8 Nathan Froyd
  10 Ehsan Akhgari
  11 Chris Peterson
  11 Mike Hommey
  11 Rafael Ávila de Espíndola
  12 Benoit Jacob
  19 Justin Lebar
  79 Jeff Walden

Comment 19

[Justin Lebar (not reading bugmail)]

Comment on attachment 691220 [details] [diff] [review]
Patch, v1

I'd be happy with glandium's review here, if that works for you, Jeff.

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 703592 [details] [diff] [review]
MOZ_ASSUME_UNREACHABLE.patch

Review of attachment 703592 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Assertions.h
@@ +132,5 @@
> +      (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5))
> +   /*
> +    * __builtin_unreachable() was implemented in gcc 4.5.  If we don't have
> +    * that, call a noreturn function; abort() will do nicely.  Qualify the call
> +    * in C++ in case there's another abort() visible in local scope.

You removed what is described here.

Comment 21

[Mike Hommey [:glandium]]

So, we have here two different approaches, none of which are entirely satisfactory imho. On one end, Justin's patch attempts to describe what MOZ_NOT_REACHED does, which is nice, but the way it currently works is not what is described: release builds will happily abort() with the current code depending on the compiler used. On the other hand, Chris's patch changes the semantics entirely so that MOZ_NOT_REACHED crashes/aborts in all cases. I think I prefer Justin's approach, provided the code is modified to actually do what is advertised.

Comment 22

[Justin Lebar (not reading bugmail)]

> On the other hand, Chris's patch changes the semantics entirely so that MOZ_NOT_REACHED 
> crashes/aborts in all cases.

Well, he essentially merges MOZ_CRASH and MOZ_NOT_REACHED.

This actually seems perhaps better to me, tbh.  It seems like this change removes an entire class of mistake / argument.

But if we go with the comment-only change, perhaps we can also change the name of MOZ_NOT_REACHED to MOZ_ASSUME_UNREACHABLE?

> provided the code is modified to actually do what is advertised.

Do you want the comment to change from "will have undefined behavior" to "may have undefined behavior"?  Or do you want to remove the existing abort()'s and replace them with a call to an empty noreturn function?

Comment 23

[Mike Hommey [:glandium]]

(In reply to Justin Lebar [:jlebar] from comment #22)
> > On the other hand, Chris's patch changes the semantics entirely so that MOZ_NOT_REACHED 
> > crashes/aborts in all cases.
> 
> Well, he essentially merges MOZ_CRASH and MOZ_NOT_REACHED.
> 
> This actually seems perhaps better to me, tbh.  It seems like this change
> removes an entire class of mistake / argument.

It will likely add an entire new class of problems, with the compiler trying to be smart and making crash reports useless because it throws away the return pointer value. (and that's not theoretical, that's why we removed noreturn from mozalloc_abort on arm)

> But if we go with the comment-only change, perhaps we can also change the
> name of MOZ_NOT_REACHED to MOZ_ASSUME_UNREACHABLE?
> 
> > provided the code is modified to actually do what is advertised.
> 
> Do you want the comment to change from "will have undefined behavior" to
> "may have undefined behavior"?  Or do you want to remove the existing
> abort()'s and replace them with a call to an empty noreturn function?

I don't know. The more I think about it, the more I think we may just want to remove that macro altogether, and add an optional reason to MOZ_CRASH, that would be displayed on debug builds. And leave __builtin_unreachable out of the equation, but I do understand some places may want it.

Comment 24

[Justin Lebar (not reading bugmail)]

> It will likely add an entire new class of problems, with the compiler trying to be smart 
> and making crash reports useless because it throws away the return pointer value.

Ah, excellent point.

> The more I think about it, the more I think we may just want to remove that macro 
> altogether, and add an optional reason to MOZ_CRASH, that would be displayed on debug 
> builds. And leave __builtin_unreachable out of the equation, but I do understand some 
> places may want it.

In the process of removing the macro, perhaps we'll get a sense for whether any codesites need the speed increase.  I'd be in favor of converting most callsites to MOZ_CRASH(reason) and creating MOZ_UNSAFE_ASSUME_UNREACHABLE() if necessary.

Comment 25

[Jeff Walden [:Waldo]]

MOZ_CRASH() has wanted a (non-optional) reason string for awhile -- see bug 763070.  The issue has always been that we want MOZ_CRASH() to be guaranteed to crash, non-exploitably -- it should be usable even with an arbitrarily corrupted heap.  That's easily enough done on Linux, but I'm not sure how to do it elsewhere.  We could expediently ignore the string on "hard" platforms, if we thought printing a reason *sometimes* to be important enough to not delay for printing it *all* the time.  Now that I state it that way, actually I think probably we should just do that, and let other platforms play catchup.  :-)

If it'll make people happy, I guess MOZ_ASSUME_UNREACHABLE() is okay with me.  But I still think we should be confident in what we assert, and we should let the compiler know, in optimized builds.  Worries about theoretical safety when we're wrong, given we're already in an incredibly unsafe language, continue to seem overblown to me.

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 691220 [details] [diff] [review]
Patch, v1

Clearing wrt comment 25.

Comment 27

[Justin Lebar (not reading bugmail)]

> The issue has always been that we want MOZ_CRASH() to be guaranteed to crash, 
> non-exploitably

It's not clear to me how we can do this safely even on Linux.

We could have MOZ_CRASH("foo") expand to

  write(1, "foo\n", 4);

but there's no guarantee that fd 1 is actually stdout.

Did you have a more clever way of doing this?

Comment 28

[Justin Lebar (not reading bugmail)]

Jeff, ping re comment 27?  I think we'd all love to resolve this bug.

Comment 29

[Jeff Walden [:Waldo]]

Mostly the concern, I think, is about the process itself being exploited, through, say, fprintf's use of the malloc heap or similar (if fprintf were naively used).  fd1 not being stdout wouldn't present an issue in this regard.

I'm willing to accept that a re-bound fd1 will cause the MOZ_CRASH string to go nowhere visible.  If we're truly paranoid we could try writing to /dev/tty, but given we're writing a constant text string we chose, it seems unlikely we'd compromise some other process or poison a file stream in an exploitable manner.

Comment 30

[Justin Lebar (not reading bugmail)]

> Mostly the concern, I think, is about the process itself being exploited, through, say, 
> fprintf's use of the malloc heap or similar (if fprintf were naively used).  fd1 not 
> being stdout wouldn't present an issue in this regard.

What if fd1 were rebound to /proc/self/mem?

/dev/tty seems pretty safe to me, naively.

Comment 31

[Jeff Walden [:Waldo]]

Didn't know /proc/self/mem was a thing!  (Although, really, I probably should have expected it.)  And the exploit there is the write goes to an unmapped page, that triggers an exploit-induced page fault handler or similar, and then goes to town somehow.  That does seem somewhat worrisome to us paranoiacs, although if you can install a custom #PF handler, it seems likely you'd have your vector already.  Maybe /dev/tty is the reasonable thing to do, then, out of an abundance of caution if for no other reason.

Comment 32

[Justin Lebar (not reading bugmail)]

> And the exploit there is the write goes to an unmapped page, that triggers an exploit-
> induced page fault handler or similar, and then goes to town somehow.

Or perhaps to modify the code we're running so the next instruction doesn't crash us!

Comment 33

[Justin Lebar (not reading bugmail)]

Okay, we should take this discussion into bug 763070.

Comment 34

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=6ef3dfed32d8

Comment 35

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=93ed07c526d2

Comment 36

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=ae0739eba891

Comment 37

[Justin Lebar (not reading bugmail)]

Attachment: Part 1a: s/MOZ_NOT_REACHED/MOZ_CRASH/ in xpcom/.

Most of the patches that I'm going to label "Part 1x" are uninteresting.  They're not totally mechanical, however, so I'd like some eyes on them.

In particular, if I've changed a MOZ_NOT_REACHED to a MOZ_CRASH in performance-critical code, please let me know.  Most everything looked non-critical.  I kicked off try runs with talos, but our infrastructure for meaningfully comparing talos results on try is so broken, I think it will probably be easier just to push to m-i and back it out if we see regressions.

Comment 38

[Justin Lebar (not reading bugmail)]

Attachment: Part 1b: s/MOZ_NOT_REACHED/MOZ_CRASH/ in widget/.

Comment 39

[Justin Lebar (not reading bugmail)]

Attachment: Part 1c: s/MOZ_NOT_REACHED/MOZ_CRASH/ in toolkit/.

Comment 40

[Justin Lebar (not reading bugmail)]

Attachment: Part 1d: s/MOZ_NOT_REACHED/MOZ_CRASH/ in security/.

Comment 41

[Justin Lebar (not reading bugmail)]

Attachment: Part 1e: s/MOZ_NOT_REACHED/MOZ_CRASH/ in netwerk/.

Comment 42

[Justin Lebar (not reading bugmail)]

Attachment: Part 1f: s/MOZ_NOT_REACHED/MOZ_CRASH/ in mozglue/.

I'm not 100% confident in these changes, so if you have any doubts, it may be worthwhile to try and poke glandium.

Comment 43

[Justin Lebar (not reading bugmail)]

Attachment: Part 1g: s/MOZ_NOT_REACHED/MOZ_CRASH/ in modules/.

Comment 44

[Justin Lebar (not reading bugmail)]

Attachment: Part 1h: s/MOZ_NOT_REACHED/MOZ_CRASH/ in mfbt/.

Comment 45

[Justin Lebar (not reading bugmail)]

Attachment: Part 1i: s/MOZ_NOT_REACHED/MOZ_CRASH/ in layout/.

There were some parts here that I wasn't sure if the code was perf-sensitive or not.  Probably doesn't make a difference.

Comment 46

[Justin Lebar (not reading bugmail)]

Attachment: Part 1j: s/MOZ_NOT_REACHED/MOZ_CRASH/ in js/.

I deal with JS_NOT_REACHED in a later patch.

Comment 47

[Justin Lebar (not reading bugmail)]

Note that I'm relying on MOZ_CRASH() being noreturn, in all of these patches.  This allows us to do

  int foo() {
    MOZ_CRASH();
  }

i.e., you don't have to put a return statement below a MOZ_CRASH().

Comment 48

[Justin Lebar (not reading bugmail)]

Attachment: Part 1k: s/MOZ_NOT_REACHED/MOZ_CRASH/ in ipc/.

Comment 49

[Justin Lebar (not reading bugmail)]

Attachment: Part 1l: s/MOZ_NOT_REACHED/MOZ_CRASH/ in hal/.

Comment 50

[Justin Lebar (not reading bugmail)]

Attachment: Part 1m: s/MOZ_NOT_REACHED/MOZ_CRASH/ in gfx/.

Comment 51

[Justin Lebar (not reading bugmail)]

Attachment: Part 1n: s/MOZ_NOT_REACHED/MOZ_CRASH/ in extensions/.

Comment 52

[Justin Lebar (not reading bugmail)]

Attachment: Part 1o: s/MOZ_NOT_REACHED/MOZ_CRASH/ in editor/.

Comment 53

[Justin Lebar (not reading bugmail)]

Attachment: Part 1p: s/MOZ_NOT_REACHED/MOZ_CRASH/ in dom/ and content/.

Sorry, this is a big one.

Comment 54

[Justin Lebar (not reading bugmail)]

Attachment: Part 1q: s/MOZ_NOT_REACHED/MOZ_CRASH/ in caps/.

Comment 55

[Justin Lebar (not reading bugmail)]

Attachment: Part 1r: s/MOZ_NOT_REACHED/MOZ_CRASH/ in accessible/.

Comment 56

[Justin Lebar (not reading bugmail)]

Attachment: Part 2a: Change MOZ_NOT_REACHED to MOZ_ASSUME_NOT_REACHED in mfbt/Assertions.h.

This is the main event, where we try to clear up confusion about MOZ_NOT_REACHED.

Comment 57

[Justin Lebar (not reading bugmail)]

Attachment: Part 2b: s/MOZ_NOT_REACHED/MOZ_ASSUME_NOT_REACHED()/ in mfbt/decimal/.

These could probably be MOZ_CRASH()'s, if you like.

Comment 58

[Justin Lebar (not reading bugmail)]

Comment on attachment 760695 [details] [diff] [review]
Part 1j: s/MOZ_NOT_REACHED/MOZ_CRASH/ in js/.

Note that I didn't get rid of all of the MOZ_NOT_REACHED()'s in this patch.  I just got rid of some that seemed (to me) obviously not hot.

Comment 59

[Justin Lebar (not reading bugmail)]

Attachment: Part 2c: s/MOZ_NOT_REACHED|JS_NOT_REACHED/MOZ_ASSUME_NOT_REACHED/ in js/.

This was sed; nothing interesting here except the removal of the macro from Utils.h.

This introduced a few whitespace errors (particularly in macros), but I think I covered them in the next patch.

I don't know if you actually want to get rid of JS_NOT_REACHED, but given that both JS_NOT_REACHED and MOZ_NOT_REACHED were used in the JS engine, I figure now is a good time to get rid of the JS one in favor of the MOZ one.

Comment 60

[Justin Lebar (not reading bugmail)]

Attachment: Part 2d: Remove code after MOZ_ASSUME_NOT_REACHED() in js/.

I left in a bunch of MOZ_ASSUME_NOT_REACHED()'s, because I don't want to mess with the finely-tuned machine that is SM.  But SM has a lot of code which looks like

  switch (foo) {
    ...
    default:
      MOZ_NOT_REACHED();
      return false;
  }

I get rid of these return statements (and other code) that sit after MOZ_NOT_REACHED.  These give us a false sense of security and imply that MOZ_NOT_REACHED does something that it doesn't do.

Comment 61

[Justin Lebar (not reading bugmail)]

In case there's any confusion, the approach here is basically:

 * Switch most users of MOZ_NOT_REACHED to MOZ_CRASH (I switched everything in Gecko, but left most of JS as-is). [per the end of comment 23]

 * Rename MOZ_NOT_REACHED to MOZ_ASSUME_NOT_REACHED.  I'd be happy to change this to MOZ_ASSUME_UNREACHABLE if that's preferred.  [per the last paragraph of comment 25]

 * Change the comment of MOZ_ASSUME_NOT_REACHED to be scarier.  [per my patch from comment 1]

 * Remove dead code beneath MOZ_CRASH() and MOZ_ASSUME_NOT_REACHED.

Comment 62

[Trevor Saunders (:tbsaunde)]

Comment on attachment 760706 [details] [diff] [review]
Part 1r: s/MOZ_NOT_REACHED/MOZ_CRASH/ in accessible/.

I can't imagine any of these coming up in the wild somehow so I think I'd prefer MOZ_ASSUME_UNREACHABLE() or whatever you're calling it, but it probably doesn't really matter

Comment 63

[Justin Lebar (not reading bugmail)]

(In reply to Trevor Saunders (:tbsaunde) from comment #62)
> I can't imagine any of these coming up in the wild somehow so I think I'd
> prefer MOZ_ASSUME_UNREACHABLE() or whatever you're calling it, but it
> probably doesn't really matter

Is this perf-sensitive code?

Comment 64

[Trevor Saunders (:tbsaunde)]

(In reply to Justin Lebar [:jlebar] from comment #63)
> (In reply to Trevor Saunders (:tbsaunde) from comment #62)
> > I can't imagine any of these coming up in the wild somehow so I think I'd
> > prefer MOZ_ASSUME_UNREACHABLE() or whatever you're calling it, but it
> > probably doesn't really matter
> 
> Is this perf-sensitive code?

I doubt it, but perhaps what I should have said is I'm comfortable enough that assert will never fire it being a exploitable bug if it does is ok with me.

Comment 65

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 760686 [details] [diff] [review]
Part 1c: s/MOZ_NOT_REACHED/MOZ_CRASH/ in toolkit/.

Review of attachment 760686 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/identity/IdentityCryptoService.cpp
@@ +358,5 @@
>    }
>    if (!*publicKey) {
>  	SECKEY_DestroyPrivateKey(*privateKey);
>  	*privateKey = NULL;
> +    MOZ_CRASH("PK11_GnerateKeyPair returned private key without public key");

There's no reason to keep what's above the MOZ_CRASH, no?

Comment 66

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 760703 [details] [diff] [review]
Part 1p: s/MOZ_NOT_REACHED/MOZ_CRASH/ in dom/ and content/.

Review of attachment 760703 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/PrimitiveConversions.h
@@ +92,5 @@
>  
>  private:
>    static inline bool converter(JSContext* cx, JS::Handle<JS::Value> v,
>                                 jstype* retval) {
> +    MOZ_CRASH("This should never be instantiated!");

Can't this be a static assertion?

Comment 67

[Justin Lebar (not reading bugmail)]

> Can't this be a static assertion?

How do you mean?  I think a STATIC_ASSERT(false) will fail even if that inline function is never called -- it's not like instantiating a template.

Comment 68

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Oh, hmm, I guess that function isn't templated even if the struct is.  That's unfortunate.

Comment 69

[Jeff Walden [:Waldo]]

Before I forget, yeah, UNREACHABLE is better than NOT_REACHED.  Still looking at the rest of this, trying to get an idea about all the patches before proceeding to mark any particular one of them (except the small one, at least).

Comment 70

[Jeff Walden [:Waldo]]

Comment on attachment 760711 [details] [diff] [review]
Part 2d: Remove code after MOZ_ASSUME_NOT_REACHED() in js/.

Review of attachment 760711 [details] [diff] [review]:
-----------------------------------------------------------------

Removing the subsequent code's a good idea.  Hopefully having apparent paths out the end of the function, or into other code, won't trigger warnings.  Guess we'll find out if they do!

::: js/src/frontend/ParseNode.cpp
@@ +403,1 @@
>          } else {

This was (is) effectively else-after-return.  So we should move the else-block statements up a level and kill off the else.

::: js/src/jsreflect.cpp
@@ +117,2 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_PARSE_NODE);      \
> +        MOZ_ASSUME_NOT_REACHED(expr);                                                  \

Make this MOZ_ASSERT + report + return false instead.  Neither the old code nor what's in your patch really fixes the problem, but this will do as a quick hackaround.

Comment 71

[Jeff Walden [:Waldo]]

Comment on attachment 760710 [details] [diff] [review]
Part 2c: s/MOZ_NOT_REACHED|JS_NOT_REACHED/MOZ_ASSUME_NOT_REACHED/ in js/.

Review of attachment 760710 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, we should only have the one.

Comment 72

[Jeff Walden [:Waldo]]

Comment on attachment 760695 [details] [diff] [review]
Part 1j: s/MOZ_NOT_REACHED/MOZ_CRASH/ in js/.

Review of attachment 760695 [details] [diff] [review]:
-----------------------------------------------------------------

As a practical matter, the only reason for JS_NOT_REACHED is that we hadn't taken the time to convert all of them to MOZ_NOT_REACHED yet.  There was no semantic difference of intent between JS_NOT_REACHED uses and MOZ_NOT_REACHED uses.  So these uses shouldn't be substituted.

The couple following-statement removals here are worth taking, tho -- r=me on those changes.

Comment 73

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #65)
> Comment on attachment 760686 [details] [diff] [review]
> Part 1c: s/MOZ_NOT_REACHED/MOZ_CRASH/ in toolkit/.
> 
> Review of attachment 760686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/identity/IdentityCryptoService.cpp
> @@ +358,5 @@
> >    }
> >    if (!*publicKey) {
> >  	SECKEY_DestroyPrivateKey(*privateKey);
> >  	*privateKey = NULL;
> > +    MOZ_CRASH("PK11_GnerateKeyPair returned private key without public key");
> 
> There's no reason to keep what's above the MOZ_CRASH, no?

I'm not totally sure; it looks like we're trying to destroy the key to keep it from leaking in memory somehow.  I think we should probably leave this.

Comment 74

[Justin Lebar (not reading bugmail)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #70)
> ::: js/src/jsreflect.cpp
> @@ +117,2 @@
> >          JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_PARSE_NODE);      \
> > +        MOZ_ASSUME_NOT_REACHED(expr);                                                  \
> 
> Make this MOZ_ASSERT + report + return false instead.  Neither the old code
> nor what's in your patch really fixes the problem, but this will do as a
> quick hackaround.

Would it be better to report + MOZ_ASSERT + return false instead?  That way we'd still get the error report either way...

Comment 75

[Jeff Walden [:Waldo]]

Comment on attachment 760708 [details] [diff] [review]
Part 2a: Change MOZ_NOT_REACHED to MOZ_ASSUME_NOT_REACHED in mfbt/Assertions.h.

Review of attachment 760708 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Assertions.h
@@ +401,5 @@
> + * This may allow the compiler to generate faster or smaller code, in some
> + * circumstances.  But outside performance- or size-critical code, you
> + * probably shouldn't use this macro, because it's unsafe.  Instead, consider
> + * using MOZ_CRASH() to safely end execution when a particular line of code is
> + * reached, or use MOZ_ASSERT() if you can recover from the failure.

I think this comment has a little bit too much FUD about the consequences of a mistake.  It's not like the consequences will always be arbitrary code execution.  It might be fallthrough to invalid instructions, nonsense instructions that quickly crash, bad computations informed by incorrectly-assumed preconditions, etc.  But the consequences aren't an immediate exploit, and I don't .

This dials back the fear a little bit, and it's a little more informative about what the compiler can do with the info.  How does this read?

"""
MOZ_ASSUME_UNREACHABLE([reason]) tells the compiler that the macro call can't be reached during execution and that any preconditions to it don't hold.  This lets the compiler generate better-optimized code.

If the macro call *is* reached, what happen depends on the compiler, its optimizer, surrounding code, etc.  *Probably* you'll crash...but you might also fall through to code letting an attacker construct an exploit.  Be careful using this macro when you're not completely confident the macro call won't be hit.  For absolute safety, use MOZ_ASSERT or MOZ_CRASH instead.
"""

Comment 76

[Jeff Walden [:Waldo]]

(In reply to Justin Lebar [:jlebar] from comment #74)
> Would it be better to report + MOZ_ASSERT + return false instead?  That way
> we'd still get the error report either way...

That would be exactly equivalent to the LOCAL_ASSERT (?) just above it.  Gets the job done, I guess, but that'd be a bigger change, and it seems nice to minimize this if it's mostly mechanical.

I'm still not particularly convinced this is better with the reason-strings for these being optional.  Explanations, even when moderately obvious in the surrounding code, are *not* simply clutter.  They incent people to better document these cases, and while it's certainly possible to specify nonsense strings, mostly that's not what people have done in the code I've looked at.  (At least in the JS engine.  Maybe people elsewhere are more of the hack-til-it-compiles mentality.)  But I don't really feel like continuing to argue this, so I guess you win.  :-|

Comment 77

[Justin Lebar (not reading bugmail)]

> I think this comment has a little bit too much FUD about the consequences of a mistake.

I don't think it's FUD; that's why I filed this bug in the first place.  :)  This is really the crux of our disagreement, I think.

The comment as written reflects how this macro is used in Gecko after the patches here.  The main reason for going through this exercise of changing everything in Gecko is to set a good example, so certainly I want the comment to match the code.

I tried to make SM's usage of the macro more similar to Gecko's, but that was vetoed in comment 72, and I'm not going to fight that.  So at this point it sounds like Gecko and SM are going to use the macro differently.  That's not ideal, but if that's the best we can do, we shouldn't pretend otherwise in the comment.

How about:

> MOZ_ASSUME_UNREACHABLE([reason]) tells the compiler that it can assume that
> the macro call cannot be reached during execution.  This lets the compiler
> generate better-optimized code under some circumstances, at the expense of
> the program's behavior being undefined if control reaches the
> MOZ_ASSUME_UNREACHABLE.
>
> In Gecko, you probably should not use this macro outside of performance- or
> size-critical code, because it's unsafe.  If you don't care about code size
> or performance, you should probably use MOZ_ASSERT or MOZ_CRASH.
>
> SpiderMonkey is a different beast, and there it's acceptable to use
> MOZ_ASSUME_UNREACHABLE more widely.

We can say whatever you want after "SpiderMonkey".

Comment 78

[Jeff Walden [:Waldo]]

That'll do.

Comment 79

[Justin Lebar (not reading bugmail)]

> That would be exactly equivalent to the LOCAL_ASSERT (?) just above it.  Gets the job done, I guess, 
> but that'd be a bigger change, and it seems nice to minimize this if it's mostly mechanical.

Okay; I have ASSERT + REPORT + return false in my patch queue.

Rebasing this is going to be fun.  :)

Comment 80

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55c1f447549d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1735d098ea86
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ecd26bc5274

Comment 81

[Justin Lebar (not reading bugmail)]

s/MOZ_ASSUME_NOT_REACHED/MOZ_ASSUME_UNREACHABLE/.  Sorry, I forgot we were doing this (but still put it in the mfbt comment and in my newsgroup post, for some reason).

https://hg.mozilla.org/integration/mozilla-inbound/rev/01ad949468fb

Comment 82

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/55c1f447549d
https://hg.mozilla.org/mozilla-central/rev/1735d098ea86
https://hg.mozilla.org/mozilla-central/rev/5ecd26bc5274
https://hg.mozilla.org/mozilla-central/rev/01ad949468fb