I've reworked the patch following comments from Wilco [1] (sorry could not find it in my MUA for some reason).
[1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00317.html
== Context ==
When saving registers, function thumb1_expand_prologue () aims at minimizing the number of push instructions. One of the optimization it does is to push LR alongside high register(s) (after having moved them to low register(s)) when there
is no low register to save. The way this is implemented is to add LR to the pushable_regs mask if it is live just before pushing the registers in that mask. The mask of live pushable registers which is used to detect whether LR needs to
be saved is then clear to ensure LR is only saved once.
== Problem ==
However beyond deciding what register to push pushable_regs is used to track what pushable register can be used to move a high register before being pushed, hence the name. That mask is cleared when all high registers have been assigned a
low register but the clearing assumes the high registers were assigned to the registers with the biggest number in that mask. This is not the case because LR is not considered when looking for a register in that mask. Furthermore, LR
might have been saved in the TARGET_BACKTRACE path above yet the mask of live pushable registers is not cleared in that case.
== Solution ==
This patch changes the loop to iterate over register LR to r0 so as to both fix the stack corruption reported in PR77933 and reuse lr to push some high register when possible. This patch also introduce a new variable lr_needs_saving to
record whether LR (still) needs to be saved at a given point in code and sets the variable accordingly throughout the code, thus fixing the second issue. Finally, this patch create a new push_mask variable to distinguish between the mask
of registers to push and the mask of live pushable registers.
== Note ==
Other bits could have been improved but have been left out to allow the patch to be backported to stable branch:
(1) using argument registers that are not holding an argument
(2) using push_mask consistently instead of l_mask (in TARGET_BACKTRACE), mask (low register push) and push_mask
(3) the !l_mask case improved in TARGET_BACKTRACE since offset == 0
(4) rename l_mask to a more appropriate name (live_pushable_regs_mask?)
ChangeLog entry are as follow:
*** gcc/ChangeLog ***
2016-11-08 Thomas Preud'homme <thomas.preudhomme@arm.com>
PR target/77933
* config/arm/arm.c (thumb1_expand_prologue): Distinguish between lr
being live in the function and lr needing to be saved. Distinguish
between already saved pushable registers and registers to push.
Check for LR being an available pushable register.
*** gcc/testsuite/ChangeLog ***
2016-11-08 Thomas Preud'homme <thomas.preudhomme@arm.com>
PR target/77933
* gcc.target/arm/pr77933-1.c: New test.
* gcc.target/arm/pr77933-2.c: Likewise.
Testing: no regression on arm-none-eabi GCC cross-compiler targeting Cortex-M0
Is this ok for trunk?
Best regards,
Thomas
On 02/11/16 17:08, Thomas Preudhomme wrote:
Hi,
When saving registers, function thumb1_expand_prologue () aims at minimizing the
number of push instructions. One of the optimization it does is to push lr
alongside high register(s) (after having moved them to low register(s)) when
there is no low register to save. The way this is implemented is to add lr to
the list of registers that can be pushed just before the push happens. This
would then push lr and allows it to be used for further push if there was not
enough registers to push all high registers to be pushed.
However, the logic that decides what register to move high registers to before
being pushed only looks at low registers (see for loop initialization). This
means not only that lr is not used for pushing high registers but also that lr
is not removed from the list of registers to be pushed when it's not used. This
extra lr push is not poped in epilogue leading in stack corruption.
This patch changes the loop to iterate over register r0 to lr so as to both fix
the stack corruption and reuse lr to push some high register when possible.
ChangeLog entry are as follow:
*** gcc/ChangeLog ***
2016-11-01 Thomas Preud'homme <thomas.preudhomme@arm.com>
PR target/77933
* config/arm/arm.c (thumb1_expand_prologue): Also check for lr being a
pushable register.
*** gcc/testsuite/ChangeLog ***
2016-11-01 Thomas Preud'homme <thomas.preudhomme@arm.com>
PR target/77933
* gcc.target/arm/pr77933.c: New test.
Testing: no regression on arm-none-eabi GCC cross-compiler targeting Cortex-M0
Is this ok for trunk?
Best regards,
Thomas