2022-03-22 15:20:36

by Willy Tarreau

[permalink] [raw]
Subject: Re: [RFC PATCH v2 3/8] tools/nolibc: i386: Implement syscall with 6 arguments

On Tue, Mar 22, 2022 at 07:02:53PM +0700, Ammar Faizi wrote:
> I propose the
> following macro (this is not so much different with other my_syscall macro),
> expect the 6th argument can be in reg or mem.
>
> The "rm" constraint here gives the opportunity for the compiler to use %ebp
> instead of memory if -fomit-frame-pointer is turned on.
>
> #define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6) \
> ({ \
> long _ret; \
> register long _num asm("eax") = (num); \
> register long _arg1 asm("ebx") = (long)(arg1); \
> register long _arg2 asm("ecx") = (long)(arg2); \
> register long _arg3 asm("edx") = (long)(arg3); \
> register long _arg4 asm("esi") = (long)(arg4); \
> register long _arg5 asm("edi") = (long)(arg5); \
> long _arg6 = (long)(arg6); /* Might be in memory */ \
> \
> asm volatile ( \
> "pushl %[_arg6]\n\t" \
> "pushl %%ebp\n\t" \
> "movl 4(%%esp), %%ebp\n\t" \
> "int $0x80\n\t" \
> "popl %%ebp\n\t" \
> "addl $4,%%esp\n\t" \
> : "=a"(_ret) \
> : "r"(_num), "r"(_arg1), "r"(_arg2), "r"(_arg3), \
> "r"(_arg4),"r"(_arg5), [_arg6]"rm"(_arg6) \
> : "memory", "cc" \
> ); \
> _ret; \
> })
>
> What do you think?

Hmmm indeed that comes back to the existing constructs and is certainly
more in line with the rest of the code (plus it will not be affected by
-O0).

I seem to remember a register allocation issue which kept me away from
implementing it this way on i386 back then, but given that my focus was
not as much on i386 as it was on other platforms, it's likely that I have
not insisted too much and not tried this one which looks like the way to
go to me.

Willy


2022-03-23 02:28:26

by David Laight

[permalink] [raw]
Subject: RE: [RFC PATCH v2 3/8] tools/nolibc: i386: Implement syscall with 6 arguments

From: Willy Tarreau
> Sent: 22 March 2022 12:14
>
> On Tue, Mar 22, 2022 at 07:02:53PM +0700, Ammar Faizi wrote:
> > I propose the
> > following macro (this is not so much different with other my_syscall macro),
> > expect the 6th argument can be in reg or mem.
> >
> > The "rm" constraint here gives the opportunity for the compiler to use %ebp
> > instead of memory if -fomit-frame-pointer is turned on.
> >
> > #define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6) \
> > ({ \
> > long _ret; \
> > register long _num asm("eax") = (num); \
> > register long _arg1 asm("ebx") = (long)(arg1); \
> > register long _arg2 asm("ecx") = (long)(arg2); \
> > register long _arg3 asm("edx") = (long)(arg3); \
> > register long _arg4 asm("esi") = (long)(arg4); \
> > register long _arg5 asm("edi") = (long)(arg5); \
> > long _arg6 = (long)(arg6); /* Might be in memory */ \
> > \
> > asm volatile ( \
> > "pushl %[_arg6]\n\t" \
> > "pushl %%ebp\n\t" \
> > "movl 4(%%esp), %%ebp\n\t" \
> > "int $0x80\n\t" \
> > "popl %%ebp\n\t" \
> > "addl $4,%%esp\n\t" \
> > : "=a"(_ret) \
> > : "r"(_num), "r"(_arg1), "r"(_arg2), "r"(_arg3), \
> > "r"(_arg4),"r"(_arg5), [_arg6]"rm"(_arg6) \
> > : "memory", "cc" \
> > ); \
> > _ret; \
> > })
> >
> > What do you think?
>
> Hmmm indeed that comes back to the existing constructs and is certainly
> more in line with the rest of the code (plus it will not be affected by
> -O0).

I'd add an 'always_inline' to the function.
That will force inline even with -O0.

> I seem to remember a register allocation issue which kept me away from
> implementing it this way on i386 back then, but given that my focus was
> not as much on i386 as it was on other platforms, it's likely that I have
> not insisted too much and not tried this one which looks like the way to
> go to me.

dunno, 'asm' register variables are rather more horrid and
should probably only be used (for asm statements) when there aren't
suitable register constraints.

