WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
150842
Don't use unreachable returns to suppress GCC warning
https://bugs.webkit.org/show_bug.cgi?id=150842
Summary
Don't use unreachable returns to suppress GCC warning
Csaba Osztrogonác
Reported
2015-11-03 10:21:57 PST
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?
Attachments
Add attachment
proposed patch, testcase, etc.
Filip Pizlo
Comment 1
2015-11-03 10:44:25 PST
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
Comment 2
2015-11-03 10:56:16 PST
OK, let's do it everywhere. ;)
Csaba Osztrogonác
Comment 3
2015-11-04 03:03:09 PST
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
Comment 4
2015-11-04 03:28:03 PST
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
Comment 5
2015-11-12 04:17:49 PST
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
Comment 6
2015-11-12 09:37:06 PST
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
Comment 7
2016-01-26 03:52:23 PST
After
https://trac.webkit.org/changeset/194858
and
bug153426
it isn't blocker for enabling B3 JIT on Linux.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug