Bug 175688 - Enhance MacroAssembler::probe() to allow the probe function to resize the stack frame and alter stack data in one pass.
Summary: Enhance MacroAssembler::probe() to allow the probe function to resize the sta...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 175725
Blocks: 174645
  Show dependency tree / graph
 
Reported: 2017-08-17 14:50 PDT by Mark Lam
Modified: 2017-08-21 21:59 PDT (History)
11 users (show)

See Also:

Attachments
work in progress. (96.18 KB, patch)
2017-08-18 15:04 PDT, Mark Lam 		no flags Details | Formatted Diff | Diff
proposed patch. (123.35 KB, patch)
2017-08-19 16:32 PDT, Mark Lam 		jfbastien: review+
Details | Formatted Diff | Diff
patch for landing. (123.32 KB, patch)
2017-08-20 17:13 PDT, Mark Lam 		no flags Details | Formatted Diff | Diff
patch for landing w/ Windows fix. (123.31 KB, patch)
2017-08-20 19:52 PDT, Mark Lam 		commit-queue: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-08-17 14:50:52 PDT 
<rdar://problem/33436870>
Comment 1 Mark Lam 2017-08-18 15:04:38 PDT 
Created attachment 318548 [details]
work in progress.
Comment 2 Build Bot 2017-08-18 15:06:09 PDT 
Attachment 318548 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:370:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:371:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:372:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:381:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:383:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:385:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:387:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:407:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerPrinter.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 9 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2017-08-18 17:54:58 PDT 
I've the patch working for x86, x86_64, and arm64 now.  There's some mysterious bug in the ARMv7 version.  So, I need to do a bit more debugging.
Comment 4 Mark Lam 2017-08-19 13:04:03 PDT 
(In reply to Mark Lam from comment #3)
> I've the patch working for x86, x86_64, and arm64 now.  There's some
> mysterious bug in the ARMv7 version.  So, I need to do a bit more debugging.

I found my issue: on ARMv7, the T1 encoding of the MOV immediate always sets the flags i.e. it's a MOVS.  This is what's messing up my flags.  The issue is technically not related to my patch, but I'll look into a fix since the probe is probably the only place where this issue matters.
Comment 5 Mark Lam 2017-08-19 16:32:15 PDT 
Created attachment 318593 [details]
proposed patch.

One more test to run before I set the review request bit.
Comment 6 Build Bot 2017-08-19 16:33:45 PDT 
Attachment 318593 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:370:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:371:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:372:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:381:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:383:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:385:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:387:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:407:  Extra space before [.  [whitespace/brackets] [5]
Total errors found: 8 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Mark Lam 2017-08-19 16:38:13 PDT 
Comment on attachment 318593 [details]
proposed patch.

All my tests are green.  Time to get a review.
Comment 8 JF Bastien 2017-08-19 22:55:24 PDT 
Comment on attachment 318593 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:-130
> -#define SAVED_PROBE_ERROR_FUNCTION_OFFSET   (PROBE_SIZE + (2 * PTR_SIZE))

I'm not sure I understand this change.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:288
> +static_assert(!(sizeof(LRRestorationRecord) & 0xf), "LRRestorationRecord must be 16-byte aligned");

Size won't guarantee alignment.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:165
> +COMPILE_ASSERT((PROBE_EXECUTOR_OFFSET + PTR_SIZE) <= (PROBE_SIZE + OUT_SIZE), Must_have_room_after_ProbeContext_to_stash_the_probe_handler);

Can you use staic_assert instead?

> Source/JavaScriptCore/assembler/ProbeStack.cpp:82
> +    return false;

return std::any_of(m_pages.begin(), m_pages.end(), [] (auto it) { return it->value=>hasWritesToFlush(); });

> Source/JavaScriptCore/assembler/ProbeStack.h:148
> +        return bitwise_cast<double>(page->get<uint64_t>(address));

Can you just bitwise_cast to T and remove the enable_if?
Comment 9 Mark Lam 2017-08-20 00:16:35 PDT 
Comment on attachment 318593 [details]
proposed patch.

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

Thanks for the review.  Looks like I have a Windows build issue.  I'll land after I resolve that.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:-130
>> -#define SAVED_PROBE_ERROR_FUNCTION_OFFSET   (PROBE_SIZE + (2 * PTR_SIZE))
> 
> I'm not sure I understand this change.

I used to stash away SAVED_PROBE_LR_OFFSET so that I can determine if the client change lr or not.  However, we really don't need to do that.  If the client did not change pc, then we're turning to the generated JIT probe return site where we'll restore lr whether it changed or not.  Hence, SAVED_PROBE_LR_OFFSET is not needed anymore.

I used to stash SAVED_PROBE_ERROR_FUNCTION_OFFSET because the trampoline needed to error out by calling this error function if the client changed both pc and lr (which is not supported).  We can now do that checking in C++ code in Probe::executeProbe().  Hence, there's no more need for SAVED_PROBE_ERROR_FUNCTION_OFFSET in the trampoline.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:288
>> +static_assert(!(sizeof(LRRestorationRecord) & 0xf), "LRRestorationRecord must be 16-byte aligned");
> 
> Size won't guarantee alignment.

Yes, but the LRRestorationRecord is pushed on top of the stack pointer, which is supposed to be aligned.  So, together, we get alignment.  Plus WebKit code uses the term "aligned" to mean "packed" as well in lots of places (since the math for alignment is the same as the math for packing).  I used to say packed a long time ago, but just conform to using the term aligned these days.

>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:165
>> +COMPILE_ASSERT((PROBE_EXECUTOR_OFFSET + PTR_SIZE) <= (PROBE_SIZE + OUT_SIZE), Must_have_room_after_ProbeContext_to_stash_the_probe_handler);
> 
> Can you use staic_assert instead?

COMPILE_ASSERT is static_assert (introduced back in the days before static_assert was available for use).  I had considered using straight static_assert, but was a bit wishy washy and settled on staying with COMPILE_ASSERT to be consistent with existing code.  Maybe I can do a pass later to change all the COMPILE_ASSERTs in probe code to static_assert.  I'd like to leave this as is for now.

>> Source/JavaScriptCore/assembler/ProbeStack.cpp:82
>> +    return false;
> 
> return std::any_of(m_pages.begin(), m_pages.end(), [] (auto it) { return it->value=>hasWritesToFlush(); });

Ooo ... nice.  I will land with that if I don't run into complications with it.

>> Source/JavaScriptCore/assembler/ProbeStack.h:148
>> +        return bitwise_cast<double>(page->get<uint64_t>(address));
> 
> Can you just bitwise_cast to T and remove the enable_if?

You might be right.  Strict aliasing issues always make my head hurt.  I think the existing code is safe.  I would like to land this as is, and give it some more thought later.  If there are no issues on further consideration, I can simplify this as you suggested in a follow up patch.
Comment 10 JF Bastien 2017-08-20 07:06:08 PDT 
(In reply to Mark Lam from comment #4)
> (In reply to Mark Lam from comment #3)
> > I've the patch working for x86, x86_64, and arm64 now.  There's some
> > mysterious bug in the ARMv7 version.  So, I need to do a bit more debugging.
> 
> I found my issue: on ARMv7, the T1 encoding of the MOV immediate always sets
> the flags i.e. it's a MOVS.  This is what's messing up my flags.  The issue
> is technically not related to my patch, but I'll look into a fix since the
> probe is probably the only place where this issue matters.

Forgot to reply to this: IIRC on Thumb you get a MOV without S by putting it in an IT block with condition AL.
Comment 11 Mark Lam 2017-08-20 17:13:53 PDT 
Created attachment 318608 [details]
patch for landing.
Comment 12 Build Bot 2017-08-20 17:16:11 PDT 
Attachment 318608 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/ProbeStack.cpp:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:370:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:371:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:372:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:381:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:383:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:385:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:387:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:407:  Extra space before [.  [whitespace/brackets] [5]
Total errors found: 9 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Mark Lam 2017-08-20 19:52:25 PDT 
Created attachment 318610 [details]
patch for landing w/ Windows fix.
Comment 14 Build Bot 2017-08-20 19:53:27 PDT 
Attachment 318610 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/ProbeStack.cpp:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:370:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:371:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:372:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:381:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:383:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:385:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:387:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:407:  Extra space before [.  [whitespace/brackets] [5]
Total errors found: 9 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 WebKit Commit Bot 2017-08-20 21:11:24 PDT 
Comment on attachment 318610 [details]
patch for landing w/ Windows fix.

Rejecting attachment 318610 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 318610, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ing rebase:
:040000 040000 233d6811df1f147ca443b4b394705bbb2f93cb09 db0b1ac60dc9ee2de6539d93219715a97d139bf9 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/4351557
Comment 16 Mark Lam 2017-08-20 21:29:23 PDT 
There were some tabs in MacroAssemblerARM.cpp and MacroAssemblerARMv7.cpp that were not detected by the style checker but was caught by the pre-commit hooks.  I've fixed them.

The patch is landed in r220958: <http://trac.webkit.org/r220958>.
Comment 18 Mark Lam 2017-08-20 23:57:43 PDT 
Landed fix for CLoop build in r220960: <http://trac.webkit.org/r220960>.
Comment 19 Carlos Alberto Lopez Perez 2017-08-21 18:35:56 PDT 
(In reply to Mark Lam from comment #18)
> Landed fix for CLoop build in r220960: <http://trac.webkit.org/r220960>.

I think there is a typo in Source/WTF/wtf/Platform.h:

-#if !ENABLE(JIT) || OS(WINDOW)
+#if !ENABLE(JIT) || OS(WINDOWS)

?
Comment 20 Mark Lam 2017-08-21 21:59:27 PDT 
(In reply to Carlos Alberto Lopez Perez from comment #19)
> (In reply to Mark Lam from comment #18)
> > Landed fix for CLoop build in r220960: <http://trac.webkit.org/r220960>.
> 
> I think there is a typo in Source/WTF/wtf/Platform.h:
> 
> -#if !ENABLE(JIT) || OS(WINDOW)
> +#if !ENABLE(JIT) || OS(WINDOWS)

Thanks.  This is fix in a follow up fix to bug 175656: see https://bugs.webkit.org/show_bug.cgi?id=175656#c6.