Bug 142997 – JSC should have a low-cost asynchronous disassembler

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

rdar://problem/20195217

[Filip Pizlo]

Created attachment 249310 [details]
the patch

[WebKit Commit Bot]

Attachment 249310 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:327:  Wrong number of spaces before statement. (expected: 9)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:346:  Wrong number of spaces before statement. (expected: 9)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:349:  Wrong number of spaces before statement. (expected: 9)  [whitespace/indent] [4]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 249310 [details]
the patch

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

r=me with issues resolved.

> Source/JavaScriptCore/disassembler/Disassembler.cpp:65
> +    char* header { nullptr };

I think this should be a std::unique_ptr<char>.  I see it being initialized with a strdup below but not cleaned up anywhere.

> Source/JavaScriptCore/ftl/FTLCompile.cpp:825
> +                }
> +                

You need to dataLog(header) here to achieve equivalence for the old showDisassembly.

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 249310 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249310&action=review
> 
> r=me with issues resolved.
> 
> > Source/JavaScriptCore/disassembler/Disassembler.cpp:65
> > +    char* header { nullptr };
> 
> I think this should be a std::unique_ptr<char>.  I see it being initialized
> with a strdup below but not cleaned up anywhere.

The destructor frees it. I believe that it cannot be a unique_ptr since that won't call free(). Since I use strdup() to create it, I need to use free() to free it.

> 
> > Source/JavaScriptCore/ftl/FTLCompile.cpp:825
> > +                }
> > +                
> 
> You need to dataLog(header) here to achieve equivalence for the old
> showDisassembly.

Good catch! Fixed.

[Mark Lam]

Comment on attachment 249310 [details]
the patch

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

>>> Source/JavaScriptCore/disassembler/Disassembler.cpp:65
>>> +    char* header { nullptr };
>> 
>> I think this should be a std::unique_ptr<char>.  I see it being initialized with a strdup below but not cleaned up anywhere.
> 
> The destructor frees it. I believe that it cannot be a unique_ptr since that won't call free(). Since I use strdup() to create it, I need to use free() to free it.

I missed the free() call in the destructor.  Looks good.

[Filip Pizlo]

Created attachment 249312 [details]
patch for landing

[WebKit Commit Bot]

Attachment 249312 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:327:  Wrong number of spaces before statement. (expected: 9)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:346:  Wrong number of spaces before statement. (expected: 9)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:349:  Wrong number of spaces before statement. (expected: 9)  [whitespace/indent] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/181887