Bug 206231 - [ARMv7] Assembler is generating wrong instruction for ldr r2, [r3, #7]
Summary: [ARMv7] Assembler is generating wrong instruction for ldr r2, [r3, #7]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords: DoNotImportToRadar
Depends on: 203563
Blocks: 205581 206602
  Show dependency tree / graph
 
Reported: 2020-01-14 07:39 PST by Caio Lima
Modified: 2020-01-22 16:00 PST (History)
9 users (show)

See Also:

Attachments
Patch (1.86 KB, patch)
2020-01-22 07:38 PST, Caio Lima 		no flags Details | Formatted Diff | Diff
Patch (2.15 KB, patch)
2020-01-22 07:43 PST, Caio Lima 		no flags Details | Formatted Diff | Diff
Patch (2.26 KB, patch)
2020-01-22 11:14 PST, Caio Lima 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) 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 Caio Lima 2020-01-14 07:39:53 PST 
...
Comment 1 Caio Lima 2020-01-22 07:38:41 PST 
Created attachment 388419 [details]
Patch
Comment 2 Caio Lima 2020-01-22 07:43:59 PST 
Created attachment 388420 [details]
Patch
Comment 3 Mark Lam 2020-01-22 08:56:50 PST 
Comment on attachment 388420 [details]
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_manual.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.
Comment 4 Caio Lima 2020-01-22 11:09:19 PST 
Comment on attachment 388420 [details]
Patch

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

Thank you very much for the review.

>> Source/JavaScriptCore/assembler/ARMv7Assembler.h:1034
>> +            // (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_manual.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.

Yes, I meant the section you pointed out LDR (immediate, Thumb). Since I'm using a 2018 version of the doc, I updated the comment with a link to it and kept the section A8.8.63.
Comment 5 Caio Lima 2020-01-22 11:14:35 PST 
Created attachment 388444 [details]
Patch
Comment 6 WebKit Commit Bot 2020-01-22 13:43:03 PST 
Comment on attachment 388444 [details]
Patch

Clearing flags on attachment: 388444

Committed r254943: <https://trac.webkit.org/changeset/254943>
Comment 7 WebKit Commit Bot 2020-01-22 13:43:05 PST 
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2020-01-22 15:58:40 PST 
<rdar://problem/58814740>