Re: [PATCH] S390: Do not clobber r7 in clone [BZ #31402]

[Stefan Liebler <stli@linux.ibm.com>]

On 21.02.24 17:31, Jakub Jelinek wrote:
> On Wed, Feb 21, 2024 at 05:13:07PM +0100, Stefan Liebler wrote:
>> Starting with commit e57d8fc97b90127de4ed3e3a9cdf663667580935
>> "S390: Always use svc 0"
>> clone clobbers the call-saved register r7 in error case:
>> function or stack is NULL.
>>
>> This patch restores the saved registers also in the error case.
> 
> You could also just restore just %r7 and not both %r6 and %r7,
> because only %r7 has been modified.  But no idea what is actually
> faster (and if equally fast what is smaller), especially when the
> saving has been done using stm/stmg of the pair.
Yes, you are right, restoring r7 is enough. Nevertheless I prefer that
the error-label is equal to the remaining code.
> 
> Also, wonder if there shouldn't be a testcase covering it, just
> call clone (NULL, NULL, ...) and verify that it returned < 0/EINVAL
> and try to create high register preassure in the function across the
> call and make sure it fails without the patch and succeeds with it?
> 
> Like below, though it will need to be adjusted for the glibc ways of doing
> testcases (instead of abort do what is usual for test failures, etc.),
> plus make sure it is tested only on arches which actually have clone (i.e.
> Linux).
I've extended the already existing testcase for all error cases and
added your suggested register clobber test. It fails without the fix and
passes with it.

Here is V2:
[PATCH v2] S390: Do not clobber r7 in clone [BZ #31402]
https://sourceware.org/pipermail/libc-alpha/2024-February/154911.html
> 
> #define _GNU_SOURCE
> #include <stdlib.h>
> #include <sched.h>
> #include <signal.h>
> #include <errno.h>
> 
> volatile unsigned v = 0xdeadbeef;
> 
> int
> main ()
> {
>   unsigned int a = v;
>   unsigned int b = v;
>   unsigned int c = v;
>   unsigned int d = v;
>   unsigned int e = v;
>   unsigned int f = v;
>   unsigned int g = v;
>   unsigned int h = v;
>   unsigned int i = v;
>   unsigned int j = v;
>   unsigned int k = v;
>   unsigned int l = v;
>   unsigned int m = v;
>   unsigned int n = v;
>   unsigned int o = v;
>   if (clone (NULL, NULL, SIGCHLD, NULL) >= 0 || errno != EINVAL)
>     abort ();
>   if (a != v || b != v || c != v || d != v || e != v
>       || f != v || g != v || h != v || i != v || j != v
>       || k != v || l != v || m != v || n != v || o != v)
>     abort ();
> }
> 
> 	Jakub
>