B-Lang-org / bsc

Bluespec Compiler (BSC)
Other
955 stars 146 forks source link

Static check of file descriptor argument to tasks like $fgetc #668

Open c-93 opened 9 months ago

c-93 commented 9 months ago

Hello together,

I would like to report a behavior, which I don't fully understand, yet.

Minimal Code example below:

Variables:

// Keeping the file descriptors in a single register containing a vector works as expected.
Reg#(Vector#(9, File)) file_descriptors_vec <- mkReg(?);
//// But the alternative (a vector with registers) crashes bluesim.
//Vector#(9, Reg#(File)) file_descriptors_vec = ?;

Vector#(9, Reg#(Bool)) openedVec <- replicateM(mkReg(False));    // Which files are opened?
Vector#(9, Reg#(Bool)) readDoneVec <- replicateM(mkReg(False));  // Which files are done?

Open File(s). Here we open just one file and use only the first index of our vector.

rule openFile( !openedVec[0] && !readDoneVec[0] );
    String readFile = "/Path/To/zero.bin"; // Or use `convertIntToFile(0)`;
    let file_descr <- $fopen(readFile, "rb" );
    file_descriptors_vec[0] <= file_descr;
    openedVec[0] <= True;
    $display("Opening file %s", readFile); // <- the last output before the crash
endrule

Read the file from the first index of the Vector.

rule readFile(openedVec[0] && !readDoneVec[0]);
    int content <- $fgetc( file_descriptors_vec[0] );    // <- Crash here. But only when using vector of registers containing the file descriptors.
    $display("Reading file");
    if ( content != -1 ) begin
        Bit#(8) c = truncate( pack(content) );
    end else begin // no more content to read
        $display( "Could not get byte from file");
        readDoneVec[0] <= True;
        $fclose ( file_descriptors_vec[0] );
        $finish(0);
    end
endrule

Output:

Opening file /Path/To/zero.bin
Segmentation fault (core dumped) 

Stackdump from journalctl (project paths shortened):

Module executable.so with build-id 5cc94a1a04278950652f45c4bf82cfc7dae608e8
Module linux-vdso.so.1 with build-id 0a3d21b3e4e0e58b2c571126b7eda0caa1c8aba5
Module libnss_sss.so.2 with build-id 4225a61239bcd336efeb7a1470d48c3a9aa64afd
Metadata for module libnss_sss.so.2 owned by FDO found: {
        "type" : "rpm",
        "name" : "sssd",
        "version" : "2.9.1-1.fc37",
        "architecture" : "x86_64",
        "osCpe" : "cpe:/o:fedoraproject:fedora:37"
}

Module libz.so.1 with build-id d94299d7572e23295ceaf7674110eb6f2689cd91
Metadata for module libz.so.1 owned by FDO found: {
        "type" : "rpm",
        "name" : "zlib",
        "version" : "1.2.12-5.fc37",
        "architecture" : "x86_64",
        "osCpe" : "cpe:/o:fedoraproject:fedora:37"
}

Module libgcc_s.so.1 with build-id 6baf7a1f1451ec21c7a598b1ea165a2d563edb07
Module libstdc++.so.6 with build-id 63d457fa92d2f36950740c13d70c9a1d1786aaf6
Module ld-linux-x86-64.so.2 with build-id 2a137f1a591c7631a2013d7ee69e30d1e098bdf1
Module libc.so.6 with build-id 0dc6d3e329f8bf5e8c1de63c4c9d560fb9953ade
Module libgmp.so.10 with build-id c88399b381c39d7e45a5c8d078e8f34a237cb03b
Metadata for module libgmp.so.10 owned by FDO found: {
        "type" : "rpm",
        "name" : "gmp",
        "version" : "6.2.1-3.fc37",
        "architecture" : "x86_64",
        "osCpe" : "cpe:/o:fedoraproject:fedora:37"
}

Module libtcl8.6.so with build-id 7bba4bd4283da69c8741ef3654eb4d3062f6d906
Metadata for module libtcl8.6.so owned by FDO found: {
        "type" : "rpm",
        "name" : "tcl",
        "version" : "8.6.12-3.fc37",
        "architecture" : "x86_64",
        "osCpe" : "cpe:/o:fedoraproject:fedora:37"
}

Module libyices.so.2.6 with build-id 9b819adadfb98e36b02d09d185ef5308767ece9b
Module libstp.so.1 with build-id 0fcfe6f7aa11917e1b3f51118ab781228207e733
Module libm.so.6 with build-id fe777a29b3563583f764b5a8ae28782dbcd65352
Module bluetcl with build-id b897699a796a2a9bf75843bff3805fcc6c4f0153
Stack trace of thread 3049273:
#0  0x00007fa696f065b1 _Z12dollar_fgetcPKcj (executable.so + 0x3065b1)
#1  0x00007fa696c368f4 _ZN20MOD_mkTestbenchDexie11RL_readFileEv (executable.so + 0x368f4)
#2  0x00007fa696ef0c58 n/a (executable.so + 0x2f0c58)
#3  0x00007fa696efa34d n/a (executable.so + 0x2fa34d)
#4  0x00007fa696efbe31 _ZN10EventQueue7executeEP9tSimState (executable.so + 0x2fbe31)
#5  0x00007fa696efa144 n/a (executable.so + 0x2fa144)
#6  0x00007fa6a613519d start_thread (libc.so.6 + 0x8b19d)
#7  0x00007fa6a61b6c60 __clone3 (libc.so.6 + 0x10cc60)

