| Summary: | JSC should have a low-cost asynchronous disassembler | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||
| Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | barraclough, benjamin, cmarcelo, commit-queue, ggaren, mark.lam, mhahnenb, mmirman, msaboff, nrotem, oliver, saam, sam | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Filip Pizlo
2015-03-23 21:23:11 PDT
Created attachment 249310 [details]
the patch
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.
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. (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. 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. Created attachment 249312 [details]
patch for landing
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.
Landed in http://trac.webkit.org/changeset/181887 |