diff mbox

[1/4] Add INTERNAL_SYSCALL_CALL

Message ID c2114f75-0e23-0636-32ed-0e879337a27d@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto Sept. 22, 2016, 1:42 p.m. UTC 
On 21/09/2016 16:22, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 20/09/2016 18:36, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> +#define __INTERNAL_SYSCALL0(name, err) \
>>>> +  INTERNAL_SYSCALL (name, err, 0)
>>>> +#define __INTERNAL_SYSCALL1(name, err, a1) \
>>>> +  INTERNAL_SYSCALL (name, err, 1, a1)
>>>> +#define __INTERNAL_SYSCALL2(name, err, a1, a2) \
>>>> +  INTERNAL_SYSCALL (name, err, 2, a1, a2)
>>>> +#define __INTERNAL_SYSCALL3(name, err, a1, a2, a3) \
>>>> +  INTERNAL_SYSCALL (name, err, 3, a1, a2, a3)
>>>> +#define __INTERNAL_SYSCALL4(name, err, a1, a2, a3, a4) \
>>>> +  INTERNAL_SYSCALL (name, err, 4, a1, a2, a3, a4)
>>>> +#define __INTERNAL_SYSCALL5(name, err, a1, a2, a3, a4, a5) \
>>>> +  INTERNAL_SYSCALL (name, err, 5, a1, a2, a3, a4, a5)
>>>> +#define __INTERNAL_SYSCALL6(name, err, a1, a2, a3, a4, a5, a6) \
>>>> +  INTERNAL_SYSCALL (name, err, 6, a1, a2, a3, a4, a5, a6)
>>>> +#define __INTERNAL_SYSCALL7(name, err, a1, a2, a3, a4, a5, a6, a7) \
>>>> +  INTERNAL_SYSCALL (name, err, 7, a1, a2, a3, a4, a5, a6, a7)
>>>
>>> It's not immediately obvious why these definitions are needed.
>>
>> I agree this is not obvious, but it follows the SYSCALL_CANCEL macro logic
>> where __INTERNAL_SYSCALL_DISP will select the correct __INTERNAL_SYSCALL
>> (based on number of arguments).
> 
> Is there anything that overrides inidivdual __INTERNAL_SYSCALLx
> macros?
> 
> What I mean is this:  Why can't this
> 
> +#define __INTERNAL_SYSCALL_DISP(b,err,...) \
> +  __INTERNAL_SYSCALL_CONCAT (b, __SYSCALL_NARGS (__VA_ARGS__)) \
> +			    (err, __VA_ARGS__)
> 
> turn into
> 
> +#define __INTERNAL_SYSCALL_DISP(b,err,...) \
> +  INTERNAL_SYSCALL (b, err, __SYSCALL_NARGS (__VA_ARGS__), __VA_ARGS__)
> 
> ?
> 

We can, at least for x86_64 for instance where it uses another indirection
for INTERNAL_SYSCALL.  However, something similar fails for i386, where
macro substitution for INTERNAL_SYSCALL will try string concatenation and
thus mess with intended behaviour.  Also, _SYSCALL_NARGS macro would be
required to be different to take in consideration the 'err' argument
required for INTERNAL syscall (something I noted I coded wrong).

I think calling the {INLINE,INTERNAL}_SYSCALL directly would be the safer 
and agnostic approach to avoid issues on how they are actually implemented 
by each port.

I tested the following patch with a build for practically all current
supported ports (aarch64, alpha, armeabi, armeaihf, hppa, ia64, i386, m68k,
microblaze, mips{32,64,n64}, nios2, powerpc{32,64,64le}, s390{-32,-64}, sh4,
sparc{64}, tile{pro,x64}, x86_64, and x32) and saw no build issues.  I also
checked on x86_64 and i386.  To actually check INTERNAL_SYSCALL_CALL macro
work I changed sysdeps/unix/sysv/linux/pthread_setaffinity.c to use it.