(I'm sure there is a comment about that in the gcc docs.)

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2022-03-23 11:14:54

by Ammar Faizi

[permalink] [raw]
Subject: Re: [RFC PATCH v2 3/8] tools/nolibc: i386: Implement syscall with 6 arguments

On 3/22/22 8:37 PM, David Laight wrote:
> dunno, 'asm' register variables are rather more horrid and
> should probably only be used (for asm statements) when there aren't
> suitable register constraints.
>
> (I'm sure there is a comment about that in the gcc docs.)

^ Hey David, yes you're right, that is very interesting...

I hit a GCC bug when playing with syscall6() implementation here.

Using register variables for all inputs for syscall6() causing GCC 11.2
stuck in an endless loop with 100% CPU usage. Reproducible with several
versions of GCC.

In GCC 6.3, the syscall6() implementation above yields ICE (Internal
Compiler Error):
```
<source>: In function '__sys_mmap':
<source>:35:1: error: unable to find a register to spill
}
^
<source>:35:1: error: this is the insn:
(insn 14 13 30 2 (set (reg:SI 95 [92])
(mem/c:SI (plus:SI (reg/f:SI 16 argp)
(const_int 28 [0x1c])) [1 offset+0 S4 A32])) <source>:33 86 {*movsi_internal}
(expr_list:REG_DEAD (reg:SI 16 argp)
(nil)))
<source>:35: confused by earlier errors, bailing out
Compiler returned: 1
```
See the full show here: https://godbolt.org/z/dYeKaYWY3

Using the appropriate constraints, it compiles nicely, now it looks
like this:
```
#define my_syscall6(num, arg1, arg2, arg3, arg4, arg5, arg6) \
({ \
long _eax = (long)(num); \
long _arg6 = (long)(arg6); /* Always be in memory */ \
asm volatile ( \
"pushl %[_arg6]\n\t" \
"pushl %%ebp\n\t" \
"movl 4(%%esp), %%ebp\n\t" \
"int $0x80\n\t" \
"popl %%ebp\n\t" \
"addl $4,%%esp\n\t" \
: "+a"(_eax) /* %eax */ \
: "b"(arg1), /* %ebx */ \
"c"(arg2), /* %ecx */ \
"d"(arg3), /* %edx */ \
"S"(arg4), /* %esi */ \
"D"(arg5), /* %edi */ \
[_arg6]"m"(_arg6) /* memory */ \
: "memory", "cc" \
); \
_eax; \
})
```
Link: https://godbolt.org/z/ozGbYWbPY

Will use that in the next patchset version.

--
Ammar Faizi

2022-03-23 13:25:22

by Alviro Iskandar Setiawan

[permalink] [raw]
Subject: Re: [RFC PATCH v2 3/8] tools/nolibc: i386: Implement syscall with 6 arguments

On Tue, Mar 22, 2022 at 8:37 PM David Laight wrote:
> dunno, 'asm' register variables are rather more horrid and
> should probably only be used (for asm statements) when there aren't
> suitable register constraints.
>
> (I'm sure there is a comment about that in the gcc docs.)

I don't find the comment that says so here:
https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html

The current code looks valid to me, but I would still prefer to use
the explicit register constraints instead of always using "r"(var) if
available. No strong reason in denying that, tho. Still looks good.

-- Viro

2022-03-23 19:54:57

by Willy Tarreau

[permalink] [raw]
Subject: Re: [RFC PATCH v2 3/8] tools/nolibc: i386: Implement syscall with 6 arguments

On Wed, Mar 23, 2022 at 01:29:39PM +0700, Ammar Faizi wrote:
> On 3/22/22 8:37 PM, David Laight wrote:
> > dunno, 'asm' register variables are rather more horrid and
> > should probably only be used (for asm statements) when there aren't
> > suitable register constraints.
> >
> > (I'm sure there is a comment about that in the gcc docs.)
>
> ^ Hey David, yes you're right, that is very interesting...
>
> I hit a GCC bug when playing with syscall6() implementation here.
>
> Using register variables for all inputs for syscall6() causing GCC 11.2
> stuck in an endless loop with 100% CPU usage. Reproducible with several
> versions of GCC.
>
> In GCC 6.3, the syscall6() implementation above yields ICE (Internal
> Compiler Error):
> ```
> <source>: In function '__sys_mmap':
> <source>:35:1: error: unable to find a register to spill

Now I'm pretty sure that it was the issue I faced when trying long ago,
I remember this error message before I found it wiser to give up.

Willy

2022-03-23 23:40:26

by David Laight

[permalink] [raw]
Subject: RE: [RFC PATCH v2 3/8] tools/nolibc: i386: Implement syscall with 6 arguments

From: Alviro Iskandar Setiawan
> Sent: 22 March 2022 14:48
>
> On Tue, Mar 22, 2022 at 8:37 PM David Laight wrote:
> > dunno, 'asm' register variables are rather more horrid and
> > should probably only be used (for asm statements) when there aren't
> > suitable register constraints.
> >
> > (I'm sure there is a comment about that in the gcc docs.)
>
> I don't find the comment that says so here:
> https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html

I've probably inferred it from:
"The only supported use for this feature is to specify registers for
input and output operands when calling Extended asm (see Extended Asm).
This may be necessary if the constraints for a particular machine don’t
provide sufficient control to select the desired register."

Here is isn't necessary because the required constraint exist/

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)