JSC: There should be a disassembler for ARM Thumb 2

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

To simplify debugging and profiling of JSC JIT'ed code on ARM devices there should be a disassembler for Thumb2.

[Michael Saboff]

Created attachment 201126 [details]
Patch

[WebKit Commit Bot]

Attachment 201126 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp', u'Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h', u'Source/JavaScriptCore/disassembler/ARMv7Disassembler.cpp', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Platform.h']" exit_code: 1
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:30:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:41:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:42:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:160:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:762:  Missing space before ( in switch(  [whitespace/parens] [5]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:1349:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h:46:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h:155:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h:658:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h:815:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h:825:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h:829:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h:849:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.h:866:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/JavaScriptCore/disassembler/ARMv7Disassembler.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 16 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 201126 [details]
Patch

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

r=me, but do the style fixes and such.

>> Source/JavaScriptCore/disassembler/ARMv7Disassembler.cpp:31
>> +#include "MacroAssemblerCodeRef.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Yeah, you should fix this.

>> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:30
>> +#include "ARMv7DOpcode.h"
> 
> You should add a blank line after implementation file's own header.  [build/include_order] [4]

Actually, this should be included about the "#if USE(..." and immediately after "#include "config.h"".

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:46
> +const char* const ARMv7DOpcode::s_optionName[8] =
> +{ "uxtb", "uxth", "uxtw", "uxtx", "sxtb", "sxth", "sxtw", "sxtx" };

This is weird, I would put the '{' at end of the previous line.

>> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:160
>> +    ASSERT (blocksize > 0 && blocksize <= MaxITBlockSize);
> 
> Extra space before ( in function call  [whitespace/parens] [4]

Word.

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:221
> +void ARMv7DOpcode::bufferPrintf(const char* format, ...)
> +{
> +    if (m_bufferOffset >= bufferSize)
> +        return;
> +
> +    va_list argList;
> +    va_start(argList, format);
> +
> +    m_bufferOffset += vsnprintf(m_formatBuffer + m_bufferOffset, bufferSize - m_bufferOffset, format, argList);
> +
> +    va_end(argList);
> +}

You could use the StringPrintStream.  But this is fine.

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:227
> +

Weird whitespace.

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:246
> +    if (length >= 7)
> +        length = 6;

std::min() ?

> Source/JavaScriptCore/disassembler/ARMv7/ARMv7DOpcode.cpp:1361
> +            return m_formatBuffer;
> +        } else
> +            appendRegisterName(rn());

I think that stylebot doesn't like you for this else statement.  I agree, it's weird.

[Michael Saboff]

Committed r149821: <http://trac.webkit.org/changeset/149821>