For below patch I changed it to the INLINE_SYSCALL macros would follow 
INLINE name and fixed the __INTERNAL_SYSCALL_NARGS argument selection
value.

--

Comments

Florian Weimer Sept. 22, 2016, 8:34 p.m. UTC | #1 
* Adhemerval Zanella:

> We can, at least for x86_64 for instance where it uses another indirection
> for INTERNAL_SYSCALL.  However, something similar fails for i386, where
> macro substitution for INTERNAL_SYSCALL will try string concatenation and
> thus mess with intended behaviour.  Also, _SYSCALL_NARGS macro would be
> required to be different to take in consideration the 'err' argument
> required for INTERNAL syscall (something I noted I coded wrong).
>
> I think calling the {INLINE,INTERNAL}_SYSCALL directly would be the safer 
> and agnostic approach to avoid issues on how they are actually implemented 
> by each port.

Okay, it looks like this is the better way for now.

> I tested the following patch with a build for practically all current
> supported ports (aarch64, alpha, armeabi, armeaihf, hppa, ia64, i386, m68k,
> microblaze, mips{32,64,n64}, nios2, powerpc{32,64,64le}, s390{-32,-64}, sh4,
> sparc{64}, tile{pro,x64}, x86_64, and x32) and saw no build issues.  I also
> checked on x86_64 and i386.  To actually check INTERNAL_SYSCALL_CALL macro
> work I changed sysdeps/unix/sysv/linux/pthread_setaffinity.c to use it.

Did you see object code changes from that?
Adhemerval Zanella Netto Sept. 23, 2016, 2:16 p.m. UTC | #2 
On 22/09/2016 17:34, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> We can, at least for x86_64 for instance where it uses another indirection
>> for INTERNAL_SYSCALL.  However, something similar fails for i386, where
>> macro substitution for INTERNAL_SYSCALL will try string concatenation and
>> thus mess with intended behaviour.  Also, _SYSCALL_NARGS macro would be
>> required to be different to take in consideration the 'err' argument
>> required for INTERNAL syscall (something I noted I coded wrong).
>>
>> I think calling the {INLINE,INTERNAL}_SYSCALL directly would be the safer 
>> and agnostic approach to avoid issues on how they are actually implemented 
>> by each port.
> 
> Okay, it looks like this is the better way for now.
> 
>> I tested the following patch with a build for practically all current
>> supported ports (aarch64, alpha, armeabi, armeaihf, hppa, ia64, i386, m68k,
>> microblaze, mips{32,64,n64}, nios2, powerpc{32,64,64le}, s390{-32,-64}, sh4,
>> sparc{64}, tile{pro,x64}, x86_64, and x32) and saw no build issues.  I also
>> checked on x86_64 and i386.  To actually check INTERNAL_SYSCALL_CALL macro
>> work I changed sysdeps/unix/sysv/linux/pthread_setaffinity.c to use it.
> 
> Did you see object code changes from that?

I haven't checked all of the, but at least x86_64, i386, aarch64, and powerpc64le
do not change.  I presume it is ok to push upstream, correct?
Florian Weimer Sept. 23, 2016, 8:39 p.m. UTC | #3 
* Adhemerval Zanella:

> On 22/09/2016 17:34, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> We can, at least for x86_64 for instance where it uses another indirection
>>> for INTERNAL_SYSCALL.  However, something similar fails for i386, where
>>> macro substitution for INTERNAL_SYSCALL will try string concatenation and
>>> thus mess with intended behaviour.  Also, _SYSCALL_NARGS macro would be
>>> required to be different to take in consideration the 'err' argument
>>> required for INTERNAL syscall (something I noted I coded wrong).
>>>
>>> I think calling the {INLINE,INTERNAL}_SYSCALL directly would be the safer 
>>> and agnostic approach to avoid issues on how they are actually implemented 
>>> by each port.
>> 
>> Okay, it looks like this is the better way for now.
>> 
>>> I tested the following patch with a build for practically all current
>>> supported ports (aarch64, alpha, armeabi, armeaihf, hppa, ia64, i386, m68k,
>>> microblaze, mips{32,64,n64}, nios2, powerpc{32,64,64le}, s390{-32,-64}, sh4,
>>> sparc{64}, tile{pro,x64}, x86_64, and x32) and saw no build issues.  I also
>>> checked on x86_64 and i386.  To actually check INTERNAL_SYSCALL_CALL macro
>>> work I changed sysdeps/unix/sysv/linux/pthread_setaffinity.c to use it.
>> 
>> Did you see object code changes from that?
>
> I haven't checked all of the, but at least x86_64, i386, aarch64, and
> powerpc64le

