Add __builtin_unreachable() to CRASH() when using Clang on Windows.

Add __builtin_unreachable() to CRASH() when using Clang on Windows.

Currently, when building with Clang on Windows, NO_RETURN_DUE_TO_CRASH
is enabled, but CRASH is defined as __debugbreak, which Clang doesn't
recognise as a call that will never return.  This causes
-Winvalid-noreturn to trigger warnings.  This CL changes CRASH() to also
include __builtin_unreachable() when using Clang on Windows.

BUG=504698
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198250

[Sam McNally, 2015-07-02 02:49:33]

sammc@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-07-02 04:54:37]

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h
File Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h#new...
Source/wtf/Assertions.h:130: #if COMPILER(GCC)
Isn't it a better fix to change this to COMIPLER(GCC) || COMIPLER(CLANG)
instead?

[Sam McNally, 2015-07-02 05:09:41]

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h
File Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h#new...
Source/wtf/Assertions.h:130: #if COMPILER(GCC)
On 2015/07/02 04:54:37, Nico wrote:
> Isn't it a better fix to change this to COMIPLER(GCC) || COMIPLER(CLANG)
> instead?

I'm not sure why, but doing that instead doesn't fix the warning:

../../third_party/WebKit/Source\wtf/CheckedArithmetic.h(80,5) :  error: function
declared 'noreturn' should not return [-Werror,-Winvalid-noreturn]
    }
    ^

[Nico, 2015-07-02 05:22:36]

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h
File Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h#new...
Source/wtf/Assertions.h:130: #if COMPILER(GCC)
On 2015/07/02 05:09:40, Sam McNally wrote:
> On 2015/07/02 04:54:37, Nico wrote:
> > Isn't it a better fix to change this to COMIPLER(GCC) || COMIPLER(CLANG)
> > instead?
> 
> I'm not sure why, but doing that instead doesn't fix the warning:
> 
> ../../third_party/WebKit/Source\wtf/CheckedArithmetic.h(80,5) :  error:
function
> declared 'noreturn' should not return [-Werror,-Winvalid-noreturn]
>     }
>     ^

Hm, strange. Seems to work in a reduced example:

Nicos-MacBook-Pro-2:llvm-build thakis$ cat test.cc
__declspec(noreturn) void f() {
}

__declspec(noreturn) void g() {
  __builtin_trap();
}
Nicos-MacBook-Pro-2:llvm-build thakis$ bin/clang-cl -c test.cc
test.cc(2,1) :  warning: function declared 'noreturn' should not return
[-Winvalid-noreturn]
}
^
1 warning generated.

I wonder if a header in a different project defines IMMEDIATE_CRASH to something
else…but it looks like blink is the only project defining this:
https://code.google.com/p/chromium/codesearch#search/&q=%5Cbimmediate_crash%5...

Can you build that one translation unit with `ninja ... -v -d keeprsp`, then
copy the compile line and paste it with `/P /Fiout.txt` appended (cd into
out\Debug first) and then look at out.txt to see what the macros expand to?

[Sam McNally, 2015-07-02 06:01:13]

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h
File Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h#new...
Source/wtf/Assertions.h:130: #if COMPILER(GCC)
On 2015/07/02 05:22:36, Nico wrote:
> On 2015/07/02 05:09:40, Sam McNally wrote:
> > On 2015/07/02 04:54:37, Nico wrote:
> > > Isn't it a better fix to change this to COMIPLER(GCC) || COMIPLER(CLANG)
> > > instead?
> > 
> > I'm not sure why, but doing that instead doesn't fix the warning:
> > 
> > ../../third_party/WebKit/Source\wtf/CheckedArithmetic.h(80,5) :  error:
> function
> > declared 'noreturn' should not return [-Werror,-Winvalid-noreturn]
> >     }
> >     ^
> 
> Hm, strange. Seems to work in a reduced example:
> 
> Nicos-MacBook-Pro-2:llvm-build thakis$ cat test.cc
> __declspec(noreturn) void f() {
> }
> 
> __declspec(noreturn) void g() {
>   __builtin_trap();
> }
> Nicos-MacBook-Pro-2:llvm-build thakis$ bin/clang-cl -c test.cc
> test.cc(2,1) :  warning: function declared 'noreturn' should not return
> [-Winvalid-noreturn]
> }
> ^
> 1 warning generated.
> 
> I wonder if a header in a different project defines IMMEDIATE_CRASH to
something
> else…but it looks like blink is the only project defining this:
>
https://code.google.com/p/chromium/codesearch#search/&q=%5Cbimmediate_crash%5...
> 
> Can you build that one translation unit with `ninja ... -v -d keeprsp`, then
> copy the compile line and paste it with `/P /Fiout.txt` appended (cd into
> out\Debug first) and then look at out.txt to see what the macros expand to?

