| Summary: | [ARMv7] Assembler is generating wrong instruction for ldr r2, [r3, #7] | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Caio Lima <ticaiolima> | ||||||||
| Component: | JavaScriptCore | Assignee: | 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
Caio Lima
2020-01-14 07:39:53 PST
Created attachment 388419 [details]
Patch
Created attachment 388420 [details]
Patch
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 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. Created attachment 388444 [details]
Patch
Comment on attachment 388444 [details] Patch Clearing flags on attachment: 388444 Committed r254943: <https://trac.webkit.org/changeset/254943> All reviewed patches have been landed. Closing bug. |