Bug 123717 - Lacking support on a arm-traditional disassembler.
Summary: Lacking support on a arm-traditional disassembler.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 108645 159408
  Show dependency treegraph
 
Reported: 2013-11-03 18:24 PST by sloanyang
Modified: 2016-12-10 00:04 PST (History)
11 users (show)

See Also:


Attachments
Patch (10.09 KB, patch)
2016-07-29 05:28 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2016-07-29 11:13 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sloanyang 2013-11-03 18:24:27 PST
The arm-traditional assembler generate codes, but it is hard to debug without a dissassembler.
Comment 1 yangqi3 2013-11-03 18:48:23 PST
This might be a problem. We could try to use the arm objdump code to generate dissambler.
Comment 2 Csaba Osztrogonác 2015-02-09 03:16:53 PST
It would be great to have a working disassembler for 
ARM traditional too to be able debug bug141193 easily.

I'm trying to make the LLVM based disassembler work on ARM.
Comment 3 Csaba Osztrogonác 2016-02-18 04:32:52 PST
LLVM dependency was removed from the trunk, now it is a 
bigger task and won't have time for it in the near future.
Comment 4 Csaba Osztrogonác 2016-07-14 05:50:50 PDT
It would be great to have ARM traditional disassembler to
debug bug159408 easily. 

I managed to implement an LLVM based disassembler,
but it is a little bit hacky now. I'm going upstream
it when I finished cleanups.
Comment 5 Csaba Osztrogonác 2016-07-29 05:28:57 PDT
Created attachment 284859 [details]
Patch
Comment 6 Csaba Osztrogonác 2016-07-29 09:43:04 PDT
Comment on attachment 284859 [details]
Patch

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

> Source/JavaScriptCore/disassembler/ARMLLVMDisassembler.cpp:66
> +        out.printf("%s%16s: [0x%08lx] %s\n", prefix, pcString, *(static_cast<unsigned long*>(pc)), instructionString);

I meant reinterpret_cast here, will update.
Comment 7 Csaba Osztrogonác 2016-07-29 11:13:25 PDT
Created attachment 284876 [details]
Patch
Comment 8 Csaba Osztrogonác 2016-08-01 04:32:13 PDT
ping?
Comment 9 Saam Barati 2016-08-01 09:05:42 PDT
I'm the wrong person to review, but it seems OK. I'll let others comment.
Comment 10 Mark Lam 2016-08-01 10:08:47 PDT
Comment on attachment 284876 [details]
Patch

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

I'm also not the best person to review this, but have a few comments and questions below.

Another question: why can't ARM traditional use the ARMv7 disassembler (which we do have implemented)?  Are the differences that great?

> Source/JavaScriptCore/CMakeLists.txt:897
> +    ${LLVM_LIBRARIES}

If it's not too much trouble, is it possible to make this be conditional on the port?  If we want to move towards using the CMake build system for all ports eventually, this seems like a burden on all ports that don't use LLVM.

> Source/JavaScriptCore/disassembler/ARMLLVMDisassembler.cpp:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

I think this should be 2016, not 2013.

> Source/cmake/FindLLVM.cmake:21
> +foreach (_program_name llvm-config llvm-config-3.7 llvm-config-3.6 llvm-config-3.5)
> +    find_program(LLVM_CONFIG_EXE NAMES ${_program_name})
> +    if (LLVM_CONFIG_EXE)
> +        execute_process(COMMAND ${LLVM_CONFIG_EXE} --version OUTPUT_VARIABLE LLVM_VERSION OUTPUT_STRIP_TRAILING_WHITESPACE)
> +        if ("${LLVM_VERSION}" VERSION_LESS "${LLVM_FIND_VERSION}")
> +            unset(LLVM_CONFIG_EXE CACHE)
> +        else ()
> +            break ()
> +        endif ()
> +    endif ()
> +endforeach ()

Naive question: would this work for a cross disassembler?  The llvm library that we want to link to for the disassembler needs to be the one that is for the target arm device.  Presumably, we would be running this makefile on an x86/x86_64 host.  How does find_program knows to find that version here?  It also seem strange to me that we would run the found LLVM_CONFIG_EXE to get its --version info.  What am I not seeing here?
Comment 11 Csaba Osztrogonác 2016-08-02 03:18:26 PDT
Comment on attachment 284876 [details]
Patch

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

>> Source/JavaScriptCore/CMakeLists.txt:897
>> +    ${LLVM_LIBRARIES}
> 
> If it's not too much trouble, is it possible to make this be conditional on the port?  If we want to move towards using the CMake build system for all ports eventually, this seems like a burden on all ports that don't use LLVM.