Great, thanks.

> do not change.  I presume it is ok to push upstream, correct?

I can't really tell you that the patch is totally unproblematic, but
all my concerns have been addressed.  Please push.
diff mbox

Patch

diff --git a/sysdeps/unix/sysdep.h b/sysdeps/unix/sysdep.h
index 94a2ce0..dfd3cfd 100644
--- a/sysdeps/unix/sysdep.h
+++ b/sysdeps/unix/sysdep.h
@@ -24,42 +24,79 @@ 
 #define	SYSCALL__(name, args)	PSEUDO (__##name, name, args)
 #define	SYSCALL(name, args)	PSEUDO (name, name, args)
 
-#define __SYSCALL0(name) \
+#define __SYSCALL_CONCAT_X(a,b)     a##b
+#define __SYSCALL_CONCAT(a,b)       __SYSCALL_CONCAT_X (a, b)
+
+
+#define __INTERNAL_SYSCALL0(name, err) \
+  INTERNAL_SYSCALL (name, err, 0)
+#define __INTERNAL_SYSCALL1(name, err, a1) \
+  INTERNAL_SYSCALL (name, err, 1, a1)
+#define __INTERNAL_SYSCALL2(name, err, a1, a2) \
+  INTERNAL_SYSCALL (name, err, 2, a1, a2)
+#define __INTERNAL_SYSCALL3(name, err, a1, a2, a3) \
+  INTERNAL_SYSCALL (name, err, 3, a1, a2, a3)
+#define __INTERNAL_SYSCALL4(name, err, a1, a2, a3, a4) \
+  INTERNAL_SYSCALL (name, err, 4, a1, a2, a3, a4)
+#define __INTERNAL_SYSCALL5(name, err, a1, a2, a3, a4, a5) \
+  INTERNAL_SYSCALL (name, err, 5, a1, a2, a3, a4, a5)
+#define __INTERNAL_SYSCALL6(name, err, a1, a2, a3, a4, a5, a6) \
+  INTERNAL_SYSCALL (name, err, 6, a1, a2, a3, a4, a5, a6)
+#define __INTERNAL_SYSCALL7(name, err, a1, a2, a3, a4, a5, a6, a7) \
+  INTERNAL_SYSCALL (name, err, 7, a1, a2, a3, a4, a5, a6, a7)
+
+#define __INTERNAL_SYSCALL_NARGS_X(a,b,c,d,e,f,g,h,n,...) n
+#define __INTERNAL_SYSCALL_NARGS(...) \
+  __INTERNAL_SYSCALL_NARGS_X (__VA_ARGS__,6,5,4,3,2,1,0,)
+#define __INTERNAL_SYSCALL_DISP(b,...) \
+  __SYSCALL_CONCAT (b,__INTERNAL_SYSCALL_NARGS(__VA_ARGS__))(__VA_ARGS__)
+
+/* Issue a syscall defined by syscall number plus any other argument required.
+   It is similar to INTERNAL_SYSCALL macro, but without the need to pass the
+   expected argument number as second parameter.  */
+#define INTERNAL_SYSCALL_CALL(...) \
+  __INTERNAL_SYSCALL_DISP (__INTERNAL_SYSCALL, __VA_ARGS__)
+
+#define __INLINE_SYSCALL0(name) \
   INLINE_SYSCALL (name, 0)
-#define __SYSCALL1(name, a1) \
+#define __INLINE_SYSCALL1(name, a1) \
   INLINE_SYSCALL (name, 1, a1)
