[micro-] PSA: IMMEDIATE_CRASH(), NOTREACHED() are now [[noreturn]]

[Wez]

Folks,

Back in February we tweaked IMMEDIATE_CRASH() to be [[noreturn]] annotated. You can now use it (and its friends) in functions with return-values without the need to follow them with unreachable "return dummy value" goop just to keep the compiler happy. \o/

e.g. You can now write a terribly useful function like this:

B AToB(A a) {

 switch (a) {

   case A::kOne: return B::kAlpha;

   ...

 }

 NOTREACHED();

 // In the bad old days, I'd have to return a dummy value here. 

}

Calls affected by this include IMMEDIATE_CRASH(), NOTREACHED() and base::Process::TerminateCurrentProcessImmediately().

We don't currently build with -Wwarn-unused, so if you forget to take advantage of this amazing benefit, don't expect the compiler to nag you.

Enjoy!

Wez

[Greg Thompson]

Hey, that looks convenient. NOTREACHED() is a debug-only thing, is it not (DCHECK(false))? What happens in NDEBUG builds?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALekkJdbyqTVT8w%3DU7jWFy-1mXxfw2aEYjYKEGQs%2BkRTmQk7Tg%40mail.gmail.com.

[Devlin Cronin]

This is awesome, Wez!  One question, as I'm not very familiar with [[noreturn]]: does this also cause the function to immediately exit?  (In debug, in release?)  In particular, another pattern I've seen is:

if (SomeAssumptionViolated()) {

  // This *should* never happen, but can in the cases of [x, y, z].  Handle it gracefully.

  NOTREACHED();

  return;

}

We generally discourage this, but there are a few cases where it's common enough practice (say, handling for corruption, etc).  With [[noreturn]], is the return; still necessary, particularly in release builds (where we won't crash)?

[Nico Weber]

On Thu, Jun 7, 2018 at 10:09 AM, Devlin Cronin <rdevlin...@chromium.org> wrote:

This is awesome, Wez!  One question, as I'm not very familiar with [[noreturn]]: does this also cause the function to immediately exit?  (In debug, in release?)  In particular, another pattern I've seen is:

if (SomeAssumptionViolated()) {

  // This *should* never happen, but can in the cases of [x, y, z].  Handle it gracefully.

  NOTREACHED();

  return;

}

We generally discourage this, but there are a few cases where it's common enough practice (say, handling for corruption, etc).  With [[noreturn]], is the return; still necessary, particularly in release builds (where we won't crash)?

Returning from a noreturn function is undefined behavior. The compiler is free to remove all code after noreturn functions, or do whatever else it wants. So this pattern is unsafe.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/30d0dd9d-26a8-4244-9c01-16f2b2d53c10%40chromium.org.

[Christopher Grant]

This is cool!  I successfully applied this to several spots in our project's tree, but the compiler still complains about one particular instance, here.  I could be blind...  does this case look exceptional for some reason?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Devlin Cronin]

Returning from a noreturn function is undefined behavior. The compiler is free to remove all code after noreturn functions, or do whatever else it wants. So this pattern is unsafe.

Is NOTREACHED() still [[noreturn]] (and thus this pattern unsafe) if DCHECK is off?  If not, then this should be fine (because any cases where DCHECK is on, this just crashes, and the compiler can do what it wants).

I haven't done a sophisticated grep yet, but just skimming through codesearch for NOTREACHED(), there seem to be many instances of `if (<some_condition>) { NOTREACHED(); return [x]; }`.

[Nico Weber]

On Thu, Jun 7, 2018 at 10:25 AM, Devlin Cronin <rdevlin...@chromium.org> wrote:

Returning from a noreturn function is undefined behavior. The compiler is free to remove all code after noreturn functions, or do whatever else it wants. So this pattern is unsafe.

Is NOTREACHED() still [[noreturn]] (and thus this pattern unsafe) if DCHECK is off?  If not, then this should be fine (because any cases where DCHECK is on, this just crashes, and the compiler can do what it wants).

