Patchwork [BZ,#19363] Use INTERNAL_SYSCALL_TIMES for Linux times

login
register
mail settings
Submitter H.J. Lu
Date Dec. 15, 2015, 4:33 a.m.
Message ID <CAMe9rOoERUBDk7zq-dqiP1wSemAEcvkYaT=cnPm=5_3F5Ca=8A@mail.gmail.com>
Download mbox | patch
Permalink /patch/10010/
State New
Headers show

Comments

H.J. Lu - Dec. 15, 2015, 4:33 a.m.
On Mon, Dec 14, 2015 at 7:59 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 14 Dec 2015 19:27, H.J. Lu wrote:
>> +  clock_t ret = INTERNAL_SYSCALL_TIMES(err, buf);
>
> needs a space before the (
>
>> +/* Incline Linux times system calls.  */
>
> "incline" ?  i don't understand what you mean.
>
>> +# define INTERNAL_SYSCALL_TIMES(err, buf)                            \
>
> no space after the #

I will fix them.

>> +  ({                                                                 \
>> +    unsigned long long int resultvar;                                        \
>> +    LOAD_ARGS_1 (buf)                                                        \
>> +    LOAD_REGS_1                                                              \
>> +    asm volatile (                                                   \
>> +    "syscall\n\t"                                                    \
>> +    : "=a" (resultvar)                                                       \
>> +    : "0" (__NR_times) ASM_ARGS_1 : "memory", "cc", "r11", "cx");    \
>
> should the cc/r11/cx be made into a sysdep define ?
> -mike

I don't feel strongly about it.  Glibc folks work on x86-64 system calls
know what they are doing.  But I don't mind the patch like this:
Mike Frysinger - Dec. 15, 2015, 2:58 p.m.
On 14 Dec 2015 20:33, H.J. Lu wrote:
> On Mon, Dec 14, 2015 at 7:59 PM, Mike Frysinger wrote:
> > On 14 Dec 2015 19:27, H.J. Lu wrote:
> >> +  ({                                                                 \
> >> +    unsigned long long int resultvar;                                        \
> >> +    LOAD_ARGS_1 (buf)                                                        \
> >> +    LOAD_REGS_1                                                              \
> >> +    asm volatile (                                                   \
> >> +    "syscall\n\t"                                                    \
> >> +    : "=a" (resultvar)                                                       \
> >> +    : "0" (__NR_times) ASM_ARGS_1 : "memory", "cc", "r11", "cx");    \
> >
> > should the cc/r11/cx be made into a sysdep define ?
> > -mike
> 
> I don't feel strongly about it.  Glibc folks work on x86-64 system calls
> know what they are doing.

that sort of thinking is what leads to desync in code paths (it's not
obvious at all that updates to the common sysdep.h needs to also be
deployed to this specific file), plus gcc changes behavior over time
and refines asm constraints.  i'm sure you can find plenty of these
examples in the diff arches as i recall them going by in the past.

not that i'm strongly saying "make the define", just taking umbrage
to your statement here.
-mike
H.J. Lu - Dec. 15, 2015, 3:50 p.m.
On Tue, Dec 15, 2015 at 6:58 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 14 Dec 2015 20:33, H.J. Lu wrote:
>> On Mon, Dec 14, 2015 at 7:59 PM, Mike Frysinger wrote:
>> > On 14 Dec 2015 19:27, H.J. Lu wrote:
>> >> +  ({                                                                 \
>> >> +    unsigned long long int resultvar;                                        \
>> >> +    LOAD_ARGS_1 (buf)                                                        \
>> >> +    LOAD_REGS_1                                                              \
>> >> +    asm volatile (                                                   \
>> >> +    "syscall\n\t"                                                    \
>> >> +    : "=a" (resultvar)                                                       \
>> >> +    : "0" (__NR_times) ASM_ARGS_1 : "memory", "cc", "r11", "cx");    \
>> >
>> > should the cc/r11/cx be made into a sysdep define ?
>> > -mike
>>
>> I don't feel strongly about it.  Glibc folks work on x86-64 system calls
>> know what they are doing.
>
> that sort of thinking is what leads to desync in code paths (it's not
> obvious at all that updates to the common sysdep.h needs to also be
> deployed to this specific file), plus gcc changes behavior over time
> and refines asm constraints.  i'm sure you can find plenty of these
> examples in the diff arches as i recall them going by in the past.
>
> not that i'm strongly saying "make the define", just taking umbrage
> to your statement here.

It belongs a separate patch:

https://sourceware.org/git/?p=glibc.git;a=patch;h=d4f465df65a5723ede4cf933afee5582312fc603

I can submit it if we agree it is necessary.
Mike Frysinger - Dec. 15, 2015, 7:51 p.m.
On 15 Dec 2015 07:50, H.J. Lu wrote:
> On Tue, Dec 15, 2015 at 6:58 AM, Mike Frysinger wrote:
> > On 14 Dec 2015 20:33, H.J. Lu wrote:
> >> On Mon, Dec 14, 2015 at 7:59 PM, Mike Frysinger wrote:
> >> > On 14 Dec 2015 19:27, H.J. Lu wrote:
> >> >> +  ({                                                                 \
> >> >> +    unsigned long long int resultvar;                                        \
> >> >> +    LOAD_ARGS_1 (buf)                                                        \
> >> >> +    LOAD_REGS_1                                                              \
> >> >> +    asm volatile (                                                   \
> >> >> +    "syscall\n\t"                                                    \
> >> >> +    : "=a" (resultvar)                                                       \
> >> >> +    : "0" (__NR_times) ASM_ARGS_1 : "memory", "cc", "r11", "cx");    \
> >> >
> >> > should the cc/r11/cx be made into a sysdep define ?
> >> > -mike
> >>
> >> I don't feel strongly about it.  Glibc folks work on x86-64 system calls
> >> know what they are doing.
> >
> > that sort of thinking is what leads to desync in code paths (it's not
> > obvious at all that updates to the common sysdep.h needs to also be
> > deployed to this specific file), plus gcc changes behavior over time
> > and refines asm constraints.  i'm sure you can find plenty of these
> > examples in the diff arches as i recall them going by in the past.
> >
> > not that i'm strongly saying "make the define", just taking umbrage
> > to your statement here.
> 
> It belongs a separate patch:
> 
> https://sourceware.org/git/?p=glibc.git;a=patch;h=d4f465df65a5723ede4cf933afee5582312fc603
> 
> I can submit it if we agree it is necessary.

i like it, but i leave it to your discretion ;)
-mike

Patch

diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
index fc132f6..c364d08 100644
--- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
@@ -218,6 +218,9 @@ 
 # undef INTERNAL_SYSCALL_DECL
 # define INTERNAL_SYSCALL_DECL(err) do { } while (0)

+/* Registers clobbered by syscall.  */
+# define REGISTERS_CLOBBERED_BY_SYSCALL "cc", "r11", "cx"
+
 # define INTERNAL_SYSCALL_NCS(name, err, nr, args...) \
   ({      \
     unsigned long int resultvar;      \
@@ -226,7 +229,7 @@ 
     asm volatile (      \
     "syscall\n\t"      \
     : "=a" (resultvar)      \
-    : "0" (name) ASM_ARGS_##nr : "memory", "cc", "r11", "cx");      \
+    : "0" (name) ASM_ARGS_##nr : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);   \
     (long int) resultvar; })
 # undef INTERNAL_SYSCALL
 # define INTERNAL_SYSCALL(name, err, nr, args...) \
@@ -240,7 +243,7 @@ 
     asm volatile (      \
     "syscall\n\t"      \
     : "=a" (resultvar)      \
-    : "0" (name) ASM_ARGS_##nr : "memory", "cc", "r11", "cx");      \
+    : "0" (name) ASM_ARGS_##nr : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);   \
     (long int) resultvar; })
 # undef INTERNAL_SYSCALL_TYPES
 # define INTERNAL_SYSCALL_TYPES(name, err, nr, args...) \