Bug 59896 - Remove JmpSrc/JmpDst types.
Summary: Remove JmpSrc/JmpDst types.
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2011-05-01 14:44 PDT by Gavin Barraclough
Modified: 2013-04-30 05:02 PDT (History)
3 users (show)

See Also:

The patch (60.29 KB, patch)
2011-05-01 14:46 PDT, Gavin Barraclough
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2011-05-01 14:44:03 PDT
The JmpSrc/JmpDst classes predate the MacroAssembler interface.  Having these object be per-assembler in unhelpful, causes unnecessary code duplication, and prevents the AssemblerBuffer from providing a richer type for labels.  The limited semantic meaning that they did convey is undermined by the manner in which their meanings have been overloaded (use of JmpSrc for Call, JmpDst for data labels).

Jumps on ARMv7 have had additional information added to the object via the ARMv7 JmpSrc.  This data should probably be in the instruction stream.  This patch does not fix the problem, and moves the data (ifdefed) to AbstractMacroAssembler::Jump (which is effectively where it was before!).  This at least closes the hole such that no further data may be added to JmpSrc, but this is unfortunate, and should be cleaned up.
Comment 1 Gavin Barraclough 2011-05-01 14:46:19 PDT
Created attachment 91847 [details]
The patch
Comment 2 WebKit Review Bot 2011-05-01 14:48:00 PDT
Attachment 91847 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:1197:  jmp_r is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/AssemblerBuffer.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 14 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Sam Weinig 2011-05-01 14:57:57 PDT
Comment on attachment 91847 [details]
The patch

Please add a comment about why 2^32-1 is enough.
Comment 4 Gavin Barraclough 2011-05-01 17:48:11 PDT
Fixed in r85432.
Comment 5 Chao-ying Fu 2011-05-02 16:25:35 PDT
Hi Gavin,

  For MIPS build, we need to switch the order of header files to get it compiled.  Thanks!


fu@debian:/disk/fu/dev/WebKit/Source/JavaScriptCore/assembler$ svn diff
Index: MacroAssemblerMIPS.h
--- MacroAssemblerMIPS.h        (revision 85498)
+++ MacroAssemblerMIPS.h        (working copy)
@@ -29,8 +29,8 @@


+#include "MIPSAssembler.h"
 #include "AbstractMacroAssembler.h"
-#include "MIPSAssembler.h"

 namespace JSC {
Comment 6 石梦军 2013-04-30 05:02:22 PDT
is exist some reason for using char* to replace void* in the getRelocatedAddress function of the sh4assembler.h file?