Re: [patch][aarch64]: fix frame pointer setup before tlsdesc call

[Richard Earnshaw <Richard dot Earnshaw at arm dot com>]

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.taylor@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.