I haven't done a sophisticated grep yet, but just skimming through codesearch for NOTREACHED(), there seem to be many instances of `if (<some_condition>) { NOTREACHED(); return [x]; }`.

Yeah, as you say NOTREACHED is DCHECK(false), so I'm not sure why wez says that NOTREACHED is noreturn now :-) It can only be noreturn in debug builds, and having functions be noreturn in some build configs and not in others seems worse to me than making it consistently _not_ noreturn (else you need a return after NOTREACHED to fix warnings in release builds but not in debug builds, where the compiler now knows that the return is unreachable code). wez?

[Alexei Svitkine]

Another option would be to make NOTREACHED() become CHECK(false) and crash also in release builds. But I assume we have enough of these being hit currently that we'd need to fix a bunch of stuff to make that not increase the crash rate significantly? (Which might still be worthwhile to do.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiG3u2enxtVetT3BZPCyDtyM7RPngg%3D4%3DVeBkhYSeBCh9g%40mail.gmail.com.

[Greg Thompson]

That's such a good idea that we're already doing it! We are shipping release builds with DCHECKs enabled to a fraction of the canary channel.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SAOgscWb8xC_vsxCBgMO5uCzaM9p2hcpB6rh7nS7kFcRg%40mail.gmail.com.

[Nico Weber]

That's something else though: That sometimes turns DCHECKs into CHECKs. Alexei means turning NOTREACHED into CHECK not DCHECK _always_ (so that it's noreturn in all build configs). I think it'd be awesome to do that, but I also expect it to be involved.

[Wez]

D'oh! Yeah, NOTREACHED() doesn't get the treatment in Release, sadly.

We could switch it to IMMEDIATE_CRASH() in Release but as Nico suggests, I suspect that may be a little involved to make stable...

[Greg Thompson]

I see the current project as a necessary first step. If we don't shake them out slowly, we'll have a very crashy browser, Indeed. 

[Nico Weber]

It's sufficient, but it isn't necessary, since not every DCHECK is a NOTREACHED :-)

[dan...@chromium.org]

On Thu, Jun 7, 2018 at 11:27 AM Wez <w...@chromium.org> wrote:

D'oh! Yeah, NOTREACHED() doesn't get the treatment in Release, sadly.

We could switch it to IMMEDIATE_CRASH() in Release but as Nico suggests, I suspect that may be a little involved to make stable...

Why do we want to treat NOTREACHED() differently than DCHECK()? We don't recomment writing CHECKs unless its a security issue, tracking down a bug, etc. DCHECKs document that something won't happen, just the same as NOTREACHED() does.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALekkJfQy-ha-GVu_FY4hFT0Lx-5E_M3dQ3RzTY4sPqq86P%3D8g%40mail.gmail.com.

[Nico Weber]

On Thu, Jun 7, 2018 at 11:33 AM, <dan...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:27 AM Wez <w...@chromium.org> wrote:

D'oh! Yeah, NOTREACHED() doesn't get the treatment in Release, sadly.

We could switch it to IMMEDIATE_CRASH() in Release but as Nico suggests, I suspect that may be a little involved to make stable...

Why do we want to treat NOTREACHED() differently than DCHECK()? We don't recomment writing CHECKs unless its a security issue, tracking down a bug, etc. DCHECKs document that something won't happen, just the same as NOTREACHED() does.

Because then you can mark it noreturn, which is kind of nice.

[dan...@chromium.org]

On Thu, Jun 7, 2018 at 11:34 AM Nico Weber <tha...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:33 AM, <dan...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:27 AM Wez <w...@chromium.org> wrote:

D'oh! Yeah, NOTREACHED() doesn't get the treatment in Release, sadly.

We could switch it to IMMEDIATE_CRASH() in Release but as Nico suggests, I suspect that may be a little involved to make stable...

Why do we want to treat NOTREACHED() differently than DCHECK()? We don't recomment writing CHECKs unless its a security issue, tracking down a bug, etc. DCHECKs document that something won't happen, just the same as NOTREACHED() does.

