Bug 175688 – Enhance MacroAssembler::probe() to allow the probe function to resize the stack frame and alter stack data in one pass.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

<rdar://problem/33436870>

[Mark Lam]

Created attachment 318548 [details]
work in progress.

[Build Bot]

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.

[Mark Lam]

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.

[Mark Lam]

(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.

[Mark Lam]

Created attachment 318593 [details]
proposed patch.

One more test to run before I set the review request bit.

[Build Bot]

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.

[Mark Lam]

Comment on attachment 318593 [details]
proposed patch.

All my tests are green.  Time to get a review.

[JF Bastien]

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?

[Mark Lam]

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.

[JF Bastien]

(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.

[Mark Lam]

Created attachment 318608 [details]
patch for landing.

[Build Bot]

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.

[Mark Lam]

Created attachment 318610 [details]
patch for landing w/ Windows fix.

[Build Bot]

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.

[WebKit Commit Bot]

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

[Mark Lam]

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>.

[Csaba Osztrogonác]

It broke the cloop build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/4544

[Mark Lam]

Landed fix for CLoop build in r220960: <http://trac.webkit.org/r220960>.

[Carlos Alberto Lopez Perez]

(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)

?

[Mark Lam]

(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.