It turns out that CRASH is defined as __debugbreak on windows
(https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...)
so IMMEDIATE_CRASH() isn't involved here.

[Sam McNally, 2015-07-03 01:16:05]

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h
File Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1207323004/diff/1/Source/wtf/Assertions.h#new...
Source/wtf/Assertions.h:130: #if COMPILER(GCC)
On 2015/07/02 06:01:12, Sam McNally wrote:
> On 2015/07/02 05:22:36, Nico wrote:
> > On 2015/07/02 05:09:40, Sam McNally wrote:
> > > On 2015/07/02 04:54:37, Nico wrote:
> > > > Isn't it a better fix to change this to COMIPLER(GCC) || COMIPLER(CLANG)
> > > > instead?
> > > 
> > > I'm not sure why, but doing that instead doesn't fix the warning:
> > > 
> > > ../../third_party/WebKit/Source\wtf/CheckedArithmetic.h(80,5) :  error:
> > function
> > > declared 'noreturn' should not return [-Werror,-Winvalid-noreturn]
> > >     }
> > >     ^
> > 
> > Hm, strange. Seems to work in a reduced example:
> > 
> > Nicos-MacBook-Pro-2:llvm-build thakis$ cat test.cc
> > __declspec(noreturn) void f() {
> > }
> > 
> > __declspec(noreturn) void g() {
> >   __builtin_trap();
> > }
> > Nicos-MacBook-Pro-2:llvm-build thakis$ bin/clang-cl -c test.cc
> > test.cc(2,1) :  warning: function declared 'noreturn' should not return
> > [-Winvalid-noreturn]
> > }
> > ^
> > 1 warning generated.
> > 
> > I wonder if a header in a different project defines IMMEDIATE_CRASH to
> something
> > else…but it looks like blink is the only project defining this:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%5Cbimmediate_crash%5...
> > 
> > Can you build that one translation unit with `ninja ... -v -d keeprsp`, then
> > copy the compile line and paste it with `/P /Fiout.txt` appended (cd into
> > out\Debug first) and then look at out.txt to see what the macros expand to?
> 
> It turns out that CRASH is defined as __debugbreak on windows
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...)
> so IMMEDIATE_CRASH() isn't involved here.

I've changed this to add a __builtin_unreachable() to the MSVC version of
CRASH() when COMPILER(CLANG) and moved it into Assertion.h instead of doing it
in the gn/gyp. WDYT?

[Nico, 2015-07-03 01:33:19]

lgtm, I think this is a pretty nice way to go about this.

rnk, should we make __debugbreak noreturn? Not really, right?

I wonder if we should make this `_debugbreak(); _exit(1);` on windows
independent of the compiler.

[Sam McNally, 2015-07-03 01:36:09]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2015-07-03 01:36:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207323004/20001

[blink-reviews, 2015-07-03 01:41:32]

On Thu, Jul 2, 2015 at 6:33 PM, <thakis@chromium.org> wrote:

> rnk, should we make __debugbreak noreturn? Not really, right?
>

We shouldn't, people (like me :) use it to insert break points at the
source level without having to interact with the debugger.

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

[commit-bot: I haz the power, 2015-07-03 02:49:32]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198250