Summary: | Update udis86 to a more recent version | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brendan Long <b.long> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||||
Severity: | Normal | CC: | b.long, buildbot, fpizlo, mhahnenberg, rniwa | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Linux | ||||||||||||||
Attachments: |
|
Description
Brendan Long
2013-06-04 10:12:44 PDT
Created attachment 203721 [details]
Patch
This definitely won't pass the style tests, because udis86's style is very very different than WebKit's. udis86_udint.h needs to be added to the Xcode project. I assume this patch will fail until that's done. I didn't make a lot of the changes that were made last time. The printf and output directory changes exist upstream now, and the other changes didn't seem necessary. I cc'd the person who did this last time because I'm not sure which of the changes made were necessary. (In reply to comment #3) > I cc'd the person who did this last time because I'm not sure which of the changes made were necessary. I mean, are still necessary. (In reply to comment #0) > The version of udis86 included in WebKit prints format string warnings every time I build because it uses %llx for uint64_t, but on 64-bit Linux, it should be %lx. This is fixed[1] in upstream udis86, it just needs to be updated. > > [1] https://github.com/vmt/udis86/commit/3d7198678c3c95aa49bfc2feda6a1106dc87810f Any reason why you didn't just do that fix? (In reply to comment #5) > Any reason why you didn't just do that fix? Because there's almost a year of other upstream improvements too, so I figured getting all the way up to date was preferable to just taking the one change. (In reply to comment #6) > (In reply to comment #5) > > Any reason why you didn't just do that fix? > > Because there's almost a year of other upstream improvements too, so I figured getting all the way up to date was preferable to just taking the one change. I can appreciate that, but I just want to make sure that these changes are worthwhile to webkit. We use udis86 just for disassembling code that we generate, and our code generator for x86 uses a fairly small subset of x86. Do you know what kinds of improvements have been made? (In reply to comment #7) > Do you know what kinds of improvements have been made? The improvements I can see (not including new instructions) are: * Fixed printf warnings. * Various fixes to instructions: * Make mov (c7 /reg=0) sign-extend the immediate operand. * Add missing mod == 3 check in decode.c:decode_operand OP_R case. * Correct loopnz to loopne, setnb to setae * Fix cmp 83 /reg=7 for imm sign extension * remove modification of const string. * etc. * Most of the changes you needed to make in differences.txt are fixed upstream, so anyone who is familiar with udis86 won't be surprised by the version in WebKit. If you're not interested in this, I can try to just incorporate the one fix. (In reply to comment #8) > (In reply to comment #7) > > Do you know what kinds of improvements have been made? > > The improvements I can see (not including new instructions) are: > > * Fixed printf warnings. > * Various fixes to instructions: > * Make mov (c7 /reg=0) sign-extend the immediate operand. > * Add missing mod == 3 check in decode.c:decode_operand OP_R case. > * Correct loopnz to loopne, setnb to setae > * Fix cmp 83 /reg=7 for imm sign extension > * remove modification of const string. > * etc. > * Most of the changes you needed to make in differences.txt are fixed upstream, so anyone who is familiar with udis86 won't be surprised by the version in WebKit. > > If you're not interested in this, I can try to just incorporate the one fix. No no, I just wanted to make sure this made sense. It seems like it does. Let's wait for the bots before landing. Have you run some spot tests on x86-32 and x86-64 to make sure that the output makes sense? It might be good to post the disassembly of some representative code on both 32-bit and 64-bit just so that we can be sure that we don't land a lemon. Apparently getting this to build in Xcode is a lot harder than I expected, so this may take some time. Created attachment 203857 [details]
jsc profile output with patch
This what I get if I run:
WebKitBuild/Debug/bin/jsc PerformanceTests/SunSpider/tests/sunspider-1.0/crypto-aes.js -p with-patch.json
I found it useful to pretty-print it with Python and then get a diff against the before-patch version (I'll upload that too):
cat with-patch.json | python3 -mjson.tool > with-patch-pretty.json
cat without-patch.json | python3 -mjson.tool > without-patch-pretty.json
diff -u without-patch-pretty.json with-patch-pretty.json
It's still pretty long, but it looks like the differences are just because the memory locations aren't deterministic.
Created attachment 203859 [details]
jsc profile output without patch
Created attachment 203860 [details]
Patch
My Mac builds works now, but I get this error at runtime: Current executable set to 'WebKitBuild/Debug/JavaScriptCore.framework/Resources/jsc' (x86_64). (lldb) run Process 37326 launched: '/Users/blong/workspace/webkit/WebKitBuild/Debug/JavaScriptCore.framework/Resources/jsc' (x86_64) dyld: Symbol not found: __ZN3JSC13PropertyTable6s_infoE Referenced from: /Users/blong/workspace/webkit/WebKitBuild/Debug/JavaScriptCore.framework/Resources/jsc Expected in: /System/Library/Frameworks/JavaScriptCore.framework/Versions/A/JavaScriptCore in /Users/blong/workspace/webkit/WebKitBuild/Debug/JavaScriptCore.framework/Resources/jsc Process 37326 stopped * thread #1: tid = 0x1f03, 0x00007fff5fc0106d dyld`dyld_fatal_error + 1, stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0) frame #0: 0x00007fff5fc0106d dyld`dyld_fatal_error + 1 dyld`dyld_fatal_error + 1: -> 0x7fff5fc0106d: nop dyld`dyldbootstrap::start(macho_header const*, int, char const**, long, macho_header const*): 0x7fff5fc0106e: pushq %rbp 0x7fff5fc0106f: movq %rsp, %rbp 0x7fff5fc01072: pushq %r15 It doesn't seem related to the patch though, because I get the same error if I build the commit before that. I'm not really sure what's wrong with my environment, but is there any chance you can test this on yours? The patch works for me with QtWebKit on Ubuntu. Most of the changes I made to make this build have been applied upstream: https://github.com/vmt/udis86/pull/42 Comment on attachment 203860 [details] Patch Attachment 203860 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/779139 New failing tests: editing/pasteboard/pasting-empty-html-falls-back-to-text.html editing/pasteboard/copy-without-selection.html editing/pasteboard/copy-two-pasteboard-types-both-work.html Created attachment 203881 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
I tried doing a clean build of WebKit on my Mac and it still crashes immediately. The EWS builds work though, so it's definitely something weird with my machine. Maybe something about the recent Safari update? I'll investigate this more when I have time. (In reply to comment #18) > I tried doing a clean build of WebKit on my Mac and it still crashes immediately. The EWS builds work though, so it's definitely something weird with my machine. Maybe something about the recent Safari update? I'll investigate this more when I have time. Any status on this? Getting the latest and greatest disassembler would be awesome :-) (In reply to comment #19) > Any status on this? Getting the latest and greatest disassembler would be awesome :-) I was originally working on this because it was making the QtWebKit build significantly more verbose (when compiling with --makeargs="-s"). Now that QtWebKit doesn't exist, it's not really important to me. Updating it wasn't particularly difficult, but I don't really know how to test it, since I don't use this feature. > I was originally working on this because it was making the QtWebKit build significantly more verbose (when compiling with --makeargs="-s"). Now that QtWebKit doesn't exist, it's not really important to me. > > Updating it wasn't particularly difficult, but I don't really know how to test it, since I don't use this feature. Fair enough. There were some rumblings about moving over to the llvm disassembler so I'll close this bug and file bug 123762. Comment on attachment 203860 [details]
Patch
Clearing the review flag.
|