On 25/06/2019 17:51, Sylvia Taylor wrote:
> Greetings,
>
> This patch fixes a bug with TLS in which the frame pointer is not
> established until after the tlsdesc call, thus not conforming to
> the aarch64 procedure call standard.
>
> Changed the tlsdesc instruction patterns to set a dependency on the
> x29 frame pointer. This helps the instruction scheduler to arrange
> the tlsdesc call after the frame pointer is set.
>
> Example of frame pointer (x29) set incorrectly after tlsdesc call:
>
> stp x29, x30, [sp, -16]!
> adrp x0, :tlsdesc:.LANCHOR0
> ldr x2, [x0, #:tlsdesc_lo12:.LANCHOR0]
> add x0, x0, :tlsdesc_lo12:.LANCHOR0
> .tlsdesccall .LANCHOR0
> blr x2
> ...
> mov x29, sp
> ...
>
> After introducing dependency on x29, the scheduler does the frame
> pointer setup before tlsdesc:
>
> stp x29, x30, [sp, -16]!
> mov x29, sp
> adrp x0, :tlsdesc:.LANCHOR0
> ldr x2, [x0, #:tlsdesc_lo12:.LANCHOR0]
> add x0, x0, :tlsdesc_lo12:.LANCHOR0
> .tlsdesccall .LANCHOR0
> blr x2
> ...
>
> Testcase used with -O2 -fpic:
>
> void foo()
> {
> static __thread int x = 0;
> bar (&x);
> }
>
> I am not sure what would classify as an effective check for this
> testcase. The only idea I received so far would be to write a regexp
> inside a scan-assembler-not that would potentially look for this pattern:
>
> <optional instructions>
> .tlsdesccall <label>
> blr <reg>
> <optional instructions>
> [mov x29, sp] OR [add x29, sp, 0]
> <optional instructions>
>
> (similar to what was attempted in gcc/testsuite/gcc.target/arm/pr85434.c)
>
> I would like maintainers' input on whether such a testcase should be added
> and if there are better ways of checking for the instruction order.
>
> Bootstrapped and tested on aarch64-none-linux-gnu.
>
> Ok for trunk? If yes, I don't have any commit rights, so can someone please
> commit it on my behalf.
>
> Cheers,
> Syl
>
> gcc/ChangeLog:
>
> 2019-06-25 Sylvia Taylor <sylvia.tay...@arm.com>
>
> * config/aarch64/aarch64.md
> (tlsdesc_small_advsimd_<mode>): Update.
> (tlsdesc_small_sve_<mode>): Likewise.
> (FP_REGNUM): New.
>
Thanks, I've put this in. When writing ChangeLogs, you need a little
more detail than this. "New" and "Update" don't really explain the
change. I modified the ChangeLog entry as follows:
* config/aarch64/aarch64.md (FP_REGNUM): New constant.
(tlsdesc_small_advsimd_<mode>): Add use of FP_REGNUM.
(tlsdesc_small_sve_<mode>): Likewise.
R.
