V2 [PATCH 1/2] Add SYSCALL_ULONG_ARG_[12] to pass long to syscall [BZ #25810]

Wed Apr 22 11:44:20 GMT 2020

On 22/04/2020 07:42, Florian Weimer wrote:
> * H. J. Lu via Libc-alpha:
> 
>> diff --git a/sysdeps/unix/make-syscalls.sh b/sysdeps/unix/make-syscalls.sh
>> index c07626677f..4a28badea6 100644
>> --- a/sysdeps/unix/make-syscalls.sh
>> +++ b/sysdeps/unix/make-syscalls.sh
>> @@ -30,6 +30,7 @@
>>  # P: optionally-NULL pointer to typed object (e.g., 3rd argument to sigaction)
>>  # s: non-NULL string (e.g., 1st arg to open)
>>  # S: optionally-NULL string (e.g., 1st arg to acct)
>> +# U: unsigned long
> 
> Maybe this?
> 
> # U: unsigned long int (32-bit types are zero-extended to 64-bit types)
> 
>>  # v: vararg scalar (e.g., optional 3rd arg to open)
>>  # V: byte-per-page vector (3rd arg to mincore)
>>  # W: wait status, optionally-NULL pointer to int (e.g., 2nd arg of wait4)
>> @@ -184,6 +185,91 @@ while read file srcfile caller syscall args strong weak; do
>>    ?:?????????) nargs=9;;
>>    esac
>>  
>> +  # Derive the unsigned long arguments from the argument signature
> 
> “unsigned long int”
> 
>> +  ulong_arg_1=0
>> +  ulong_arg_2=0
>> +  case $args in
>> +  ?:U*)
>> +    ulong_arg_1=1
>> +    case $args in
>> +    ?:UU*) ulong_arg_2=2;;
>> +    ?:U?U*) ulong_arg_2=3;;
>> +    ?:U??U*) ulong_arg_2=4;;
>> +    ?:U???U*) ulong_arg_2=5;;
>> +    ?:U????U*) ulong_arg_2=6;;
>> +    ?:U?????U*) ulong_arg_2=7;;
>> +    ?:U??????U*) ulong_arg_2=8;;
>> +    ?:U???????U) ulong_arg_2=9;;
>> +    esac
>> +    ;;
> 
>> +  ?:????????U)
>> +    ulong_arg_1=9
>> +    ;;
>> +  esac
>> +
> 
> I must say that I find this really, really ugly.  We should rewrite
> this in Python as soon as possible (in a separate patch).

And I think we should make long term gold to just get rid of this
assembly macro and focus on automatic generation to a C code file
as well.