There is no need to add conditional here. LLVMDisassembler is optional and
disabled by default an can be enabled with setting USE_ARM_LLVM_DISASSEMBLER
in the cmake buildsystem (eg. build-jsc --cmakears=-DUSE_ARM_LLVM_DISASSEMBLER=ON)

If we don't set it, cmake won't call find_package(LLVM REQUIRED)
and this variable is an empty string.

>> Source/JavaScriptCore/disassembler/ARMLLVMDisassembler.cpp:2
>> + * Copyright (C) 2013 Apple Inc. All rights reserved.
> 
> I think this should be 2016, not 2013.

I added back the pre-r196729 LLVMDisassembler.cpp which has 2013 copyright.
I removed many lines and changed some lines to make it ARM traditional
only and dump all instructions in hexadecimal format too.

If we update the copyright, I think we should keep the original 
one and add 2016 with our company, not Apple.

>> Source/cmake/FindLLVM.cmake:21
>> +endforeach ()
> 
> Naive question: would this work for a cross disassembler?  The llvm library that we want to link to for the disassembler needs to be the one that is for the target arm device.  Presumably, we would be running this makefile on an x86/x86_64 host.  How does find_program knows to find that version here?  It also seem strange to me that we would run the found LLVM_CONFIG_EXE to get its --version info.  What am I not seeing here?

No, it won't work because of what you noticed. But cross building of JSC/WebKit
on Linux wasn't officially supported ever. Now we can build for ARM target 
on an ARM hardware or on an X86(_64) hardware inside a complete ARM rootFS
emulating almost everything (perl, python, ruby, llvm-config, ...).

As I mentioned in the changelog, I added back exactly the r196749 state 
of this file without a bit modification. It worked fine for years for
LLVM-based FTL JIT, I don't plan to refactor it in the near futere.
Comment 12 Csaba Osztrogonác 2016-08-02 05:38:17 PDT
Based on comments https://bugs.webkit.org/show_bug.cgi?id=154323#c20
and https://bugs.webkit.org/show_bug.cgi?id=154323#c26 , could 
you guys r+ this patch to add the ARM traditional disassembler?
Comment 13 Csaba Osztrogonác 2016-08-03 04:01:25 PDT
(In reply to comment #10)

> Another question: why can't ARM traditional use the ARMv7 disassembler
> (which we do have implemented)?  Are the differences that great?

I missed this question, sorry. 

ARMv7Disassembler is to disassemble Thumb2 instructions generated by
ARMv7Assembler. But ARMAssembler (traditional) generates ARM instructions 
(fixed 32-bit sized), not Thumb2 instructions. They are two different
instruction set.
Comment 14 Mark Lam 2016-08-03 09:53:25 PDT
Comment on attachment 284876 [details]
Patch

r=me
Comment 15 WebKit Commit Bot 2016-08-03 10:14:16 PDT
Comment on attachment 284876 [details]
Patch

Clearing flags on attachment: 284876

Committed r204084: <http://trac.webkit.org/changeset/204084>
Comment 16 WebKit Commit Bot 2016-08-03 10:14:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Konstantin Tokarev 2016-12-09 14:33:48 PST
>But cross building of JSC/WebKit on Linux wasn't officially supported ever. Now we can build for ARM target on an ARM hardware or on an X86(_64) hardware inside a complete ARM rootFS emulating almost everything (perl, python, ruby, llvm-config, ...).

FWIW, I'm routinely cross-compile Qt port for MIPS and never thought about going through such insanity as you described :)
Comment 18 Csaba Osztrogonác 2016-12-09 23:54:44 PST
(In reply to comment #17)
> >But cross building of JSC/WebKit on Linux wasn't officially supported ever. Now we can build for ARM target on an ARM hardware or on an X86(_64) hardware inside a complete ARM rootFS emulating almost everything (perl, python, ruby, llvm-config, ...).
> 
> FWIW, I'm routinely cross-compile Qt port for MIPS and never thought about
> going through such insanity as you described :)

I don't understand why do think building in a rootfs (emulating with qemu) is an insanity. I would be very curios how do you would like to run update-webkit<efl|gtk>libs, build-webkit, run-javascriptcore-tests (perl and ruby) in the _same_ X86_64 environment to build for a non X86_64 target. It's not as simple as you need a cross compiler, not at all.
Comment 19 Konstantin Tokarev 2016-12-09 23:59:51 PST
Ah, if you do not run tests on real hardware it makes sense indeed.
Comment 20 Csaba Osztrogonác 2016-12-10 00:04:45 PST
(In reply to comment #19)
> Ah, if you do not run tests on real hardware it makes sense indeed.

We don run tests on real hardware with --remote-config-file option of run-javascriptcore-tests. But it calls ldd to collect the dependecies
of the jsc binary. x86-ldd can't do anything with an ARM binary JSC.

Not to mention how complex or impossible to build 20-30 jhbuild dependencies
without qemu.