c3lang / c3c

Compiler for the C3 language
https://c3-lang.org
GNU Lesser General Public License v3.0
2.68k stars 163 forks source link

Fix os::native_is_{file,dir} bug. Add tests #1189

Closed unereal closed 2 months ago

unereal commented 4 months ago

The example program in man page for stat(2) uses sb.st_mode & S_IFMT == xyz to check for xyz bits. Maybe this isn't what we want. /dev/log, for example, has multiple bits set:

stat.st_mode & libc::S_IFREG: True stat.st_mode & libc::S_IFMT == libc::S_IFREG: False

lerno commented 4 months ago

I don't understand @unereal. That can't be true unless S_IFMT doesn't contain S_IFREG then?

unereal commented 4 months ago

I too don't understand. This program uses both stat.st_mode & libc::S_IFMT == XYZ and stat.st_mode & XYZ

import std::io;
import libc;

// Build: c3c compile -o ~/bin/c3stat c3stat.c3

fn void c3stat(String file)
{
    Stat stat;
    if (@ok(os::native_stat(&stat, file)))
    {
        io::printn("# stat info");
        io::printfn("stat.st_mode                = %b", stat.st_mode);
        io::printfn("stat.st_mode & libc::S_IFMT = %b", stat.st_mode & libc::S_IFMT);
        io::printn("# stat.st_mode & libc::S_IFMT == XYZ");
        io::printf("file '%s' is ", file);
        switch (stat.st_mode & libc::S_IFMT)
        {
            case libc::S_IFREG:
                io::printn("regular file");
            case libc::S_IFDIR:
                io::printn("directory");
            case libc::S_IFBLK:
                io::printn("block device");
            case libc::S_IFCHR:
                io::printn("character device");
            case libc::S_IFIFO:
                io::printn("fifo");
            case libc::S_IFSOCK:
                io::printn("socket");
            case libc::S_IFLNK:
                io::printn("symbolic link");
            default:
                io::printn("unknown");
        }

        io::printn("# stat.st_mode & XYZ");
        io::printfn("file '%s' is", file);
        if (stat.st_mode & libc::S_IFREG) {
            io::printn("- regular file");
        }
        if (stat.st_mode & libc::S_IFDIR) {
            io::printn("- directory");
        }
        if (stat.st_mode & libc::S_IFBLK) {
            io::printn("- block device");
        }
        if (stat.st_mode & libc::S_IFCHR) {
            io::printn("- character device");
        }
        if (stat.st_mode & libc::S_IFIFO) {
            io::printn("- fifo");
        }
        if (stat.st_mode & libc::S_IFSOCK) {
            io::printn("- socket");
        }
        if (stat.st_mode & libc::S_IFLNK) {
            io::printn("- symbolic link");
        }
    }
    io::printn();
}

fn void main(String[] argv)
{
    io::printn("# libc constants");
    io::printfn("libc::S_IFMT                = %-16b", libc::S_IFMT);
    io::printfn("libc::S_IFREG               = %-16b", libc::S_IFREG);
    io::printfn("libc::S_IFDIR               = %-16b", libc::S_IFDIR);
    io::printfn("libc::S_IFBLK               = %-16b", libc::S_IFBLK);
    io::printfn("libc::S_IFCHR               = %-16b", libc::S_IFCHR);
    io::printfn("libc::S_IFIFO               = %-16b", libc::S_IFIFO);
    io::printfn("libc::S_IFSOCK              = %-16b", libc::S_IFSOCK);
    io::printfn("libc::S_IFLNK               = %-16b", libc::S_IFLNK);
    io::printn();

    assert(libc::S_IFMT == (libc::S_IFREG|libc::S_IFDIR|libc::S_IFBLK|libc::S_IFCHR|libc::S_IFIFO|libc::S_IFSOCK|libc::S_IFLNK));

    foreach (f : argv[1..])
    {
        c3stat(f);
    }
}

Output on debian-x64:

c3stat /dev/log /dev/tty
# libc constants
libc::S_IFMT                = 1111000000000000                
libc::S_IFREG               = 1000000000000000                
libc::S_IFDIR               =  100000000000000                
libc::S_IFBLK               =  110000000000000                
libc::S_IFCHR               =   10000000000000                
libc::S_IFIFO               =    1000000000000                
libc::S_IFSOCK              = 1100000000000000                
libc::S_IFLNK               = 1010000000000000                

# stat info
stat.st_mode                = 1100000110110110
stat.st_mode & libc::S_IFMT = 1100000000000000
# stat.st_mode & libc::S_IFMT == XYZ
file '/dev/log' is socket
# stat.st_mode & XYZ
file '/dev/log' is
- regular file
- directory
- block device
- socket
- symbolic link

# stat info
stat.st_mode                = 10000110110110
stat.st_mode & libc::S_IFMT = 10000000000000
# stat.st_mode & libc::S_IFMT == XYZ
file '/dev/tty' is character device
# stat.st_mode & XYZ
file '/dev/tty' is
- block device
- character device
- symbolic link

Using stat(1), (the correct output I suppose):

stat -c %F /dev/log /dev/tty
symbolic link
character special file
unereal commented 4 months ago

Okay, stat(2) follows symlinks by default. To retrieve information from the link itself, one should use lstat(2) instead.

lerno commented 4 months ago

Well, note that if (stat.st_mode & libc::S_IFBLK) will return true even if (stat.st_mode & libc::S_IFBLK) == libc::S_IFBLK) is false as S_IFBLK is not a mask with a single bit.

unereal commented 4 months ago

It turns out that stat.st_mode & libc::S_IFMT == XYZ in the original patch was correct.

lerno commented 4 months ago

Shouldn't it properly be (stat.st_mode & libc::S_IFBLK) == libc::S_IFBLK)?

unereal commented 4 months ago

Yes, (stat.st_mode & libc::S_IFBLK) == libc::S_IFBLK) should work.

Since libc::S_IFMT contains libc::S_IFBLK, (stat.st_mode & libc::S_IFMT) == libc::S_IFBLK) also works. That's how the S_ISXYZ() macros are defined in both musl and glibc.

Maybe there's something on those macros that I can't immediately see.

https://git.musl-libc.org/cgit/musl/tree/include/sys/stat.h#n50 https://sourceware.org/git/?p=glibc.git;a=blob;f=io/sys/stat.h;h=3b4ba80132d0b70390e318602d165665c56c36ff;hb=HEAD#l123

unereal commented 4 months ago

Yes, (stat.st_mode & libc::S_IFBLK) == libc::S_IFBLK) should work.

Looks like this style doesn't work with file '/dev/log'. It's a symbolic link but '(sb.st_mode & S_IFCHR) == S_IFCHR' passes.

Anyway, there are missing parentheses in the original patch. I'll add a commit shortly.

lerno commented 4 months ago

Why would you need the parenthesis?

unereal commented 4 months ago

I was testing various combinations of S_IFXYZ bits with and without the S_IFMT mask, with and without parenthesis around a & b == c in C code. I would convert it to C3 later tonight. But I won't. By the interactions here and elsewhere It is clear to me that these contributions are unappreciated in here, I''ll leave it to other volunteers carry the torch.

lerno commented 4 months ago

I was testing various combinations of S_IFXYZ bits with and without the S_IFMT mask, with and without parenthesis around a & b == c in C code.

Oh, this is because C3 has different precedence on & so for C, you need (stat.st_mode & libc::S_IFBLK) == libc::S_IFBLK, but in C3 stat.st_mode & libc::S_IFBLK == libc::S_IFBLK) this is fine.

By the interactions here and elsewhere It is clear to me that these contributions are unappreciated in here, I''ll leave it to other volunteers carry the torch.

🤔 What interactions are you referring to?

lerno commented 2 months ago

After checking this in detail I think it's correct. I pulled it into master.