> 
> You could try this instead:
> 
> $ echo U1U | grep -ob U
> 0:U
> 2:U
> 
> And maybe guard it with a case match, so that performance does not
> suffer too much.
> 
> In any case, there should be an error check that covers the more
> than-two-Us case.
> 
> 
>>    # Make sure only the first syscall rule is used, if multiple dirs
>>    # define the same syscall.
>>    echo ''
>> @@ -245,6 +331,8 @@ while read file srcfile caller syscall args strong weak; do
>>  	\$(make-target-directory)
>>  	(echo '#define SYSCALL_NAME $syscall'; \\
>>  	 echo '#define SYSCALL_NARGS $nargs'; \\
>> +	 echo '#define SYSCALL_ULONG_ARG_1 $ulong_arg_1'; \\
>> +	 echo '#define SYSCALL_ULONG_ARG_2 $ulong_arg_2'; \\
> 
> Should this be conditional on whether $ulong_arg_1 and $ulong_arg_2
> are empty?  I think that might be less confusing.
> 
> Otherwise, the comment at the beginning should mention the special
> value zero.
> 
>>  	 echo '#define SYSCALL_SYMBOL $strong'; \\
>>  	 echo '#define SYSCALL_NOERRNO $noerrno'; \\
>>  	 echo '#define SYSCALL_ERRVAL $errval'; \\
>> diff --git a/sysdeps/unix/syscall-template.S b/sysdeps/unix/syscall-template.S
>> index cf6c7a58fb..0824a3c61e 100644
>> --- a/sysdeps/unix/syscall-template.S
>> +++ b/sysdeps/unix/syscall-template.S
>> @@ -25,6 +25,10 @@
>>     defining a few macros:
>>  	SYSCALL_NAME		syscall name
>>  	SYSCALL_NARGS		number of arguments this call takes
>> +	SYSCALL_ULONG_ARG_1	the first unsigned long argument this
>> +				call takes
>> +	SYSCALL_ULONG_ARG_2	the second unsigned long argument this
>> +				call takes
> 
> “unsigned long int”
> 
>>  	SYSCALL_SYMBOL		primary symbol name
>>  	SYSCALL_NOERRNO		1 to define a no-errno version (see below)
>>  	SYSCALL_ERRVAL		1 to define an error-value version (see below)
>> @@ -44,9 +48,27 @@
>>  /* This indirection is needed so that SYMBOL gets macro-expanded.  */
>>  #define syscall_hidden_def(SYMBOL)		hidden_def (SYMBOL)
>>  
>> -#define T_PSEUDO(SYMBOL, NAME, N)		PSEUDO (SYMBOL, NAME, N)
>> -#define T_PSEUDO_NOERRNO(SYMBOL, NAME, N)	PSEUDO_NOERRNO (SYMBOL, NAME, N)
>> -#define T_PSEUDO_ERRVAL(SYMBOL, NAME, N)	PSEUDO_ERRVAL (SYMBOL, NAME, N)
>> +/* If PSEUDOS_HAVE_4_ARGS is defined, PSEUDO macros have 4 arguments.  */
>> +#ifndef PSEUDOS_HAVE_4_ARGS
>> +# undef SYSCALL_ULONG_ARG_1
>> +# define SYSCALL_ULONG_ARG_1 0
>> +#endif
> 
> PSEUDOS_HAVE_4_ARGS should be PSEUDOS_HAVE_ULONG_INDICES or something
> like that.  And a comment that briefly explains all the macro
> arguments.  (Not sure if T_PSEUDO is documented somewhere else
> already.)
> 
>> diff --git a/sysdeps/unix/syscalls.list b/sysdeps/unix/syscalls.list
>> index 01c4a0e6b1..e63a8b9d23 100644
>> --- a/sysdeps/unix/syscalls.list
>> +++ b/sysdeps/unix/syscalls.list
>> @@ -37,11 +37,11 @@ kill		-	kill		i:ii	__kill		kill
>>  link		-	link		i:ss	__link		link
>>  listen		-	listen		i:ii	__listen	listen
>>  lseek		-	lseek		i:iii	__libc_lseek	__lseek lseek
>> -madvise		-	madvise		i:pii	__madvise	madvise
>> +madvise		-	madvise		i:pUi	__madvise	madvise
>>  mkdir		-	mkdir		i:si	__mkdir		mkdir
>>  mmap		-	mmap		b:aniiii __mmap		mmap
>> -mprotect	-	mprotect	i:aii	__mprotect	mprotect
>> -munmap		-	munmap		i:ai	__munmap	munmap
>> +mprotect	-	mprotect	i:aUi	__mprotect	mprotect
>> +munmap		-	munmap		i:aU	__munmap	munmap
>>  open		-	open		Ci:siv	__libc_open __open open
>>  profil		-	profil		i:piii	__profil	profil
>>  ptrace		-	ptrace		i:iiii	ptrace
> 
> What about read, readlink, write, etc.?
> 
>> diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h
>> index a37d520f86..b8a74ad2c2 100644
>> --- a/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h
>> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h
>> @@ -26,7 +26,25 @@
>>  #undef LO_HI_LONG
>>  #define LO_HI_LONG(val) (val)
>>  
>> -#ifndef __ASSEMBLER__
>> +#ifdef __ASSEMBLER__
>> +# undef ZERO_EXTEND_1
>> +# define ZERO_EXTEND_1 movl %edi, %edi;
>> +# undef ZERO_EXTEND_2
>> +# define ZERO_EXTEND_2 movl %esi, %esi;
>> +# undef ZERO_EXTEND_3
>> +# define ZERO_EXTEND_3 movl %edx, %edx;
>> +# if SYSCALL_ULONG_ARG_1 == 4 || SYSCALL_ULONG_ARG_2 == 4
>> +#  undef DOARGS_4
>> +#  define DOARGS_4 movl %ecx, %r10d;
>> +# else
>> +#  undef ZERO_EXTEND_4
>> +#  define ZERO_EXTEND_4 movl %r10d, %r10d;
>> +# endif
>> +# undef ZERO_EXTEND_5
>> +# define ZERO_EXTEND_5 movl %r8d, %r8d;
>> +# undef ZERO_EXTEND_6
>> +# define ZERO_EXTEND_6 movl %r9d, %r9d;
>> +#else /* !__ASSEMBLER__ */
>>  # undef ARGIFY
>>  /* Enforce zero-extension for pointers and array system call arguments.
>>     For integer types, extend to int64_t (the full register) using a
> 
> The comment should come before the newly added changes, I think.
> 