[webkit-reviews] review granted: [Bug 173677] Switch VMTraps to use halt instructions rather than breakpoint instructions : [Attachment 313658] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 173677: Switch VMTraps to use halt instructions rather than breakpoint
instructions
https://bugs.webkit.org/show_bug.cgi?id=173677

Attachment 313658: Patch

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




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 313658
  --> https://bugs.webkit.org/attachment.cgi?id=313658
Patch

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

r=me with some a suggestion and typo fixes.

>> Source/JavaScriptCore/ChangeLog:11
>> +	    any exceptioning instruction we want. I went weth the halt
instructions
> 
> "weth"

typo: /weth/with/

> Source/JavaScriptCore/ChangeLog:12
> +	   since the naming was consistant and partly because I couldn't find

typo: /consistant/consistent/

> Source/JavaScriptCore/runtime/VMTraps.cpp:123
> +    // ARM does not have any instruction guarenteed to segmentation fault.

typo: /guarenteed/guaranteed/.

Since this code needs to match what is being done at in other code not
obviously seen locally in this function, I think it's worth adding a comment
here saying:
tryInstallTrapBreakpoints() will install "Halt" instructions (via
MacroAssembler::replaceWithHalt()) that will trigger SIGILL on ARM64. On x86
and x86_64, the "Halt" instruction will trigger SIGSEGV.