| Message ID | 20200210192038.23588-8-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | 861be5fd6601bed58b63ae0eb23097abf1ac0e1c |
| Headers | show |
| Series | [01/15] powerpc: Consolidate Linux syscall definition | expand |
* Adhemerval Zanella: > +#define INTERNAL_SYSCALL_ERROR_P(val, err) \ > + ((unsigned long) (val) >= (unsigned long) -4095) This should use unsigned long int. Rest looks okay. Thanks, Florian
On Feb 10 2020, Adhemerval Zanella wrote: > diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h > index b02730bd23..eab888df32 100644 > --- a/sysdeps/unix/sysv/linux/nios2/sysdep.h > +++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h > @@ -157,13 +157,14 @@ > (int) result_var; }) > > #undef INTERNAL_SYSCALL_DECL > -#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused)) > +#define INTERNAL_SYSCALL_DECL(err) do { } while (0) > > #undef INTERNAL_SYSCALL_ERROR_P > -#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err)) > +#define INTERNAL_SYSCALL_ERROR_P(val, err) \ > + ((unsigned long) (val) >= (unsigned long) -4095) Perhaps -4095UL instead? Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
On 11/02/2020 08:20, Florian Weimer wrote: > * Adhemerval Zanella: > >> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \ >> + ((unsigned long) (val) >= (unsigned long) -4095) > > This should use unsigned long int. Rest looks okay. > > Thanks, > Florian > I didn't bother because this snippet will be removed by the 'linux: Consolidate INLINE_SYSCALL' patch in this set, but you are correct. I have changed to: #define INTERNAL_SYSCALL_ERROR_P(val, err) \ ((unsigned long) (val) > -4096UL) (which is the one I used on consolidation).
On 2/10/20 11:20 AM, Adhemerval Zanella wrote: > It changes the nios INTERNAL_SYSCALL_RAW macro to return a negative > value instead of 'r2' register value on 'err' macro argument. > > The macro INTERNAL_SYSCALL_DECL is no longer required, and the > INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS. > > Checked with a build against nios2-linux-gnu. > --- > sysdeps/unix/sysv/linux/nios2/sysdep.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h > index b02730bd23..eab888df32 100644 > --- a/sysdeps/unix/sysv/linux/nios2/sysdep.h > +++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h > @@ -157,13 +157,14 @@ > (int) result_var; }) > > #undef INTERNAL_SYSCALL_DECL > -#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused)) > +#define INTERNAL_SYSCALL_DECL(err) do { } while (0) > > #undef INTERNAL_SYSCALL_ERROR_P > -#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err)) > +#define INTERNAL_SYSCALL_ERROR_P(val, err) \ > + ((unsigned long) (val) >= (unsigned long) -4095) > > #undef INTERNAL_SYSCALL_ERRNO > -#define INTERNAL_SYSCALL_ERRNO(val, err) ((void) (err), val) > +#define INTERNAL_SYSCALL_ERRNO(val, err) (-(val)) > > #undef INTERNAL_SYSCALL_RAW > #define INTERNAL_SYSCALL_RAW(name, err, nr, args...) \ > @@ -180,8 +181,7 @@ > : "+r" (_r2), "=r" (_err) \ > : ASM_ARGS_##nr \ > : __SYSCALL_CLOBBERS); \ > - _sys_result = _r2; \ > - err = _err; \ > + _sys_result = _err != 0 ? -_r2 : -_r2; \ Is there a typo here ? both cases seem to be -ve > } \ > (int) _sys_result; }) > >
On 19/02/2020 18:40, Vineet Gupta wrote: > On 2/10/20 11:20 AM, Adhemerval Zanella wrote: >> It changes the nios INTERNAL_SYSCALL_RAW macro to return a negative >> value instead of 'r2' register value on 'err' macro argument. >> >> The macro INTERNAL_SYSCALL_DECL is no longer required, and the >> INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS. >> >> Checked with a build against nios2-linux-gnu. >> --- >> sysdeps/unix/sysv/linux/nios2/sysdep.h | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h >> index b02730bd23..eab888df32 100644 >> --- a/sysdeps/unix/sysv/linux/nios2/sysdep.h >> +++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h >> @@ -157,13 +157,14 @@ >> (int) result_var; }) >> >> #undef INTERNAL_SYSCALL_DECL >> -#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused)) >> +#define INTERNAL_SYSCALL_DECL(err) do { } while (0) >> >> #undef INTERNAL_SYSCALL_ERROR_P >> -#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err)) >> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \ >> + ((unsigned long) (val) >= (unsigned long) -4095) >> >> #undef INTERNAL_SYSCALL_ERRNO >> -#define INTERNAL_SYSCALL_ERRNO(val, err) ((void) (err), val) >> +#define INTERNAL_SYSCALL_ERRNO(val, err) (-(val)) >> >> #undef INTERNAL_SYSCALL_RAW >> #define INTERNAL_SYSCALL_RAW(name, err, nr, args...) \ >> @@ -180,8 +181,7 @@ >> : "+r" (_r2), "=r" (_err) \ >> : ASM_ARGS_##nr \ >> : __SYSCALL_CLOBBERS); \ >> - _sys_result = _r2; \ >> - err = _err; \ >> + _sys_result = _err != 0 ? -_r2 : -_r2; \ > > Is there a typo here ? both cases seem to be -ve It is, thanks for catching it. I have pushed b790c8c2ed to fix and double checked nios2 syscall handling (arch/nios2/kernel/entry.S:205) to certify that the modification does follow nios2 kABI.
Hi Adhemerval, On 2/20/20 5:14 AM, Adhemerval Zanella wrote: > > > On 19/02/2020 18:40, Vineet Gupta wrote: >> On 2/10/20 11:20 AM, Adhemerval Zanella wrote: >>> It changes the nios INTERNAL_SYSCALL_RAW macro to return a negative >>> value instead of 'r2' register value on 'err' macro argument. >>> >>> The macro INTERNAL_SYSCALL_DECL is no longer required, and the >>> INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS. >>> >>> Checked with a build against nios2-linux-gnu. >>> --- >>> sysdeps/unix/sysv/linux/nios2/sysdep.h | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h >>> index b02730bd23..eab888df32 100644 >>> --- a/sysdeps/unix/sysv/linux/nios2/sysdep.h >>> +++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h >>> @@ -157,13 +157,14 @@ >>> (int) result_var; }) >>> >>> #undef INTERNAL_SYSCALL_DECL >>> -#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused)) >>> +#define INTERNAL_SYSCALL_DECL(err) do { } while (0) >>> >>> #undef INTERNAL_SYSCALL_ERROR_P >>> -#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err)) >>> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \ >>> + ((unsigned long) (val) >= (unsigned long) -4095) >>> >>> #undef INTERNAL_SYSCALL_ERRNO >>> -#define INTERNAL_SYSCALL_ERRNO(val, err) ((void) (err), val) >>> +#define INTERNAL_SYSCALL_ERRNO(val, err) (-(val)) >>> >>> #undef INTERNAL_SYSCALL_RAW >>> #define INTERNAL_SYSCALL_RAW(name, err, nr, args...) \ >>> @@ -180,8 +181,7 @@ >>> : "+r" (_r2), "=r" (_err) \ >>> : ASM_ARGS_##nr \ >>> : __SYSCALL_CLOBBERS); \ >>> - _sys_result = _r2; \ >>> - err = _err; \ >>> + _sys_result = _err != 0 ? -_r2 : -_r2; \ >> >> Is there a typo here ? both cases seem to be -ve > > It is, thanks for catching it. I have pushed b790c8c2ed to fix and > double checked nios2 syscall handling (arch/nios2/kernel/entry.S:205) > to certify that the modification does follow nios2 kABI. Actually the reason I spotted it was trying to replicate similar changes in ARC port and it seems to be hosed now. It is quite likely a snaufu at my end, but I don't quite understand the new logic. Consider brk syscall which does __curbrk = (void *) INTERNAL_SYSCALL_CALL (brk, addr); Through a maze of defines this ends up calling INTERNAL_SYSCALL_RAW which seems be unconditionally converting !0 value (success) into -ve and returning it. So won't it convert a legit brk address return into a -ve and save in __curbrk. Am I not following this correctly ? Thx, -Vineet
On 2/20/20 12:39 PM, Vineet Gupta wrote:
> Am I not following this correctly ?
Oh never mind, they use 2 seperate regs to convey syscall result and error, so
your code is right.
On 20/02/2020 18:04, Vineet Gupta wrote: > > Through a maze of defines this ends up calling INTERNAL_SYSCALL_RAW which seems be > unconditionally converting !0 value (success) into -ve and returning it. So won't > it convert a legit brk address return into a -ve and save in __curbrk. > On 2/20/20 12:39 PM, Vineet Gupta wrote: >> Am I not following this correctly ? > > Oh never mind, they use 2 seperate regs to convey syscall result and error, so > your code is right. > One of my goals is to disentangle the {INTERNAL,INLINE}_SYSCALL macros to consolidate their definitions and move the arch-specific bits to inline functions instead of macros. Another one is to remove the requirement to define similar assembly macros to be used by the auto-generated one. The idea is an architecture will just need to define a set of inline_syscall{0-6} functions.
diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h index b02730bd23..eab888df32 100644 --- a/sysdeps/unix/sysv/linux/nios2/sysdep.h +++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h @@ -157,13 +157,14 @@ (int) result_var; }) #undef INTERNAL_SYSCALL_DECL -#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused)) +#define INTERNAL_SYSCALL_DECL(err) do { } while (0) #undef INTERNAL_SYSCALL_ERROR_P -#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err)) +#define INTERNAL_SYSCALL_ERROR_P(val, err) \ + ((unsigned long) (val) >= (unsigned long) -4095) #undef INTERNAL_SYSCALL_ERRNO -#define INTERNAL_SYSCALL_ERRNO(val, err) ((void) (err), val) +#define INTERNAL_SYSCALL_ERRNO(val, err) (-(val)) #undef INTERNAL_SYSCALL_RAW #define INTERNAL_SYSCALL_RAW(name, err, nr, args...) \ @@ -180,8 +181,7 @@ : "+r" (_r2), "=r" (_err) \ : ASM_ARGS_##nr \ : __SYSCALL_CLOBBERS); \ - _sys_result = _r2; \ - err = _err; \ + _sys_result = _err != 0 ? -_r2 : -_r2; \ } \ (int) _sys_result; })