Bug 206231

Summary: [ARMv7] Assembler is generating wrong instruction for ldr r2, [r3, #7]
Product: WebKit Reporter: Caio Lima <ticaiolima>
Component: JavaScriptCoreAssignee: Caio Lima <ticaiolima>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, jsc32, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: DoNotImportToRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 203563    
Bug Blocks: 205581, 206602    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Caio Lima
Reported 2020-01-14 07:39:53 PST
...
Attachments
Patch (1.86 KB, patch)
2020-01-22 07:38 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (2.15 KB, patch)
2020-01-22 07:43 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (2.26 KB, patch)
2020-01-22 11:14 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Caio Lima
Comment 1 2020-01-22 07:38:41 PST
Created attachment 388419 [details] Patch
Caio Lima
Comment 2 2020-01-22 07:43:59 PST
Created attachment 388420 [details] Patch
Mark Lam
Comment 3 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.
Caio Lima
Comment 4 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.
Caio Lima
Comment 5 2020-01-22 11:14:35 PST
Created attachment 388444 [details] Patch
WebKit Commit Bot
Comment 6 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>
WebKit Commit Bot
Comment 7 2020-01-22 13:43:05 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 8 2020-01-22 15:58:40 PST
<rdar://problem/58814740>
Note You need to log in before you can comment on or make changes to this bug.