-#define __SYSCALL2(name, a1, a2) \
+#define __INLINE_SYSCALL2(name, a1, a2) \
   INLINE_SYSCALL (name, 2, a1, a2)
-#define __SYSCALL3(name, a1, a2, a3) \
+#define __INLINE_SYSCALL3(name, a1, a2, a3) \
   INLINE_SYSCALL (name, 3, a1, a2, a3)
-#define __SYSCALL4(name, a1, a2, a3, a4) \
+#define __INLINE_SYSCALL4(name, a1, a2, a3, a4) \
   INLINE_SYSCALL (name, 4, a1, a2, a3, a4)
-#define __SYSCALL5(name, a1, a2, a3, a4, a5) \
+#define __INLINE_SYSCALL5(name, a1, a2, a3, a4, a5) \
   INLINE_SYSCALL (name, 5, a1, a2, a3, a4, a5)
-#define __SYSCALL6(name, a1, a2, a3, a4, a5, a6) \
+#define __INLINE_SYSCALL6(name, a1, a2, a3, a4, a5, a6) \
   INLINE_SYSCALL (name, 6, a1, a2, a3, a4, a5, a6)
-#define __SYSCALL7(name, a1, a2, a3, a4, a5, a6, a7) \
+#define __INLINE_SYSCALL7(name, a1, a2, a3, a4, a5, a6, a7) \
   INLINE_SYSCALL (name, 7, a1, a2, a3, a4, a5, a6, a7)
 
-#define __SYSCALL_NARGS_X(a,b,c,d,e,f,g,h,n,...) n
-#define __SYSCALL_NARGS(...) \
-  __SYSCALL_NARGS_X (__VA_ARGS__,7,6,5,4,3,2,1,0,)
-#define __SYSCALL_CONCAT_X(a,b)     a##b
-#define __SYSCALL_CONCAT(a,b)       __SYSCALL_CONCAT_X (a, b)
-#define __SYSCALL_DISP(b,...) \
-  __SYSCALL_CONCAT (b,__SYSCALL_NARGS(__VA_ARGS__))(__VA_ARGS__)
+#define __INLINE_SYSCALL_NARGS_X(a,b,c,d,e,f,g,h,n,...) n
+#define __INLINE_SYSCALL_NARGS(...) \
+  __INLINE_SYSCALL_NARGS_X (__VA_ARGS__,7,6,5,4,3,2,1,0,)
+#define __INLINE_SYSCALL_DISP(b,...) \
+  __SYSCALL_CONCAT (b,__INLINE_SYSCALL_NARGS(__VA_ARGS__))(__VA_ARGS__)
 
-#define __SYSCALL_CALL(...) __SYSCALL_DISP (__SYSCALL, __VA_ARGS__)
+/* Issue a syscall defined by syscall number plus any other argument
+   required.  Any error will be handled using arch defined macros and errno
+   will be set accordingly.
+   It is similar to INLINE_SYSCALL macro, but without the need to pass the
+   expected argument number as second parameter.  */
+#define INLINE_SYSCALL_CALL(...) \
+  __INLINE_SYSCALL_DISP (__INLINE_SYSCALL, __VA_ARGS__)
 
 #define SYSCALL_CANCEL(...) \
   ({									     \
     long int sc_ret;							     \
     if (SINGLE_THREAD_P) 						     \
-      sc_ret = __SYSCALL_CALL (__VA_ARGS__);   				     \
+      sc_ret = INLINE_SYSCALL_CALL (__VA_ARGS__); 			     \
     else								     \
       {									     \
 	int sc_cancel_oldtype = LIBC_CANCEL_ASYNC ();			     \
-	sc_ret = __SYSCALL_CALL (__VA_ARGS__);				     \
+	sc_ret = INLINE_SYSCALL_CALL (__VA_ARGS__);			     \
         LIBC_CANCEL_RESET (sc_cancel_oldtype);				     \
       }									     \
     sc_ret;