powerpc Linux scv support and scv system call ABI proposal

[Adhemerval Zanella]

On 30/01/2020 18:41, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Jan 30, 2020 at 02:04:51PM -0300, Adhemerval Zanella wrote:
>> On 30/01/2020 10:50, Segher Boessenkool wrote:
>>> On Thu, Jan 30, 2020 at 01:03:53PM +0100, Florian Weimer wrote:
>>>>> This is why that *is* the only supported use.  The documentation could
>>>>> use a touch-up, I think.  Unless we still have problems here?
>>>>
>>>> I really don't know.  GCC still has *some* support for the old behavior,
>>>> though.
>>>
>>> No.  No support.  It still does some of the same things, but that can
>>> change (and probably should).  But this hasn't been supported since the
>>> dark ages, and the documentation has become gradually more explicit
>>> about it.
>>>
>>
>> I think this might be related to an odd sparc32 issue I am seeing with 
>> newer clock_nanosleep.  The expanded code is:
>>
>> --
>>   register long err __asm__("g1");                                   // INTERNAL_SYSCALL_DECL  (err)
>>   r = ({                                                             // r = INTERNAL_SYSCALL_CANCEL (...)
>> 	 long int sc_ret;
>>          if (SINGLE_THREAD_P)
>> 	   sc_ret = INTERNAL_SYSCALL_CALL (__VA_ARGS__);
>>          else
>>            {
>> 	     int sc_cancel_oldtype = __libc_enable_asynccancel ();
>> 	     sc_ret = INTERNAL_SYSCALL_CALL (__VA_ARGS__);          // It issues the syscall with the asm (...)
>> 	     __librt_disable_asynccancel (sc_cancel_oldtype);
>> 	   }
>>          sc_ret;
>>        });
>>   if ((void) (val), __builtin_expect((err) != 0, 0))                // if (! INTERNAL_SYSCALL_ERROR_P (r, err))
>>     return 0;
>>   if ((-(val)) != ENOSYS)                                           // if (INTERNAL_SYSCALL_ERRNO (r, err) != ENOSYS)
>>     return ((-(val)));                                              //   return INTERNAL_SYSCALL_ERRNO (r, err);
>>
>>   [...]
>>
>>   r = ({                                                             // r = INTERNAL_SYSCALL_CANCEL (...)
>>        [...]
>>       )}
>>   if ((void) (val), __builtin_expect((err) != 0, 0))                // if (! INTERNAL_SYSCALL_ERROR_P (r, err))
>>     {
>>       [...]
>>     }
>>   return ((void) (val), __builtin_expect((err) != 0, 0))            // return (INTERNAL_SYSCALL_ERROR_P (r, err)
>>          ? ((-(val))) : 0;                                          //        ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
>> --
>>
>> It requires that 'err' (assigned to 'g1')
> 
> What do you mean by "assigned to g1"?

I meant 'err' being a local register variable.

> 
>> be value propagated over
>> functions calls and over different scopes, which I take from your 
>> explanation is not supported and fragile.
> 
> You probably misundertand that, but let me ask: where is err assigned to
> at all in the code you quoted?  I don't see it.  Maybe it's hidden in some
> macro?

Indeed it was not explicit in the example code, it is burried in the
INTERNAL_SYSCALL_CALL macro which calls sparc-defined macros. For instance, 
with 6 argument kernel syscall, it issues:

#define inline_syscall6(string,err,name,arg1,arg2,arg3,arg4,arg5,arg6)  \
({                                                                      \
        register long __o0 __asm__ ("o0") = (long)(arg1);               \
        register long __o1 __asm__ ("o1") = (long)(arg2);               \
        register long __o2 __asm__ ("o2") = (long)(arg3);               \
        register long __o3 __asm__ ("o3") = (long)(arg4);               \
        register long __o4 __asm__ ("o4") = (long)(arg5);               \
        register long __o5 __asm__ ("o5") = (long)(arg6);               \
        err = name;                                                     \
        __asm __volatile (string : "=r" (err), "=r" (__o0) :            \
                          "0" (err), "1" (__o0), "r" (__o1),            \
                          "r" (__o2), "r" (__o3), "r" (__o4),           \
                          "r" (__o5) :                                  \
                          __SYSCALL_CLOBBERS);                          \
        __o0;                                                           \
})

Where 'err' defined by 'INTERNAL_SYSCALL_DECL' should be the 'err' macro
argument.

> 
> Or, maybe some asm writes to g1?  This is explicitly not okay (quote
> from the GCC manual):

Yes, that's the case.

> 
>   Defining a register variable does not reserve the register.  Other than
>   when invoking the Extended 'asm', the contents of the specified register
>   are not guaranteed.  For this reason, the following uses are explicitly
>   _not_ supported.  If they appear to work, it is only happenstance, and
>   may stop working as intended due to (seemingly) unrelated changes in
>   surrounding code, or even minor changes in the optimization of a future
>   version of gcc:
> 
>    * Passing parameters to or from Basic 'asm'
>    * Passing parameters to or from Extended 'asm' without using input or
>      output operands.
>    * Passing parameters to or from routines written in assembler (or
>      other languages) using non-standard calling conventions.
> 
>> It also seems that if I 
>> move the __libc_enable_* calls before 'err' initialization and after
>> its usage the code seems to works, but again it seems this usage
>> is not really supported on gcc.
>>
>> So it seems that the current usage of 'INTERNAL_SYSCALL_DECL' and
>> 'INTERNAL_SYSCALL_ERROR_P' are fragile if the architecture *does*
>> use the 'err' variable and it is defined a register alias (which 
>> its the case only for sparc currently).
>>
>> Although a straightforward for sparc would be redefine 
>> INTERNAL_SYSCALL_DECL to not use a register alias, I still think
>> we should just follow Linux kernel ABI convention where value in 
>> the range between -4095 and -1 indicates an error and handle any 
>> specific symbols that might not strictly follow it with an 
>> arch-specific implementation (as we do for lseek on x32 and
>> mips64n32).  It would allow cleanup a lot of code and avoid such
>> pitfalls.
> 
> I don't really understand what you call a "register alias", either.
> (And i don't know the Sparc ABI well enough to help you with that).

I meant a register variable where its use 'after' the extended asm
is expected to use the define register.