[webkit-reviews] review granted: [Bug 175688] Enhance MacroAssembler::probe() to allow the probe function to resize the stack frame and alter stack data in one pass. : [Attachment 318593] proposed patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Aug 19 22:55:24 PDT 2017


JF Bastien <jfbastien at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 175688: Enhance MacroAssembler::probe() to allow the probe function to
resize the stack frame and alter stack data in one pass.
https://bugs.webkit.org/show_bug.cgi?id=175688

Attachment 318593: proposed patch.

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




--- Comment #8 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 318593
  --> https://bugs.webkit.org/attachment.cgi?id=318593
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?


More information about the webkit-reviews mailing list