Bug 138671 – Reduce amount of cut-and-paste needed for probe mechanism implementations

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

The present implementation requires that each MacroAssembler implementation provide their own copy of all of the probe implementations even when most of it is identical.  This patch reimplements the common parts into MacroAssemblerProbe.h and have each target specific MacroAssembler implementation inherit from it instead.  The target specific MacroAssembler only needs to implement a few target specific methods that are expected by and documented in MacroAssemblerProbe.h.  MacroAssembler ports that do not care to implement the probe mechanism simply need no inherit from MacroAssemblerProbe.

In this patch, I also fill out some missing xmm registers in the X86_64's FP register list.  Also simplified the X86 and X86_64 ports to use the same port implementation.

This is all done in preparation to add printing functionality in JITted code for debugging.

[Mark Lam]

Created attachment 241444 [details]
the patch.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:131:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:143:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerProbe.h:39:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerProbe.h:42:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:102:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:104:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:110:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:112:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:118:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:120:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:37:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:40:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:45:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:52:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:54:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:58:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:61:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:63:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:37:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:39:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:45:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:47:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:53:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:55:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/ARMAssembler.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 25 in 14 files


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

[Geoffrey Garen]

Comment on attachment 241444 [details]
the patch.

Let's not build up a side definition of the CPU in the probing machinery -- instead, let's use the shared definition provided by the assembler.

(1) The assembler header should define the canonical list of registers in a macro.

(2) The definition of the register namespaces should use the canonical list.

(3) The probe should use the canonical list.

(4) The probe should treat FP registers as 64bit values, as the assembler does. This eliminates some abstractions like GPRegValue and FPRegValue. 

(5) If there's some need for a global definition of an "Assembler" type, that should bear the name "Assembler", rather than being specialized with a probe-related name. However, I'll note that the MacroAssembler class defines its assembler type as a dependent type, so there's probably no need for a global type name.

(6) Let's call CPUInfo CPUState, since that's a little clearer about recording the temporary state of the CPU, and let's put it in its own header, and remove the redeclaration of RegisterID and FPRegisterID.

[Mark Lam]

Created attachment 241531 [details]
alternate implementation based on Geoff's suggestions

[Mark Lam]

Created attachment 241534 [details]
alternate implementation based on Geoff's suggestions + a missing ChangeLog comment.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:843:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:846:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:860:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:873:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:885:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:891:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:40:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:45:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:52:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:54:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:60:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 11 in 11 files


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

[Geoffrey Garen]

Comment on attachment 241534 [details]
alternate implementation based on Geoff's suggestions + a missing ChangeLog comment.

r=me

[Mark Lam]

Thanks for the review.  Landed in r176134: <http://trac.webkit.org/r176134>.