[ping] Change static chain to r11 on aarch64

[ping] Change static chain to r11 on aarch64

[Olivier Hainque]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

Hello,

Ping for part of the changes last proposed here:

 https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00761.html

Submitted separately as an attempt to facilitate the review
process.

This piece is the change of static chain from r18 to r11.

Regression testing with languages=all on an aarch64-linux box
uncovered (various go related test failures) that libffi needs
a synchronized adjustment.

This is the updated version of the patch, so bootstrapped and
regression tested clean on aarch64-linux.

OK to commit ?

Thanks in advance!

With Kind Regards,

Olivier

2018-12-12  Olivier Hainque  <hainque@adacore.com>

	gcc/
	* config/aarch64/aarch64.h (STATIC_CHAIN_REGNUM): Use R11 instead
	of R18.

	gcc/testsuite/
	* gcc.dg/cwsc1.c: Expect r11 as the static chain on aarch64.

	libffi/
	* src/aarch64/sysv.S (ffi_call_SYSV): Expect r11 as the static chain.
	(ffi_go_closure_SYSV): Likewise.
	* testsuite/libffi.go/static-chain.h: Likewise.



[-- Attachment #2: 0003-Change-static-chain-from-r18-to-r11-on-aarch64.patch --]
[-- Type: application/octet-stream, Size: 3054 bytes --]

From 2fe2661e581e8e3989e62b81db6ca37d2392f83d Mon Sep 17 00:00:00 2001
From: Olivier Hainque <hainque@adacore.com>
Date: Wed, 12 Dec 2018 07:05:23 -0800
Subject: [PATCH 3/4] Change static chain from r18 to r11 on aarch64

	gcc/
	* config/aarch64/aarch64.h (STATIC_CHAIN_REGNUM): Use R11 instead
	of R18.

	gcc/testsuite/
	* gcc.dg/cwsc1.c: Expect r11 as the static chain on aarch64.

	libffi/
	* src/aarch64/sysv.S (ffi_call_SYSV): Expect r11 as the static chain.
	(ffi_go_closure_SYSV): Likewise.
	* testsuite/libffi.go/static-chain.h: Likewise.
---
 gcc/config/aarch64/aarch64.h              | 2 +-
 gcc/testsuite/gcc.dg/cwsc1.c              | 2 +-
 libffi/src/aarch64/sysv.S                 | 6 +++---
 libffi/testsuite/libffi.go/static-chain.h | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 0c833a8..a8063c2 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -422,7 +422,7 @@ extern unsigned aarch64_architecture_version;
    uses alloca.  */
 #define EXIT_IGNORE_STACK	(cfun->calls_alloca)
 
-#define STATIC_CHAIN_REGNUM		R18_REGNUM
+#define STATIC_CHAIN_REGNUM		R11_REGNUM
 #define HARD_FRAME_POINTER_REGNUM	R29_REGNUM
 #define FRAME_POINTER_REGNUM		SFP_REGNUM
 #define STACK_POINTER_REGNUM		SP_REGNUM
diff --git a/gcc/testsuite/gcc.dg/cwsc1.c b/gcc/testsuite/gcc.dg/cwsc1.c
index e793e26..be22317 100644
--- a/gcc/testsuite/gcc.dg/cwsc1.c
+++ b/gcc/testsuite/gcc.dg/cwsc1.c
@@ -6,7 +6,7 @@
 #elif defined(__i386__)
 # define CHAIN  "%ecx"
 #elif defined(__aarch64__)
-# define CHAIN  "x18"
+# define CHAIN  "x11"
 #elif defined(__alpha__)
 # define CHAIN  "$1"
 #elif defined(__arm__)
diff --git a/libffi/src/aarch64/sysv.S b/libffi/src/aarch64/sysv.S
index c1bf9b9..12e7ec3 100644
--- a/libffi/src/aarch64/sysv.S
+++ b/libffi/src/aarch64/sysv.S
@@ -89,7 +89,7 @@ CNAME(ffi_call_SYSV):
 	mov	x9, x2			/* save fn */
 	mov	x8, x3			/* install structure return */
 #ifdef FFI_GO_CLOSURES
-	mov	x18, x5			/* install static chain */
+	mov	x11, x5			/* install static chain */
 #endif
 	stp	x3, x4, [x29, #16]	/* save rvalue and flags */
 
@@ -415,8 +415,8 @@ CNAME(ffi_go_closure_SYSV):
 	stp     x6, x7, [sp, #16 + 16*N_V_ARG_REG + 48]
 
 	/* Load ffi_closure_inner arguments.  */
-	ldp	PTR_REG(0), PTR_REG(1), [x18, #PTR_SIZE]/* load cif, fn */
-	mov	x2, x18					/* load user_data */
+	ldp	PTR_REG(0), PTR_REG(1), [x11, #PTR_SIZE]/* load cif, fn */
+	mov	x2, x11					/* load user_data */
 	b	.Ldo_closure
 	cfi_endproc
 
diff --git a/libffi/testsuite/libffi.go/static-chain.h b/libffi/testsuite/libffi.go/static-chain.h
index e120eea..d22b037 100644
--- a/libffi/testsuite/libffi.go/static-chain.h
+++ b/libffi/testsuite/libffi.go/static-chain.h
@@ -1,5 +1,5 @@
 #ifdef __aarch64__
-# define STATIC_CHAIN_REG  "x18"
+# define STATIC_CHAIN_REG  "x11"
 #elif defined(__alpha__)
 # define STATIC_CHAIN_REG  "$1"
 #elif defined(__arm__)
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

Re: [ping] Change static chain to r11 on aarch64

[Richard Earnshaw (lists)]

On 12/12/2018 15:47, Olivier Hainque wrote:
> Hello,
> 
> Ping for part of the changes last proposed here:
> 
>  https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00761.html
> 
> Submitted separately as an attempt to facilitate the review
> process.
> 
> This piece is the change of static chain from r18 to r11.
> 
> Regression testing with languages=all on an aarch64-linux box
> uncovered (various go related test failures) that libffi needs
> a synchronized adjustment.
> 
> This is the updated version of the patch, so bootstrapped and
> regression tested clean on aarch64-linux.
> 
> OK to commit ?
> 
> Thanks in advance!
> 
> With Kind Regards,
> 
> Olivier
> 
> 2018-12-12  Olivier Hainque  <hainque@adacore.com>
> 
> 	gcc/
> 	* config/aarch64/aarch64.h (STATIC_CHAIN_REGNUM): Use R11 instead
> 	of R18.
> 
> 	gcc/testsuite/
> 	* gcc.dg/cwsc1.c: Expect r11 as the static chain on aarch64.
> 
> 	libffi/
> 	* src/aarch64/sysv.S (ffi_call_SYSV): Expect r11 as the static chain.
> 	(ffi_go_closure_SYSV): Likewise.
> 	* testsuite/libffi.go/static-chain.h: Likewise.
> 

libffi is a separate project; so a patch for that needs to be sent to
the libffi maintainers.  However, that introduces an issue that that
code is potentially used across multiple versions of gcc, with
potentially different choices of the static chain register.  Hmm, this
might need some more careful thought....

I'm also not keen on the fact that we are now seriously eating into the
space of call clobbered registers; what's the argument behind your
selection of r11 as opposed to any other register?  On Arm we manage
with IP; is there some reason why we can't do something similar on AArch64?

R.

> 
> 
> 0003-Change-static-chain-from-r18-to-r11-on-aarch64.patch
> 
> From 2fe2661e581e8e3989e62b81db6ca37d2392f83d Mon Sep 17 00:00:00 2001
> From: Olivier Hainque <hainque@adacore.com>
> Date: Wed, 12 Dec 2018 07:05:23 -0800
> Subject: [PATCH 3/4] Change static chain from r18 to r11 on aarch64
> 
> 	gcc/
> 	* config/aarch64/aarch64.h (STATIC_CHAIN_REGNUM): Use R11 instead
> 	of R18.
> 
> 	gcc/testsuite/
> 	* gcc.dg/cwsc1.c: Expect r11 as the static chain on aarch64.
> 
> 	libffi/
> 	* src/aarch64/sysv.S (ffi_call_SYSV): Expect r11 as the static chain.
> 	(ffi_go_closure_SYSV): Likewise.
> 	* testsuite/libffi.go/static-chain.h: Likewise.
> ---
>  gcc/config/aarch64/aarch64.h              | 2 +-
>  gcc/testsuite/gcc.dg/cwsc1.c              | 2 +-
>  libffi/src/aarch64/sysv.S                 | 6 +++---
>  libffi/testsuite/libffi.go/static-chain.h | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index 0c833a8..a8063c2 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -422,7 +422,7 @@ extern unsigned aarch64_architecture_version;
>     uses alloca.  */
>  #define EXIT_IGNORE_STACK	(cfun->calls_alloca)
>  
> -#define STATIC_CHAIN_REGNUM		R18_REGNUM
> +#define STATIC_CHAIN_REGNUM		R11_REGNUM
>  #define HARD_FRAME_POINTER_REGNUM	R29_REGNUM
>  #define FRAME_POINTER_REGNUM		SFP_REGNUM
>  #define STACK_POINTER_REGNUM		SP_REGNUM
> diff --git a/gcc/testsuite/gcc.dg/cwsc1.c b/gcc/testsuite/gcc.dg/cwsc1.c
> index e793e26..be22317 100644
> --- a/gcc/testsuite/gcc.dg/cwsc1.c
> +++ b/gcc/testsuite/gcc.dg/cwsc1.c
> @@ -6,7 +6,7 @@
>  #elif defined(__i386__)
>  # define CHAIN  "%ecx"
>  #elif defined(__aarch64__)
> -# define CHAIN  "x18"
> +# define CHAIN  "x11"
>  #elif defined(__alpha__)
>  # define CHAIN  "$1"
>  #elif defined(__arm__)
> diff --git a/libffi/src/aarch64/sysv.S b/libffi/src/aarch64/sysv.S
> index c1bf9b9..12e7ec3 100644
> --- a/libffi/src/aarch64/sysv.S
> +++ b/libffi/src/aarch64/sysv.S
> @@ -89,7 +89,7 @@ CNAME(ffi_call_SYSV):
>  	mov	x9, x2			/* save fn */
>  	mov	x8, x3			/* install structure return */
>  #ifdef FFI_GO_CLOSURES
> -	mov	x18, x5			/* install static chain */
> +	mov	x11, x5			/* install static chain */
>  #endif
>  	stp	x3, x4, [x29, #16]	/* save rvalue and flags */
>  
> @@ -415,8 +415,8 @@ CNAME(ffi_go_closure_SYSV):
>  	stp     x6, x7, [sp, #16 + 16*N_V_ARG_REG + 48]
>  
>  	/* Load ffi_closure_inner arguments.  */
> -	ldp	PTR_REG(0), PTR_REG(1), [x18, #PTR_SIZE]/* load cif, fn */
> -	mov	x2, x18					/* load user_data */
> +	ldp	PTR_REG(0), PTR_REG(1), [x11, #PTR_SIZE]/* load cif, fn */
> +	mov	x2, x11					/* load user_data */
>  	b	.Ldo_closure
>  	cfi_endproc
>  
> diff --git a/libffi/testsuite/libffi.go/static-chain.h b/libffi/testsuite/libffi.go/static-chain.h
> index e120eea..d22b037 100644
> --- a/libffi/testsuite/libffi.go/static-chain.h
> +++ b/libffi/testsuite/libffi.go/static-chain.h
> @@ -1,5 +1,5 @@
>  #ifdef __aarch64__
> -# define STATIC_CHAIN_REG  "x18"
> +# define STATIC_CHAIN_REG  "x11"
>  #elif defined(__alpha__)
>  # define STATIC_CHAIN_REG  "$1"
>  #elif defined(__arm__)
> 
> 
> 
> 

Re: [ping] Change static chain to r11 on aarch64

[Olivier Hainque]

Hi Richard,

Thanks for your feedback on this ! 

> On 12 Dec 2018, at 18:21, Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote:
> 
> libffi is a separate project; so a patch for that needs to be sent to
> the libffi maintainers.

Oh, I hadn't realized that.

>  However, that introduces an issue that that
> code is potentially used across multiple versions of gcc, with
> potentially different choices of the static chain register.  Hmm, this
> might need some more careful thought....

Indeed.

> I'm also not keen on the fact that we are now seriously eating into the
> space of call clobbered registers; what's the argument behind your
> selection of r11 as opposed to any other register?

No super strong motivation for r11 in particular.

The primary goal is to get to something else than r18 to
accommodate configurations where r18 is used by the runtime
environment, as allowed by the ABI and which at least VxWorks
leverages.

https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01125.html

suggested r9, then I discovered that r9 and r10 were used
by the stack probing mechanism, so I just picked the following
one that didn't seem to be used for other purposes already.

>  On Arm we manage
> with IP; is there some reason why we can't do something similar on AArch64?

I am unfortunately not familiar enough with the port to tell
out of the top of my head.

Maybe Kyrill, James or Wilco (cc'ed) would know ?

Re: [ping] Change static chain to r11 on aarch64

[Wilco Dijkstra]

Hi,

>> On 12 Dec 2018, at 18:21, Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote:
>
>>  However, that introduces an issue that that
>> code is potentially used across multiple versions of gcc, with
>> potentially different choices of the static chain register.  Hmm, this
>> might need some more careful thought....

The static chain is only used inside nested functions, so it's not an ABI but a
function-local agreement. Although it looks like you can take the address of
a nested function, I think you cannot ever export it in a way that exposes a
different static chain given each address-taken nested function would emit
its own trampoline on the stack.

In fact the trampoline implementation is broken by design since the stack
should not be executable by default.

>> I'm also not keen on the fact that we are now seriously eating into the
>> space of call clobbered registers; what's the argument behind your
>> selection of r11 as opposed to any other register?

The static chain register is only used on entry to a nested function. That's why
I suggested using x9 given x8 is the last argument register.

> suggested r9, then I discovered that r9 and r10 were used
> by the stack probing mechanism, so I just picked the following
> one that didn't seem to be used for other purposes already.

We could rename those temporaries if we think x9 is better than x11.

Wilco

    

Re: [ping] Change static chain to r11 on aarch64

[Uecker, Martin]

Am Mittwoch, den 12.12.2018, 18:53 +0000 schrieb Wilco Dijkstra:
> Hi,
> 
> > > On 12 Dec 2018,@18:21, Richard Earnshaw (lists) <Richard.Earnshaw@arm.com>
> 
> wrote:
> 
> > > However, that introduces an issue that that
> > > code is potentially used across multiple versions of gcc, with
> > > potentially different choices of the static chain register.  Hmm, this
> > > might need some more careful thought....
> 
> The static chain is only used inside nested functions, so it's not an ABI but a
> function-local agreement. Although it looks like you can take the address of
> a nested function, I think you cannot ever export it in a way that exposes a
> different static chain given each address-taken nested function would emit
> its own trampoline on the stack.
> 
> In fact the trampoline implementation is broken by design since the stack
> should not be executable by default.

Does a non-executable stack actually improve security?


For the alternative implementation using (custom) function
descriptors (-fno-trampolines) the static chain becomes
part of the ABI or not?

Best,
Martin



> > > I'm also not keen on the fact that we are now seriously eating into the
> > > space of call clobbered registers; what's the argument behind your
> > > selection of r11 as opposed to any other register?
> 
> The static chain register is only used on entry to a nested function.
> That's why I suggested using x9 given x8 is the last argument register.
> 
> > suggested r9, then I discovered that r9 and r10 were used
> > by the stack probing mechanism, so I just picked the following
> > one that didn't seem to be used for other purposes already.
> 
> We could rename those temporaries if we think x9 is better than x11.
> 
> Wilco
> 

Re: [ping] Change static chain to r11 on aarch64

[Wilco Dijkstra]

Hi Martin,

> Does a non-executable stack actually improve security?

Absolutely, it's like closing your front door rather than just leave it open
for anyone.

> For the alternative implementation using (custom) function
> descriptors (-fno-trampolines) the static chain becomes
> part of the ABI or not?

I've not seen such an alternative implementation (-fno-trampolines is
ignored on all targets I tried), but it wouldn't affect the ABI since you can
only take the address of a nested function when you're the parent function.

Wilco

Re: [ping] Change static chain to r11 on aarch64

[Uecker, Martin]

Hi Wilco,

Am Mittwoch, den 12.12.2018, 22:04 +0000 schrieb Wilco Dijkstra:
> Hi Martin,
> 
> > Does a non-executable stack actually improve security?
> 
> Absolutely, it's like closing your front door rather than just leave it open
> for anyone.

The question is whether it is like closing the front door
while leaving a window open. It makes it harder to
exploit a system but does not really prevent it.

> > For the alternative implementation using (custom) function
> > descriptors (-fno-trampolines) the static chain becomes
> > part of the ABI or not?
> 
> I've not seen such an alternative implementation (-fno-trampolines is
> ignored on all targets I tried),

It was implemented for Ada. But here is a patch to also
activate it for C:

https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00853.html

With this patch one can use nested functions in C without
having an executable stack.


>  but it wouldn't affect the ABI since you can
> only take the address of a nested function when you're
> the parent function.

But you can pass the address to another function. Without
trampolines, this other function needs to call the nested
function directly using the right ABI.

Best,
Martin

Re: [ping] Change static chain to r11 on aarch64

[Paul Koning]

> On Dec 12, 2018, at 5:12 PM, Uecker, Martin <Martin.Uecker@med.uni-goettingen.de> wrote:
>> ...
>> I've not seen such an alternative implementation (-fno-trampolines is
>> ignored on all targets I tried),
> 
> It was implemented for Ada. But here is a patch to also
> activate it for C:
> 
> https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00853.html
> 
> With this patch one can use nested functions in C without
> having an executable stack.

That's interesting, because the internals manual (section 18.11, "Support for nested functions") clearly implies that it's already supported so long as the target defines the necessary macros / target hooks.  Though admittedly the documentation is a bit muddled.

I've been wanting to use this, since executable stacks really need to be avoided on pdp11.

	paul

Re: [ping] Change static chain to r11 on aarch64

[Segher Boessenkool]

On Wed, Dec 12, 2018 at 10:04:11PM +0000, Wilco Dijkstra wrote:
> Hi Martin,
> 
> > Does a non-executable stack actually improve security?
> 
> Absolutely, it's like closing your front door rather than just leave it open
> for anyone.

On many Linux systems, if you use trampolines anywhere then the whole
stack will be mapped executable during the whole lifetime of the process.
*That* is not so good, of course.  But there is nothing wrong with having
some executable code on the stack, in principle.

> > For the alternative implementation using (custom) function
> > descriptors (-fno-trampolines) the static chain becomes
> > part of the ABI or not?
> 
> I've not seen such an alternative implementation (-fno-trampolines is
> ignored on all targets I tried), but it wouldn't affect the ABI since you can
> only take the address of a nested function when you're the parent function.

Also if you are in a (later) sibling:

===
void *p;
void f(void)
{
	void g(void)
	{
	}

	void h(void)
	{
		p = g;
	}

	h();
}
===


Segher

Re: [ping] Change static chain to r11 on aarch64

[Wilco Dijkstra]

Hi Martin,

Uecker, Martin wrote:
>Am Mittwoch, den 12.12.2018, 22:04 +0000 schrieb Wilco Dijkstra:
>> Hi Martin,
>> 
>> > Does a non-executable stack actually improve security?
>> 
>> Absolutely, it's like closing your front door rather than just leave it open
>> for anyone.
>
> The question is whether it is like closing the front door
> while leaving a window open. It makes it harder to
> exploit a system but does not really prevent it.

Security is never absolute, it's all about making it harder and more expensive
for attackers so they go after other, easier targets.

> It was implemented for Ada. But here is a patch to also
> activate it for C:
>
> https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00853.html
>
> With this patch one can use nested functions in C without
> having an executable stack.

I tried your patch and it seems to inline the code to load the static chain at every
indirect callsite. For Ada I don't think that is ABI (IIRC no separate compilation),
but for C it would create a new ABI.

>  but it wouldn't affect the ABI since you can
> only take the address of a nested function when you're
> the parent function.

> But you can pass the address to another function. Without
> trampolines, this other function needs to call the nested
> function directly using the right ABI.

Yes that was a really bad idea - function pointers with a descriptor should be explictly 
typed to avoid the need to use special trampolines.

If we didn't want to expose the static chain register as an ABI with -fno-trampolines,
we could use a helper function which could be made backwards compatible even
if one changes the static chain register (it just needs to set all of them!).

Wilco

Re: [ping] Change static chain to r11 on aarch64

[Uecker, Martin]

Hi Wilco,

Am Donnerstag, den 13.12.2018, 16:33 +0000 schrieb Wilco Dijkstra:
> Uecker, Martin wrote:
> > Am Mittwoch, den 12.12.2018, 22:04 +0000 schrieb Wilco Dijkstra:
> > > Hi Martin,
> > > 
> > > > Does a non-executable stack actually improve security?
> > > 
> > > Absolutely, it's like closing your front door rather than just leave it open
> > > for anyone.
> > 
> > The question is whether it is like closing the front door
> > while leaving a window open. It makes it harder to
> > exploit a system but does not really prevent it.
> 
> Security is never absolute, it's all about making it harder and more expensive
> for attackers so they go after other, easier targets.

One could also argue that it creates a false sense of security
and diverts resources from properly fixing the real problems
i.e. the buffer overflows which lets an attacker write to the
stack in the first place. A program without buffer overflows
is secure even without an executable stack and a program with
buffer overflows is still insecure even with a non-executable
stack.

> > It was implemented for Ada. But here is a patch to also
> > activate it for C:
> > 
> > https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00853.html
> > 
> > With this patch one can use nested functions in C without
> > having an executable stack.
> 
> I tried your patch and it seems to inline the code to load the static chain at every
> indirect callsite. For Ada I don't think that is ABI (IIRC no separate compilation),
> but for C it would create a new ABI.

I am a bit surprised that the static chain register is not
always already a fixed part of the ABI.

> >   but it wouldn't affect the ABI since you can
> > only take the address of a nested function when you're
> > the parent function.
> > But you can pass the address to another function. Without
> > trampolines, this other function needs to call the nested
> > function directly using the right ABI.
> 
> Yes that was a really bad idea - function pointers with a
> descriptor should be explictly  typed to avoid the need to
> use special trampolines.

This is essentially what Apple's block extension does. The
downside is that you cannot pass such pointers to existing code. 

> If we didn't want to expose the static chain register as an ABI
> with -fno-trampolines, we could use a helper function which could
> be made backwards compatible even if one changes the static chain
> register (it just needs to set all of them!).

Yes, this is a possibility. But I think it could simply be
fixed as part of the ABI.

Best,
Martin

Re: [ping] Change static chain to r11 on aarch64

[Wilco Dijkstra]

Hi Martin,

> One could also argue that it creates a false sense of security
> and diverts resources from properly fixing the real problems
> i.e. the buffer overflows which lets an attacker write to the
> stack in the first place. A program without buffer overflows
> is secure even without an executable stack and a program with
> buffer overflows is still insecure even with a non-executable
> stack.

Avoiding the root cause of the problem is practically impossible, even the
so called "safe" languages didn't turn out to be as safe as claimed after all...

So anything that is simple and free is absolutely worth doing, and far more
useful than mitigations that just slow things down but don't really improve
security. The _chk variants of various string functions are a total waste of
time for example and give a false sense of security. And yet people waste
a large amount of effort optimizing them in assembler code, even creating
ifuncs for them etc...

> I am a bit surprised that the static chain register is not
> always already a fixed part of the ABI.

Why? If every language or extension needed several special ABI registers you'd
run out of registers pretty quickly and the ABI would be too complex to
implement. The this-pointer in C++ doesn't need a special ABI register - likewise
a static chain is just an extra argument.

>> If we didn't want to expose the static chain register as an ABI
>> with -fno-trampolines, we could use a helper function which could
>> be made backwards compatible even if one changes the static chain
>> register (it just needs to set all of them!).
>
> Yes, this is a possibility. But I think it could simply be
> fixed as part of the ABI.

Yes we could just pick a register now since this feature won't be used much.

Wilco

Re: [ping] Change static chain to r11 on aarch64

[Hans-Peter Nilsson]

On Wed, 12 Dec 2018, Wilco Dijkstra wrote:
> I've not seen such an alternative implementation (-fno-trampolines is
> ignored on all targets I tried), but it wouldn't affect the ABI since you can
> only take the address of a nested function when you're the parent function.

Regarding the trampoline-code-on-executable-stack variant of
nested functions, don't miss one fine detail:

While the choice of static-chain register does not affect the
ABI, it's the other way round: the choice of static-chain
register matters, specifically it's call-clobberedness.

The sublime port (to invent a soundalike term for port
maintainership alluding to unused terminology) is recommended to
pick a call-clobbered register that isn't used to carry any odd
special part of a function argument (including but not
restricted to regular function arguments and the
struct-value-regnum).

It looks like the current aarch64 static-chain register R18 is
call-saved but without special provisions to save and restore
the static chain register, i.e. the port is broken wrt.
trampolines but may appear to work (likely as-if you got the
call-clobberedness wrong for a special case; I haven't
investigated).  I understand the i386 port gets this right.
The CRIS port does not, but attempts and adds another bug (you
can't use the trampoline as a register-save area on return).

So, changing from R18 to R11 for aarch64 seems right, as the
latter is call-clobbered and the former is call-saved IIUC.

Unless of course I and grep both miss something or other.

brgds, H-P

Re: [ping] Change static chain to r11 on aarch64

[Wilco Dijkstra]

Hi Hans-Peter,

> While the choice of static-chain register does not affect the
> ABI, it's the other way round: the choice of static-chain
> register matters, specifically it's call-clobberedness.

Agreed.

> It looks like the current aarch64 static-chain register R18 is
> call-saved but without special provisions to save and restore
> the static chain register, i.e. the port is broken wrt.
> trampolines but may appear to work (likely as-if you got the
> call-clobberedness wrong for a special case; I haven't
> investigated).  I understand the i386 port gets this right.
> The CRIS port does not, but attempts and adds another bug (you
> can't use the trampoline as a register-save area on return).

> So, changing from R18 to R11 for aarch64 seems right, as the
> latter is call-clobbered and the former is call-saved IIUC.

The AArch64 ABI defines x18 as platform specific:
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf
On Linux it is call-clobbered, but it could be a fixed register on other
platforms (eg. a thread-local pointer). I don't think it's possible to make
it a callee-save. Still it is the wrong register to use since it already has
different uses. Using x9 would make its use as an extra argument clearer.

Wilco

Re: [ping] Change static chain to r11 on aarch64

[Hans-Peter Nilsson]

On Mon, 17 Dec 2018, Wilco Dijkstra wrote:
> H-P:
> > So, changing from R18 to R11 for aarch64 seems right, as the
> > latter is call-clobbered and the former is call-saved IIUC.
>
> The AArch64 ABI defines x18 as platform specific:
> http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf
> On Linux it is call-clobbered, but it could be a fixed register on other
> platforms (eg. a thread-local pointer). I don't think it's possible to make
> it a callee-save.

JFTR, in gcc, it's treated as call-saved, AFAICS, with no
special-casing (other than being STATIC_CHAIN_REGNUM, which
by itself has no other effect).

Is that a bug or deliberate?  If deliberate, a comment at
CALL_USED_REGISTERS would have cleared that question.
The document you refer to seems to be on the fence regarding
whether a compiler should treat it as call-clobbered.

> Still it is the wrong register to use since it already has
> different uses. Using x9 would make its use as an extra argument clearer.

Sure.  For one, it would avoid the bug with
STATIC_CHAIN_REGNUM being call-saved. :)

brgds, H-P

Re: [ping] Change static chain to r11 on aarch64

[Hans-Peter Nilsson]

On Mon, 17 Dec 2018, Hans-Peter Nilsson wrote:
> On Mon, 17 Dec 2018, Wilco Dijkstra wrote:
> > H-P:
> > > So, changing from R18 to R11 for aarch64 seems right, as the
> > > latter is call-clobbered and the former is call-saved IIUC.
> >
> > The AArch64 ABI defines x18 as platform specific:
> > http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf
> > On Linux it is call-clobbered, but it could be a fixed register on other
> > platforms (eg. a thread-local pointer). I don't think it's possible to make
> > it a callee-save.
>
> JFTR, in gcc, it's treated as call-saved, AFAICS, with no
> special-casing (other than being STATIC_CHAIN_REGNUM, which
> by itself has no other effect).

I take that back,  My bad reading.  Sorry for the noise.

brgds, H-P

Re: [ping] Change static chain to r11 on aarch64

[Olivier Hainque]

Hi Wilco,

(and thanks everyone for the interesting input on this)

> On 17 Dec 2018, at 14:55, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> 
> The AArch64 ABI defines x18 as platform specific:
[...]
> Using x9 would make its use as an extra argument clearer.

I'm experimenting with the idea of adjusting the
stack probing code using r9 today, to see if it could
save/restore that reg if it happens to be the static chain
as well.

If that can be made to work, maybe that would be a better
alternative than just swapping and have the stack probing
code use r10 and r11 instead (1 fewer register with dedicated
use).

Olivier

Re: [ping] Change static chain to r11 on aarch64

[Wilco Dijkstra]

Hi Olivier,

> I'm experimenting with the idea of adjusting the
> stack probing code using r9 today, to see if it could
> save/restore that reg if it happens to be the static chain
> as well.
>
> If that can be made to work, maybe that would be a better
> alternative than just swapping and have the stack probing
> code use r10 and r11 instead (1 fewer register with dedicated
> use).

Remember these are just temporaries for use in the prolog and epilog -
there is no need to save/restore the static base. Setting static chain
to x9 and the temporaries to x10/x11 is the simplest solution. We
can separately look at why the prolog uses more than a single
temporary.

Cheers,
Wilco

    

Re: [ping] Change static chain to r11 on aarch64

[Olivier Hainque]

Hi Wilco,


> On 21 Dec 2018, at 19:02, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> 
> Remember these are just temporaries for use in the prolog and epilog -
> there is no need to save/restore the static base. Setting static chain
> to x9 and the temporaries to x10/x11 is the simplest solution. We
> can separately look at why the prolog uses more than a single
> temporary.

Ok, I'm happy to test that :-)

With local adjustments to libffi for starters.

Thanks for your feedback!

Re: Change static chain from r18 to r9 (not r11) on aarch64

[Olivier Hainque]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

Hi Wilco,

> On 23 Dec 2018, at 12:09, Olivier Hainque <hainque@adacore.com> wrote:
> 
>> Remember these are just temporaries for use in the prolog and epilog -
>> there is no need to save/restore the static base. Setting static chain
>> to x9 and the temporaries to x10/x11 is the simplest solution. We
>> can separately look at why the prolog uses more than a single
>> temporary.
> 
> Ok, I'm happy to test that :-)
> 
> With local adjustments to libffi for starters.

Finally back on this after a Christmas break,
some time away on travel, then an oversight in the patch
which required extra adjustment & testing cycles.

The attached path works fine for our gcc-8 based
Ada toolchains on VxWorks, and bootstraps + regression tests
fine with a recent mainline on aarch64-linux configured with
--enable-languages=all.

I understand that this is not stage 4 material and
whatever we want to do in this area will probably have
to wait until stage 1 reopens.

I also understand that the libffi parts are to
be proposed to the libffi project, so some synchronization
will be needed on this part at least.

So, keeping the ball rolling ...

Thanks in advance for your input!

With Kind Regards,

Olivier

2019-01-22  Olivier Hainque  <hainque@adacore.com>

       Change static chain from r18 to r9 on aarch64

        gcc/
        * config/aarch64/aarch64.h (STATIC_CHAIN_REGNUM): Use r9 instead
        of r18.
        * config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG,
        PROBE_STACK_SECOND_REG): Use r10/r11 instead r9/r10.

        gcc/testsuite/
        * gcc.dg/cwsc1.c: Expect r9 as the static chain on aarch64.

        libffi/
        * src/aarch64/sysv.S (ffi_call_SYSV): Expect r9 as the static chain.
        (ffi_go_closure_SYSV): Likewise.
        * testsuite/libffi.go/static-chain.h: Likewise.


[-- Attachment #2: aarch64-static-chain.diff --]
[-- Type: application/octet-stream, Size: 3789 bytes --]

commit 40f08c3374d4a0ca6b703682263275f4c0477095
Author: Olivier Hainque <hainque@adacore.com>
Date:   Wed Dec 12 07:05:23 2018 -0800

    Change static chain from r18 to r9 on aarch64
    
    	gcc/
    	* config/aarch64/aarch64.h (STATIC_CHAIN_REGNUM): Use r9 instead
    	of r18.
    	* config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG,
    	PROBE_STACK_SECOND_REG): Use r10/r11 instead r9/r10.
    
    	gcc/testsuite/
    	* gcc.dg/cwsc1.c: Expect r9 as the static chain on aarch64.
    
    	libffi/
    	* src/aarch64/sysv.S (ffi_call_SYSV): Expect r9 as the static chain.
    	(ffi_go_closure_SYSV): Likewise.
    	* testsuite/libffi.go/static-chain.h: Likewise.

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 1c45243..67ace48 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -4036,8 +4036,8 @@ aarch64_libgcc_cmp_return_mode (void)
 #endif
 
 /* The pair of scratch registers used for stack probing.  */
-#define PROBE_STACK_FIRST_REG  R9_REGNUM
-#define PROBE_STACK_SECOND_REG R10_REGNUM
+#define PROBE_STACK_FIRST_REG  R10_REGNUM
+#define PROBE_STACK_SECOND_REG R11_REGNUM
 
 /* Emit code to probe a range of stack addresses from FIRST to FIRST+POLY_SIZE,
    inclusive.  These are offsets from the current stack pointer.  */
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 2617a8c..77fd10c 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -416,7 +416,7 @@ extern unsigned aarch64_architecture_version;
    uses alloca.  */
 #define EXIT_IGNORE_STACK	(cfun->calls_alloca)
 
-#define STATIC_CHAIN_REGNUM		R18_REGNUM
+#define STATIC_CHAIN_REGNUM		R9_REGNUM
 #define HARD_FRAME_POINTER_REGNUM	R29_REGNUM
 #define FRAME_POINTER_REGNUM		SFP_REGNUM
 #define STACK_POINTER_REGNUM		SP_REGNUM
diff --git a/gcc/testsuite/gcc.dg/cwsc1.c b/gcc/testsuite/gcc.dg/cwsc1.c
index e793e26..6972110 100644
--- a/gcc/testsuite/gcc.dg/cwsc1.c
+++ b/gcc/testsuite/gcc.dg/cwsc1.c
@@ -6,7 +6,7 @@
 #elif defined(__i386__)
 # define CHAIN  "%ecx"
 #elif defined(__aarch64__)
-# define CHAIN  "x18"
+# define CHAIN  "x9"
 #elif defined(__alpha__)
 # define CHAIN  "$1"
 #elif defined(__arm__)
diff --git a/libffi/src/aarch64/sysv.S b/libffi/src/aarch64/sysv.S
index c1bf9b9..a20fee2 100644
--- a/libffi/src/aarch64/sysv.S
+++ b/libffi/src/aarch64/sysv.S
@@ -86,10 +86,10 @@ CNAME(ffi_call_SYSV):
 	cfi_rel_offset (x29, 0)
 	cfi_rel_offset (x30, 8)
 
-	mov	x9, x2			/* save fn */
+	mov	x10, x2			/* save fn */
 	mov	x8, x3			/* install structure return */
 #ifdef FFI_GO_CLOSURES
-	mov	x18, x5			/* install static chain */
+	mov	x9, x5			/* install static chain */
 #endif
 	stp	x3, x4, [x29, #16]	/* save rvalue and flags */
 
@@ -110,7 +110,7 @@ CNAME(ffi_call_SYSV):
 	/* Deallocate the context, leaving the stacked arguments.  */
 	add	sp, sp, #CALL_CONTEXT_SIZE
 
-	blr     x9			/* call fn */
+	blr     x10			/* call fn */
 
 	ldp	x3, x4, [x29, #16]	/* reload rvalue and flags */
 
@@ -415,8 +415,8 @@ CNAME(ffi_go_closure_SYSV):
 	stp     x6, x7, [sp, #16 + 16*N_V_ARG_REG + 48]
 
 	/* Load ffi_closure_inner arguments.  */
-	ldp	PTR_REG(0), PTR_REG(1), [x18, #PTR_SIZE]/* load cif, fn */
-	mov	x2, x18					/* load user_data */
+	ldp	PTR_REG(0), PTR_REG(1), [x9, #PTR_SIZE]	/* load cif, fn */
+	mov	x2, x9					/* load user_data */
 	b	.Ldo_closure
 	cfi_endproc
 
diff --git a/libffi/testsuite/libffi.go/static-chain.h b/libffi/testsuite/libffi.go/static-chain.h
index e120eea..111cbe9 100644
--- a/libffi/testsuite/libffi.go/static-chain.h
+++ b/libffi/testsuite/libffi.go/static-chain.h
@@ -1,5 +1,5 @@
 #ifdef __aarch64__
-# define STATIC_CHAIN_REG  "x18"
+# define STATIC_CHAIN_REG  "x9"
 #elif defined(__alpha__)
 # define STATIC_CHAIN_REG  "$1"
 #elif defined(__arm__)