Because then you can mark it noreturn, which is kind of nice.

I see. Would you expect the binary size delta to be insignificant?

[Nico Weber]

On Thu, Jun 7, 2018 at 11:35 AM, <dan...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:34 AM Nico Weber <tha...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:33 AM, <dan...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:27 AM Wez <w...@chromium.org> wrote:

D'oh! Yeah, NOTREACHED() doesn't get the treatment in Release, sadly.

We could switch it to IMMEDIATE_CRASH() in Release but as Nico suggests, I suspect that may be a little involved to make stable...

Why do we want to treat NOTREACHED() differently than DCHECK()? We don't recomment writing CHECKs unless its a security issue, tracking down a bug, etc. DCHECKs document that something won't happen, just the same as NOTREACHED() does.

Because then you can mark it noreturn, which is kind of nice.

I see. Would you expect the binary size delta to be insignificant?

I'd say this is more about developer ergonomics than binary size, yes.

Oh, you probably "do you expect binary size will go up a lot if we ship CHECKs everywhere we have NOTREACHEDs", not "do you expect binary size to go down since the compiler can optimize more" :-)

Currently, `NOTREACHED(); return val;` moves val into eax and jumps to the function exit point. CHECK() is something like `int3; ud2; push __COUNTER` which can be done in 5 bytes, so that might be a bit less than the mov and the jmp, or at least not a ton more. We have ~6000 NOTREACHED()s, so I think it's going to be a wash.

[Wez]

On Thu, 7 Jun 2018 at 08:42, Nico Weber <tha...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:35 AM, <dan...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:34 AM Nico Weber <tha...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:33 AM, <dan...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:27 AM Wez <w...@chromium.org> wrote:

D'oh! Yeah, NOTREACHED() doesn't get the treatment in Release, sadly.

We could switch it to IMMEDIATE_CRASH() in Release but as Nico suggests, I suspect that may be a little involved to make stable...

Why do we want to treat NOTREACHED() differently than DCHECK()? We don't recomment writing CHECKs unless its a security issue, tracking down a bug, etc. DCHECKs document that something won't happen, just the same as NOTREACHED() does.

Because then you can mark it noreturn, which is kind of nice.

I see. Would you expect the binary size delta to be insignificant?

I'd say this is more about developer ergonomics than binary size, yes.

Oh, you probably "do you expect binary size will go up a lot if we ship CHECKs everywhere we have NOTREACHEDs", not "do you expect binary size to go down since the compiler can optimize more" :-)

Currently, `NOTREACHED(); return val;` moves val into eax and jumps to the function exit point. CHECK() is something like `int3; ud2; push __COUNTER` which can be done in 5 bytes, so that might be a bit less than the mov and the jmp, or at least not a ton more. We have ~6000 NOTREACHED()s, so I think it's going to be a wash.

I'd expect that the binary size will be more-or-less the same, or possibly slightly smaller.

However, rather than speculate, I'll do a Release build with NOTREACHED set to IMMEDIATE_CRASH and report back here. :)

[Peter Kasting]

On Thu, Jun 7, 2018 at 7:26 AM Devlin Cronin <rdevlin...@chromium.org> wrote:

I haven't done a sophisticated grep yet, but just skimming through codesearch for NOTREACHED(), there seem to be many instances of `if (<some_condition>) { NOTREACHED(); return [x]; }`

If anyone ever happens to run across this pattern in code, a helpful thing to do is to change to DCHECK(!some_condition), eliminating the return.  This never introduces bugs; it can cause new manifestations of failures, but if they fail, the old code was broken too, and the fault does not lie with the person making this change.  Instead, now the failure mode of the old code is clear and can be corrected, either by handling the error or fixing the violated assumption up the stack.

Ultimately it'd be good to get to the point where NOTREACHED() is only ever used for cases like "control shouldn't have escaped the above switch statement", but no one person can make that happen on their own.

PK

[Daniel Bratell]

