Bug 117205 - Update udis86 to a more recent version
Summary: Update udis86 to a more recent version
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-04 10:12 PDT by Brendan Long
Modified: 2013-11-04 23:02 PST (History)
5 users (show)

See Also:


Attachments
Patch (274.84 KB, patch)
2013-06-04 13:33 PDT, Brendan Long
no flags Details | Formatted Diff | Diff
jsc profile output with patch (2.21 MB, application/json)
2013-06-05 09:12 PDT, Brendan Long
no flags Details
jsc profile output without patch (2.21 MB, application/json)
2013-06-05 09:14 PDT, Brendan Long
no flags Details
Patch (278.51 KB, patch)
2013-06-05 09:23 PDT, Brendan Long
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (566.58 KB, application/zip)
2013-06-05 13:51 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brendan Long 2013-06-04 10:12:44 PDT
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
Comment 1 Brendan Long 2013-06-04 13:33:03 PDT
Created attachment 203721 [details]
Patch
Comment 2 Brendan Long 2013-06-04 13:38:49 PDT
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.
Comment 3 Brendan Long 2013-06-04 13:42:02 PDT
I cc'd the person who did this last time because I'm not sure which of the changes made were necessary.
Comment 4 Brendan Long 2013-06-04 13:42:15 PDT
(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.
Comment 5 Filip Pizlo 2013-06-04 13:45:27 PDT
(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?
Comment 6 Brendan Long 2013-06-04 13:50:46 PDT
(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.
Comment 7 Filip Pizlo 2013-06-04 13:57:49 PDT
(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?
Comment 8 Brendan Long 2013-06-04 14:16:17 PDT
(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.
Comment 9 Filip Pizlo 2013-06-04 14:24:14 PDT
(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.
Comment 10 Brendan Long 2013-06-04 15:51:50 PDT
Apparently getting this to build in Xcode is a lot harder than I expected, so this may take some time.
Comment 11 Brendan Long 2013-06-05 09:12:03 PDT
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.
Comment 12 Brendan Long 2013-06-05 09:14:03 PDT
Created attachment 203859 [details]
jsc profile output without patch
Comment 13 Brendan Long 2013-06-05 09:23:06 PDT
Created attachment 203860 [details]
Patch
Comment 14 Brendan Long 2013-06-05 09:31:01 PDT
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.
Comment 15 Brendan Long 2013-06-05 10:51:13 PDT
Most of the changes I made to make this build have been applied upstream:

https://github.com/vmt/udis86/pull/42
Comment 16 Build Bot 2013-06-05 13:51:48 PDT
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
Comment 17 Build Bot 2013-06-05 13:51:50 PDT
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
Comment 18 Brendan Long 2013-06-10 08:38:29 PDT
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.
Comment 19 Mark Hahnenberg 2013-11-04 10:42:45 PST
(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 :-)
Comment 20 Brendan Long 2013-11-04 16:12:42 PST
(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.
Comment 21 Mark Hahnenberg 2013-11-04 16:16:20 PST
> 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 22 Zan Dobersek 2013-11-04 23:02:43 PST
Comment on attachment 203860 [details]
Patch

Clearing the review flag.