Don't use unreachable returns to suppress GCC warning

[Csaba Osztrogonác]

https://bugs.webkit.org/show_bug.cgi?id=150794#c7

Some ideas:
- use strongly typed enums
- suppress warnings with pragma, because the design guarantees these are false positive warnings
- ???

any better idea?

[Filip Pizlo]

I think it would be useful to come up with a good solution that can be used elsewhere in WebKit.

We have a lot of code like:

inline int superFastThing()
{
    switch (someEnum) {
    all cases:
        fast things
    }
    RELEASE_ASSERT_NOT_REACHED();
    return 0;
}

Microoptimizations are important.  I think if we removed the assert path from all such fast inline functions, it would be a meaningful improvement.

[Csaba Osztrogonác]

OK, let's do it everywhere. ;)

[Csaba Osztrogonác]

It is believed that this warning is valid and not a bug in GCC:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53479

#include <assert.h>

enum class Foo
{
    Bar,
    Baz
};

bool doFoo(Foo foo)
{
    switch(foo)
    {
    case Foo::Bar:
    case Foo::Baz:
        return true;
    }
}

int main()
{
    Foo f = Foo(2);
    assert(doFoo(f)); <======== undefined behaviour !!!
}


So what do you suggest? Should we suppress this warning for these functions
with the statement: "Trust me, I really promise that I won't call this
function with out-of-range argument."?

[Csaba Osztrogonác]

Let's continue the discussion started in bug150794 here:

(In reply to comment #9)
> 
> I just wrote this program, and compiled it with clang -O3.
> 
> #include <stdlib.h>
> 
> int foo(int x)
> {
>     switch (x) {
>     case 42:
>         return 23;
>     case 85:
>         return 12;
>     }
> }

[snip]
 
> test.o:
> (__TEXT,__text) section
> _foo:
> 0000000000000000	pushq	%rbp
> 0000000000000001	movq	%rsp, %rbp
> 0000000000000004	cmpl	$0x55, %edi
> 0000000000000007	movl	$0xc, %ecx
> 000000000000000c	movl	$0x17, %eax
> 0000000000000011	cmovel	%ecx, %eax
> 0000000000000014	popq	%rbp
> 0000000000000015	retq
> 0000000000000016	nopw	%cs:(%rax,%rax)

In this case your foo(x) will always return 23 if x is different from 
42 and 85. I tested it with clang and GCC too. With clang O0 the
program crashed on ud2 instruction, with O1,O2,O3 it returned 42.
With GCC O0 the foo(x) returned x, with O1,O2,O3 it returned 0.

It is obviously undefined behaviour, which is not good.

What about using debug only asserts (ASSERT_NOT_REACHED) and suppressing
the warnings for these functions assuming the test coverage is good enough
to catch all possibly errors in debug mode before shipping?

[Csaba Osztrogonác]

Any suggestion for a fool-proof fix which guarantees we don't have undefined
behaviour and don't generate bigger code just for silence the valid warning.

[Darin Adler]

The problem here is a conflict between what “should” be true and can actually be true in practice.

For example, GCC won’t let us write code that incorrectly checks "this" for null, but does nothing to prevent us from using null pointer to call a non-virtual member function by accident. Similarly, it does not guarantee that we don’t get illegal enum values at runtime, but doesn’t allow us to check for that kind of problem at runtime.

Strongly typed enums do not solve this problem.

I don’t currently have a proposal for how to take advantage of the compiler’s knowledge that a code path should be unreachable, but also let us control what our program does when this illegal state somehow occurs.

[Csaba Osztrogonác]

After https://trac.webkit.org/changeset/194858 and 
bug153426 it isn't blocker for enabling B3 JIT on Linux.