skift-org / skift

🥑 A modern delightful operating system
https://skiftos.org/
GNU Lesser General Public License v3.0
2.33k stars 128 forks source link

Incorrect advice on Reddit to resolve issue with __plug_system_get_time #30

Closed mpetch closed 5 years ago

mpetch commented 5 years ago

You recently asked on reddit about an optimization problem with this code:

timestamp_t __plug_system_get_time(void)
{
    timestamp_t timestamp = 0;
    __syscall(SYS_SYSTEM_GET_TIME, (int)&timestamp, 0,0,0,0);
    return timestamp; // get 0 here
}

The solution given was to mark timestamp as volatile by doing:

volatile timestamp_t timestamp = 0;

This only masks the real problem that is in __syscall. You have this code:

static inline int __syscall(syscall_t syscall, int p1, int p2, int p3, int p4, int p5)
{
    int __ret;
    __asm__ __volatile__("push %%ebx; movl %2,%%ebx; int $0x80; pop %%ebx"
                         : "=a"(__ret)
                         : "0"(syscall), "r"((int)(p1)), "c"((int)(p2)), "d"((int)(p3)), "S"((int)(p4)), "D"((int)(p5)));
    return __ret;
}

You potentially pass addresses via registers. GCC isn't being told that what those registers point to are having their data read and/or written to. Because of that the compiler is free to assume that what memory those registers point at is not being used, only the address in the register itself is being used. The GCC documentation suggests for a generic case like this that you add a "memory" clobber to ensure that all data is realized into memory (and restored afterwards if need be). The solution is to make this change:

static inline int __syscall(syscall_t syscall, int p1, int p2, int p3, int p4, int p5)
{
    int __ret;
    __asm__ __volatile__("push %%ebx; movl %2,%%ebx; int $0x80; pop %%ebx"
                         : "=a"(__ret)
                         : "0"(syscall), "r"((int)(p1)), "c"((int)(p2)), "d"((int)(p3)), "S"((int)(p4)), "D"((int)(p5))
                         : "memory");
    return __ret;
}

There is a comment thread about this under your original Reddit post that provides more information

sleepy-monax commented 5 years ago

The change has been commit to master, tanks @mpetch :+1: