Bug 188415 - Yarr JIT should include annotations with dumpDisassembly=true
Summary: Yarr JIT should include annotations with dumpDisassembly=true
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-08 13:46 PDT by Michael Saboff
Modified: 2018-08-08 15:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (36.79 KB, patch)
2018-08-08 14:16 PDT, Michael Saboff
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2018-08-08 13:46:03 PDT
All of the other JSC related JITs intermingle the byte code / IR in the disassembler output.  The Yarr regular expression JIT should also annotate its output with the compiled pattern info.
Comment 1 Michael Saboff 2018-08-08 14:15:46 PDT
<rdar://problem/43063283>
Comment 2 Michael Saboff 2018-08-08 14:16:16 PDT
Created attachment 346794 [details]
Patch
Comment 3 EWS Watchlist 2018-08-08 14:18:43 PDT
Attachment 346794 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/yarr/YarrDisassembler.h:96:  The parameter name "vectorOrder" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/yarr/YarrDisassembler.h:98:  The parameter name "vectorOrder" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Yusuke Suzuki 2018-08-08 15:07:03 PDT
Comment on attachment 346794 [details]
Patch

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

r=me

> Source/JavaScriptCore/yarr/YarrDisassembler.h:77
> +    enum VectorOrder {

enum class?

> Source/JavaScriptCore/yarr/YarrDisassembler.h:109
> +    unsigned m_indentLevel;

Making `m_identLevel { 0 };` looks nice to me instead of initializing it in a constructor.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:45
> +//    friend void jitCompile(VM*, YarrCodeBlock&, const String& pattern, unsigned& numSubpatterns, const char*& error, bool ignoreCase, bool multiline);

Let's remove this.
Comment 5 Michael Saboff 2018-08-08 15:35:01 PDT
(In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 346794 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346794&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/yarr/YarrDisassembler.h:77
> > +    enum VectorOrder {
> 
> enum class?

Done.

> > Source/JavaScriptCore/yarr/YarrDisassembler.h:109
> > +    unsigned m_indentLevel;
> 
> Making `m_identLevel { 0 };` looks nice to me instead of initializing it in
> a constructor.

Done.
 
> > Source/JavaScriptCore/yarr/YarrJIT.cpp:45
> > +//    friend void jitCompile(VM*, YarrCodeBlock&, const String& pattern, unsigned& numSubpatterns, const char*& error, bool ignoreCase, bool multiline);
> 
> Let's remove this.

Removed.  This was a holdover when I was checking if this was dead.
Comment 6 Michael Saboff 2018-08-08 15:42:32 PDT
Committed r234713: <https://trac.webkit.org/changeset/234713>