Fix two incorrect conditions in the ARM packed exception handling#3541
Fix two incorrect conditions in the ARM packed exception handling#3541colin-home merged 1 commit intoMicrosoftDocs:mainfrom
Conversation
|
@mstorsjo : Thanks for your contribution! The author(s) have been notified to review your proposed change. |
|
#label:"aq-pr-triaged" |
|
@mstorsjo |
|
Thanks, Martin. I defer this to @pmsjt. |
|
Thanks for contributing. I'll look at this as soon as I can. It might take a couple of weeks tho. I appreciate the patience. |
Thanks! I noticed another thing that needs clarifying too (I'll amend the patch in a moment): For prologue instruction 3a, |
mstorsjo
force-pushed
the
arm-exception-packed
branch
from
c0a6ba1 to
7fab074
Compare
|
@mstorsjo : Thanks for your contribution! The author(s) have been notified to review your proposed change. |
|
Ping @pmsjt for a review before we all go on vacation. :) |
| |9b|*`H`*==1 and *`L`*==1|32|`ldr pc,[sp],#0x14`| | ||
| |8|*`C`*==1 or (*`L`*==1 and (*`H`*==0 or *`Ret`* !=0)) or *`R`*==0 or *`EF`*==1|16/32|`pop {registers}`| | ||
| |9a|*`H`*==1 and (*`L`*==0 or *`Ret`*!=0)|16|`add sp,sp,#0x10`| | ||
| |9b|*`H`*==1 and *`L`*==1 and *`Ret`*==0|32|`ldr pc,[sp],#0x14`| |
There was a problem hiding this comment.
Should we also add an explanation to the Ret==0 case saying that it could be pop {pc} OR it could also be ldr pc,[sp],#0x14?
There was a problem hiding this comment.
On instruction 8? Maybe, but I'm not sure how to include that information without blowing up the table too much. Because if R==0 then you'd still have instruction 8 popping a number of registers, but not including pc. The logical condition, while complex, should be accurate for when this instruction is present though.
docs/build/arm-exception-handling.md
Outdated
| Instructions 7 and 8 use the same logic as the prologue to determine which registers are restored from the stack, but with these three changes: first, *`EF`* is used in place of *`PF`*; second, if *`Ret`* = 0 and *`H`* = 0, then LR is replaced with PC in the register list and the epilogue ends immediately; third, if *`Ret`* = 0 and *`H`* = 1, then LR is omitted from the register list and popped by instruction 9b. | ||
|
|
||
| If *`H`* is set, then either instruction 9a or 9b is present. Instruction 9a is used when *`L`* is 0, to indicate that the LR is not on the stack. In this case, the stack is manually adjusted and *`Ret`* must be 1 or 2 to specify an explicit return. Instruction 9b is used when *`L`* is 1, to indicate an early end to the epilogue, and to return and adjust the stack at the same time. | ||
| If *`H`* is set, then either instruction 9a or 9b is present. Instruction 9a is used when *`Ret`* is nonzero, when instruction 10a or 10b is present (if *`L`* is 1, it is popped as part of instruction 8). In this case, the stack is manually adjusted. Instruction 9b is used when *`L`* is 1 and *`Ret`* is zero, to indicate an early end to the epilogue, and to return and adjust the stack at the same time. |
There was a problem hiding this comment.
This is correct but became too long of a sentence IMO. The two "when" make it hard to decode what is causal here. It is also not clear that the parenthesis part applies to the 10a vs 10b choice, not the 9a vs 9b.
How about something like:
"[...] 9a is used when Ret != 0, which also implies the presence of either 10a or 10b. If L=1, then LR was popped as part of instruction 8.
There was a problem hiding this comment.
That sounds good, changing it that way.
|
The truth table for the registers saved is also not great. Lines [127, 130] refer to C=1, L=0 which is an invalid configuration. I wonder if we should just replace the cells on those 4 lines with "Invalid Encoding" or something like that. |
|
Since you are already making changes here, I wonder if you would also mind fixing line 84: "required" -> "requires". |
That would be good for consistency, yes, otherwise those lines can be misleading.
Sure. |
For the prologue, the distinction between `mov r11, sp` and `add r11, sp, #xx` lies only in whether other registers are pushed before R11 and LR in prologue instruction 2. (The condition `R`==1 and `PF`==0 corresponds to no other registers pushed before `r11`.) The `L` flag doesn't affect it (and the documentation explicit says that when `C` is set, then `L` also must be set, thus `C`==1 and `L`==0 is tautological too). This was correctly described in the textual form, but the conditions in the table included one erronous case. For the epilogue, when `Ret`!=0, the function doesn't return by popping LR directly into PC, but with an explicit branch instruction (10a or 10b). In this case, if `L`==1, the LR register is included in the list of registers popped in instruction 8. If register homing is used (`H`==1), then instruction 9a is used instead of 9b. This case seemed to have been overlooked both in the textual description and in the table. Also, when prologue instruction form 3a is used, clarify that it corresponds to the unwind code FB (narrow nop) only, that unwinding such a prologue doesn't restore `sp` from `r11`. Also fix a typo and clarify that encodings with C=1, L=0 are invalid.
mstorsjo
force-pushed
the
arm-exception-packed
branch
from
7fab074 to
97c4dc2
Compare
|
@mstorsjo : Thanks for your contribution! The author(s) have been notified to review your proposed change. |
|
I applied most of the suggested changes, except for the extra explanation about there either being Thanks for the review! |
pmsjt
left a comment
pmsjt
left a comment
There was a problem hiding this comment.
Thank you so much for your contribution. Can't believe how badly broken this was, with contradictions no more than a paragraph apart.
9 participants
For the prologue, the distinction between
mov r11, spandadd r11, sp, #xxlies only in whether other registers are pushedbefore R11 and LR in prologue instruction 2. (The condition
R==1 andPF==0 corresponds to no other registers pushed beforer11.) TheLflag doesn't affect it (and the documentation explicitsays that when
Cis set, thenLalso must be set, thusC==1and
L==0 is tautological too).This was correctly described in the textual form, but the conditions
in the table included one erronous case.
For the epilogue, when
Ret!=0, the function doesn't return bypopping LR directly into PC, but with an explicit branch instruction
(10a or 10b). In this case, if
L==1, the LR register is includedin the list of registers popped in instruction 8. If register
homing is used (
H==1), then instruction 9a is used instead of 9b.This case seemed to have been overlooked both in the textual
description and in the table.
CC @ThomsonTan who might be familiar with this area.