On Thu, 07 Jun 2018 18:24:57 +0200, Wez <w...@chromium.org> wrote:

On Thu, 7 Jun 2018 at 08:42, Nico Weber <tha...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:35 AM, <dan...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:34 AM Nico Weber <tha...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:33 AM, <dan...@chromium.org> wrote:

On Thu, Jun 7, 2018 at 11:27 AM Wez <w...@chromium.org> wrote:

D'oh! Yeah, NOTREACHED() doesn't get the treatment in Release, sadly.

We could switch it to IMMEDIATE_CRASH() in Release but as Nico suggests, I suspect that may be a little involved to make stable...

Why do we want to treat NOTREACHED() differently than DCHECK()? We don't recomment writing CHECKs unless its a security issue, tracking down a bug, etc. DCHECKs document that something won't happen, just the same as NOTREACHED() does.

Because then you can mark it noreturn, which is kind of nice.

I see. Would you expect the binary size delta to be insignificant?

I'd say this is more about developer ergonomics than binary size, yes.

Oh, you probably "do you expect binary size will go up a lot if we ship CHECKs everywhere we have NOTREACHEDs", not "do you expect binary size to go down since the compiler can optimize more" :-)

Currently, `NOTREACHED(); return val;` moves val into eax and jumps to the function exit point. CHECK() is something like `int3; ud2; push __COUNTER` which can be done in 5 bytes, so that might be a bit less than the mov and the jmp, or at least not a ton more. We have ~6000 NOTREACHED()s, so I think it's going to be a wash.

I'd expect that the binary size will be more-or-less the same, or possibly slightly smaller.

However, rather than speculate, I'll do a Release build with NOTREACHED set to IMMEDIATE_CRASH and report back here. :)

I looked at the macro now and can't help wondering why NOTREACHED doesn't do __builtin_unreachable() to tell the compiler that the code is unreachable? Is it to avoid undefined behaviour?

/Daniel

--

/* Opera Software, Linköping, Sweden: CEST (UTC+2) */

[Wez]

Daniel,

Yes, the implementation allows for the possibility that the code actually _is_ reachable, albeit unintentionally, in which case in Release builds we need to not hit the undefined behaviour Nico mentioned.  Related to that, it actually compiles to a log-only (i.e non-fatal) code snippet on ChromeOS, so that would have to be changed to allow the unreachable annotation, at least.

Wez

[Jakob Kummerow]

