Re: powerpc Linux scv support and scv system call ABI proposal

[Segher Boessenkool <segher@kernel.crashing.org>]

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"?

> 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?

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

  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).


Segher