| Summary: | JIT probe for ARMv7 and ARM + bug fixes | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | fpizlo, ggaren, mhahnenberg, msaboff, oliver | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 202487 [details]
the patch
Created attachment 203087 [details]
updated patch: fixed some typos, and changed some "copy-and-paste"d comments to just refer to the X86_64 version.
Comment on attachment 203087 [details] updated patch: fixed some typos, and changed some "copy-and-paste"d comments to just refer to the X86_64 version. View in context: https://bugs.webkit.org/attachment.cgi?id=203087&action=review > Source/JavaScriptCore/config.h:82 > +#define WTF_USE_MASM_PROBE 1 should not have enabled by default. Forgot to disable this after testing a build. Will fix and upload another patch. > Source/WebKit2/Shared/mac/ChildProcessMac.mm:54 > +// mlam extern "C" CGError CGSShutdownServerConnections(); These should not have been included in the patch. Created attachment 203089 [details]
corrected patch.
Comment on attachment 203089 [details] corrected patch. View in context: https://bugs.webkit.org/attachment.cgi?id=203089&action=review r=me, with comments. > Source/JavaScriptCore/jit/JITStubsARM.h:290 > + // 2. Issue 1 means we will need to write to the stack location at > + // ProbeContext.cpu.sp - 4. But if the user probe function had modified > + // the value of ProbeContext.cpu.sp to point in the range between > + // &ProbeContext.cpu.ip thru &ProbeContext.cpu.aspr, then the action for > + // Issue 1 may trash the values to be restored before we can restore > + // them. Do we really care if the user modified sp? Seems like something we shouldn't have to worry about. > Source/JavaScriptCore/jit/JITStubsARMv7.h:393 > + // 2. Issue 1 means we will need to write to the stack location at > + // ProbeContext.cpu.sp - 4. But if the user probe function had modified > + // the value of ProbeContext.cpu.sp to point in the range between > + // &ProbeContext.cpu.ip thru &ProbeContext.cpu.aspr, then the action for > + // Issue 1 may trash the values to be restored before we can restore > + // them. Same comment as above. > Source/JavaScriptCore/jit/JITStubsX86.h:196 > + // 2. The user probe function may have altered the restore value of esp to > + // point to the vicinity of one of the restore values for the remaining > + // registers left to be restored. > + // That means, for requirement 1, we may end up writing over some of the > + // restore values. We can check for this, and first copy the restore > + // values to a "safe area" on the stack before commencing with the action > + // for requirement 1. Ditto. > Source/JavaScriptCore/jit/JITStubsX86_64.h:225 > + // 2. The user probe function may have altered the restore value of esp to > + // point to the vicinity of one of the restore values for the remaining > + // registers left to be restored. > + // That means, for requirement 1, we may end up writing over some of the > + // restore values. We can check for this, and first copy the restore > + // values to a "safe area" on the stack before commencing with the action > + // for requirement 1. Ditto (In reply to comment #5) > (From update of attachment 203089 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203089&action=review > > r=me, with comments. > > > Source/JavaScriptCore/jit/JITStubsARM.h:290 > > + // 2. Issue 1 means we will need to write to the stack location at > > + // ProbeContext.cpu.sp - 4. But if the user probe function had modified > > + // the value of ProbeContext.cpu.sp to point in the range between > > + // &ProbeContext.cpu.ip thru &ProbeContext.cpu.aspr, then the action for > > + // Issue 1 may trash the values to be restored before we can restore > > + // them. > > Do we really care if the user modified sp? Seems like something we shouldn't have to worry about. The promise of the probe is that if the probe functions modify the registers values in the ProbeContext, we'll set the machine registers to those values upon returning. The code as is honors that promise without exception. The only case where we'll want that feature is if we're debugging JIT generated code with the probe mechanism, and encounter a case where we see the stack at a wrong place (e.g. some new JIT code we wrote forgot to pop 2 values), and we want to adjust it from our debugging session in gdb to see if that resolves the failure we're debugging. It's rare that we'll encounter such situations, but it can happen. I'm going to commit it as is. Thanks. Landed in r150844: <http://trac.webkit.org/changeset/150844>. |
1. ARMv7 + traditional ARM support 2. Fix bugs: a. Don't assume the sp is already aligned when we setup our probe call. Ensure alignment ourselves. b. Ensure that we can handle the case where the user modifies the sp to point into the region where the ProbeContext is stored on the stack. 3. Make X86_64 dumps look nicer (sort registers to match gdb's order). 4. Save condition code registers. Patch coming.