In V8, the UNREACHABLE() macro (which is the equivalent of Chromium's NOTREACHED()) has been a fatal crash and marked [[noreturn]] even in Release builds for a while now. We want the semantics to be "we believe this code to be 100% unreachable, and if someone on the internet does manage to reach it, we would like to get a crash report please", rather than just "our test suite currently doesn't reach this, so we think it probably doesn't matter what the code would do if reached, and if someone does reach it, just happily carry on and execute something that may or may not cause corruption of data or logic later on". As far as I remember, the switch was fairly painless, but of course that might be different for Chromium.

So I'm +1 to making NOTREACHED a CHECK eventually.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALekkJduMt-J5%3DfyFZxP2%2Bo73_vEDaEe%3Dw%3Dgm7mJkRx2dOCXrw%40mail.gmail.com.

[Daniel Bratell]

So basically UNREACHABLE() adds intentionally unreachable code. This will make -Wunreachable-code pretty useless (though gcc seems to have dropped that option a while back). I much prefer compile time diagnostics to crash reports.

/Daniel

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKSzg3QG1gT%3DPkjJ5F8k0eNfX2WY0yUP6njgUAJ-3fpNQ%3D%2Bm3g%40mail.gmail.com.

[Nico Weber]

On Fri, Jun 8, 2018 at 5:17 AM, Daniel Bratell <bra...@opera.com> wrote:

So basically UNREACHABLE() adds intentionally unreachable code. This will make -Wunreachable-code pretty useless (though gcc seems to have dropped that option a while back). I much prefer compile time diagnostics to crash reports.

Huh? On the contrary, -Wunreachable-code can infer that things after UNREACHABLE are unreachable in v8, which makes it more useful there.

 

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKSzg3QG1gT%3DPkjJ5F8k0eNfX2WY0yUP6njgUAJ-3fpNQ%3D%2Bm3g%40mail.gmail.com.

[Daniel Bratell]

On Fri, 08 Jun 2018 15:10:21 +0200, Nico Weber <tha...@chromium.org> wrote:

On Fri, Jun 8, 2018 at 5:17 AM, Daniel Bratell <bra...@opera.com> wrote:

So basically UNREACHABLE() adds intentionally unreachable code. This will make -Wunreachable-code pretty useless (though gcc seems to have dropped that option a while back). I much prefer compile time diagnostics to crash reports.

Huh? On the contrary, -Wunreachable-code can infer that things after UNREACHABLE are unreachable in v8, which makes it more useful there.

This is weird. Using this as a test:

#define UNREACHABLE() log_and_crash()

extern void log_and_crash();

int foo() {
return 1;
UNREACHABLE();
}

int bar() {
return 1;
log_and_crash();
}

-Wunreachable-code will warn for |bar| but not for |foo|, despite them having identical code. So -Wunreachable-code will not warn if the unreachable code epxanded from a macro?

bratell@bratell-linux:~/a/path/clang -O3 -Wunreachable-code -Wall -Wextra -c testunreachable.cc 
testunreachable.cc:12:3: warning: code will never be executed [-Wunreachable-code]
log_and_crash();
^~~~~~~~~~~~~
1 warning generated.

If that is how -Wunreachable-code works, then I guess my concern is not applicable since NOTREACHED is a macro.

/Daniel

Won't it also infer that the CHECK itself (the call to logging) will be unreachable?

 

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKSzg3QG1gT%3DPkjJ5F8k0eNfX2WY0yUP6njgUAJ-3fpNQ%3D%2Bm3g%40mail.gmail.com.

--

/* Opera Software, Linköping, Sweden: CEST (UTC+2) */

[Jakob Kummerow]

The interesting case is not UNREACHABLE-after-return, but the other way round, e.g.:

if (foo) { 

  ...

} else if (bar) {

  ...

} else {

  UNREACHABLE();

}

or the variant:

switch (foo) {

case A: return 1;

case B: return 2;

}

UNREACHABLE();

// return 0;  // not needed thanks to [[noreturn]]

Of course there are special cases where the compiler shouldn't need any manual annotations (e.g. a switch over an enum that handles all cases); UNREACHABLE() is most useful when the reasoning why a branch is unreachable is non-trivial / non-local.

[Wez]

Daniel, Jakob,

Thanks for the discussion. I had no idea that code we never run would excite people to this degree.

To clarify the current Chromium situation:

- IMMEDIATE_CRASH() and TerminateCurrentProcessImmediately() are [[noreturn]], in effect[1].

- NOTREACHED() is defined as:

  - LOG(ERROR) << "NOTREACHED() hit" in ChromeOS non-DCHECK/Debug builds.

  - DCHECK(false) otherwise (i.e. crash in DCHECK/Debug, noop otherwise).

If we want to have NOTREACHED() be [[noreturn]] then it becomes a synonym for IMMEDIATE_CRASH(), perhaps with a preceding log-line if DCHECK_IS_ON().

Then there are three things to address:

1. Stability of ChromeOS release binaries.

2. Cleanup of call-sites which stream additional logging to NOTREACHED(), since we can't [[noreturn]] a logging stream. ;)

3. Cleanup of call-sites which fail due to now-unreachable code, e.g. we have a lot of NOTREACHED(); FALLTHROUGH(); pairs in switch cases.

Unless I hear objections, I'll file a bug to track this work later today.

Wez

[1] IMMEDIATE_CRASH() is a crash followed by __builtin_unreachable(), technically.

[Wez]

OK, I've filed a bug for this at https://bugs.chromium.org/p/chromium/issues/detail?id=851128.