[webkit-reviews] review granted: [Bug 206231] [ARMv7] Assembler is generating wrong instruction for ldr r2, [r3, #7] : [Attachment 388420] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 206231: [ARMv7] Assembler is generating wrong instruction for ldr r2, [r3,
#7]
https://bugs.webkit.org/show_bug.cgi?id=206231

Attachment 388420: Patch

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




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

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

r=me

> Source/JavaScriptCore/assembler/ARMv7Assembler.h:1034
> +	       // We can use Encoding T1 when imm is a multiple of 4.
> +	       // (see A8.8.63 on ARM Architecture Reference Manual ARMv7-A and
ARMv7-R edition)

I think you mean A8.8.62 LDR (immediate, Thumb) on page A8-406.  See
https://static.docs.arm.com/ddi0406/cc/DDI0406C_C_arm_architecture_reference_ma
nual.pdf.

Please update the comment to say "We can only use Encoding T1 when imm is a
multiple of 4."
Please also update comment with the reference to the section of the spec. 
Alternatively, if you have a reference to a newer version of the spec where the
relevant details are in section A8.8.63, please provide a url to that spec so
that the reader / reviewer can confirm the correctness of this implementation. 
Thanks.