Bug 159120

Summary: B3 should not use Nops when deleting unreachable code
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, commit-queue, ggaren, keith_miller, mark.lam, mhahnenb, msaboff, oliver, saam, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch
none
the patch msaboff: review+

Description Filip Pizlo 2016-06-25 13:55:52 PDT
Replacing an unreachable value with Nop is bad if the value is non-Void, since other unreachable code will now reference a Void value.  This can easily happen because deleting unreachable code is often an iterative process where you may not discover that the users of an unreachable value are unreachable until after you have already tried to do other things to those unreachable values.
Comment 1 Filip Pizlo 2016-06-25 14:07:38 PDT
Created attachment 282083 [details]
the patch
Comment 2 WebKit Commit Bot 2016-06-25 14:10:01 PDT
Attachment 282083 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:12064:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:12078:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3InsertionSet.h:88:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2016-06-25 17:58:29 PDT
Created attachment 282084 [details]
the patch

Better patch.
Comment 4 WebKit Commit Bot 2016-06-25 17:59:47 PDT
Attachment 282084 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:12064:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:12078:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Michael Saboff 2016-06-27 09:46:42 PDT
Comment on attachment 282084 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=282084&action=review

r=me

> Source/JavaScriptCore/ChangeLog:13
> +        - replaceWithNop() if the type is Void or if know that we'll fix any users of this value.

Change "if know" to "if we know".

> Source/JavaScriptCore/b3/B3Value.h:96
> +    // method less you kill a value safely. It will replace Void values with Nop and non-Void

"less" => "lets"
Comment 6 Filip Pizlo 2016-06-27 11:35:56 PDT
Landed in https://trac.webkit.org/changeset/202502