Stack trace of thread 3049262:
#0  0x00007fa6a6131d86 __futex_abstimed_wait_common (libc.so.6 + 0x87d86)
#1  0x00007fa6a613cf10 __new_sem_wait_slow64.constprop.0 (libc.so.6 + 0x92f10)
#2  0x00007fa696eff401 wait_on_semaphore (executable.so + 0x2ff401)
#3  0x00007fa696ef9ff1 bk_advance (executable.so + 0x2f9ff1)
#4  0x00000000013d9785 n/a (bluetcl + 0xfd9785)
ELF object binary architecture: AMD x86-64
quark17 commented 9 months ago
// But the alternative (a vector with registers) crashes bluesim.
Vector#(9, Reg#(File)) file_descriptors_vec = ?;

The above code doesn't create registers. An interface assigned with ? will ignore writes and return an undetermined value, and so I'd guess that the segfault comes from trying to access the bogus file handle. Use an assignment like this instead:

file_descriptors_vec <- replicateM(mkRegU);

I would avoid using ? when possible, because it can mask problems that BSC would otherwise be able to catch. In particular, I would avoid ever writing this:

Vector#(n,T) vec = ?;

If you declare a vector with no assignment on the same line, and then assign individually to the elements, BSC is smart enough to detect which elements of a vector have been unassigned and will give you an error if you attempt to access them. For example:

Vector#(3,Bool) vec;
vec[0] = True;
vec[1] = False;
$display(fshow(vec)); // This will give an error because vec[3] is not set

But if vec was assigned with ? to start, then you're disabling BSC's ability to check for uninitialized entries, because you've given the vector a value.

If you ever need to explicitly create a vector with uninitialized entires, you can use newVector. I see that BSC currently has a limitation (that should be fixed) that nested Vectors can't be tracked for uninitialized values. So you'd have to do this:

Vector#(n, Vector#(m, Bool)) vec2d = replicate(newVector);

vec2d[0][0] = True; // Currently this will trigger an error if newVector isn't used

Anyway, does replacing ? with replicateM(mkRegU) prevent the segfault?

Maybe the Bluesim implementation of $fgetc can be updated to check the file descriptor argument that's given to it, so that you can get a helpful error instead of a segfault? We could use fcntl(fd, F_GETFD), maybe, although would that extra overhead slow down simulation for users who don't want it? Plus, there's also a risk that the descriptor is open somewhere else, and the check wouldn't detect that the access should still be considered invalid.

Some other notes, if you don't mind me commenting:

I used mkRegU in place of mkReg(?), because then the register doesn't use a reset signal; the mkReg(?) instance will have a reset value, but you're saying that you don't care what that value is.

Also note that you can use a Maybe type to combine the valid bit with the value:

Vector#(9, Reg#(Maybe#(File))) fd_vec <- replicateM(mkReg(tagged Invalid));

rule openFile( fd_vec[0] matches tagged Invalid &&& !readDoneVec[0] );
    ...
    fd_vec[0] <= tagged Valid file_descr;
    ...
endrule

rule readFile( fd_vec[0] matcges tagged Valid .fd &&& !readDoneVec[0] );
    ...
    c <- $fgetc(fd);
    ...
endrule
c-93 commented 9 months ago

Thanks for your fast and detailed explanation! Yes, your suggestion (correctly initializing by using replicateM(mkRegU)) fixes my mistake.
You may close the issue.

I agree, a better error message could be helpful. Could this be implemented as a basic compile-time check, without simulation runtime overhead?

quark17 commented 9 months ago

It would be interesting to see the behavior when generated to Verilog and run through various simulators, to see if they segfault as well, or if they do some kind of check. (It occurs to me that Bluesim could keep track of the file descriptors that it has opened, from calls to $fopen, and could check whether the argument to tasks like $fgetc is one of those values. But a static check as you suggest would be better.)

One simple check that BSC could do is to check whether the expression for a file descriptor argument contains a don't-care value (?). That would work in this case and would be easy to implement.

A more robust check would be to see whether the argument comes ultimately from an $fopen call. However, that requires analysis of the state element(s) that the descriptor is stored in and then read from. That may not be hard for registers, but BSC may have to give up if the value is stored in another module or passed across synthesis boundaries. Of course, it's unfortunate that simulation-only values like the file descriptor need to be stored in a normal register at all; there's been an idea before that maybe BSC should support a new kind of module (mkSimReg or something) that could be used for storing such values, and that wouldn't turn into a register in the generated Verilog (and could be guarded by synthesis_off..on pragmas). If there was something like that, maybe it would make the analysis easier (but I haven't thought through it).

But certainly we could do the don't-care check.