Bug 117205 – Update udis86 to a more recent version

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Brendan Long]

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

[Brendan Long]

Created attachment 203721 [details]
Patch

[Brendan Long]

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.

[Brendan Long]

I cc'd the person who did this last time because I'm not sure which of the changes made were necessary.

[Brendan Long]

(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.

[Filip Pizlo]

(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?

[Brendan Long]

(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.

[Filip Pizlo]

(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?

[Brendan Long]

(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.

[Filip Pizlo]

(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.

[Brendan Long]

Apparently getting this to build in Xcode is a lot harder than I expected, so this may take some time.

[Brendan Long]

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.

[Brendan Long]

Created attachment 203859 [details]
jsc profile output without patch

[Brendan Long]

Created attachment 203860 [details]
Patch

[Brendan Long]

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.

[Brendan Long]

Most of the changes I made to make this build have been applied upstream:

https://github.com/vmt/udis86/pull/42

[Build Bot]

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

[Build Bot]

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

[Brendan Long]

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.

[Mark Hahnenberg]

(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 :-)

[Brendan Long]

(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.

[Mark Hahnenberg]

> 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.

[Zan Dobersek]

Comment on attachment 203860 [details]
Patch

Clearing the review flag.