yhzhang0128 / egos-2000

Envision a future where every student can read all the code of a teaching operating system.
Other
2.2k stars 157 forks source link

Bug due to integer promotion in `mtime_get` #13

Closed red1bluelost closed 1 year ago

red1bluelost commented 1 year ago

Take the function mtime_get

static long long mtime_get() {
    int low, high;
    do {
        high = *(int*)(0x200bff8 + 4);
        low  = *(int*)(0x200bff8);
    }  while ( *(int*)(0x200bff8 + 4) != high );

    return (((long long)high) << 32) | low;
}

If low is a negative number (sign bit 31 is 1), then sign extension will occur on the return statement. C conversion rules state that a small sized integer is promoted to the larger type https://en.cppreference.com/w/c/language/conversion. The problem is demonstrated at this godbolt link: https://gary.godbolt.org/z/jxP4hKGMf.

For loading the individual components of the long long, these should probably be unsigned.

yhzhang0128 commented 1 year ago

Thanks for reporting the issue. Indeed, I guess low, hight and the return value should all be unsigned. I will fix it tomorrow.

BTW, I was referencing this function: https://github.com/sifive/freedom-metal/blob/master/src/drivers/sifive_clic0.c#L420

UPDATE: I tried to fix it in commit 114657d. Let me know if you believe the issue still exists.

red1bluelost commented 1 year ago

Thanks for fixing this in 114657dbcd8e7ed3a0658ba320b827be5e9b1fea :)