Re: [PATCH 1/4] Add INTERNAL_SYSCALL_CALL

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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

> 
>> +#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__, 7, 6, 5, 4, 3, 2, 1, 0, )
>> +#define __INTERNAL_SYSCALL_CONCAT_X(a, b) a##b
>> +#define __INTERNAL_SYSCALL_CONCAT(a, b)   __SYSCALL_CONCAT_X (a, b)
> 
> I think you can reuse the macros from sysdeps/unix/sysdep.h here.

Indeed this is just a duplicate macro logic with different name.  I
think it is safe to remove.

> 
>> +/* Issue a syscall defined by syscall number plus any other argument required.
> 
> These comment lines are rather long.

Fixed.

> 
>> +   Any error will be handled using arch defined macros and errno will be se
>> +   accordingly.
> 
> “will be set”
> 

Fixed.

I think patch below simplify the macro code and address your comments:

--

diff --git a/sysdeps/unix/sysdep.h b/sysdeps/unix/sysdep.h
index 94a2ce0..7816b6a 100644
--- a/sysdeps/unix/sysdep.h
+++ b/sysdeps/unix/sysdep.h
@@ -24,6 +24,40 @@
 #define	SYSCALL__(name, args)	PSEUDO (__##name, name, args)
 #define	SYSCALL(name, args)	PSEUDO (name, name, args)
 
+/* Glue macros to select the correct {INTERNAL,INLINE}_SYSCALL variant based
+   on number of arguments used.  */
+#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 __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)
+
+/* 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(nr, err, ...) \
+  __SYSCALL_DISP (__INTERNAL_SYSCALL, nr, err, __VA_ARGS__)
+
 #define __SYSCALL0(name) \
   INLINE_SYSCALL (name, 0)
 #define __SYSCALL1(name, a1) \
@@ -41,25 +75,22 @@
 #define __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 __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(...) __SYSCALL_DISP (__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;								     \