Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle file descriptors as usize #3

Open
saza-ku opened this issue Dec 29, 2023 · 9 comments
Open

Handle file descriptors as usize #3

saza-ku opened this issue Dec 29, 2023 · 9 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@saza-ku
Copy link
Contributor

saza-ku commented Dec 29, 2023

File descriptors are handled as i32 in WASI functions arguments, like this.

pub export fn fd_close(fd: i32) callconv(.C) WasiError

We pass them around as i32. But indices must be usize in Zig, so we must cast them every time we index the file descriptor table.

    pub fn get(self: *Self, fd: i32) ?*Stream {
        const streams = self.streams.acquire();
        defer self.streams.release();
        const s = &streams.*[@as(usize, @intCast(fd))];
        if (s.* == null) {
            return null;
        }

        return @as(*Stream, @ptrCast(s));
    }

To avoid it, we want to cast fd to usize in WASI functions, and handle it as usize in stream module.

@saza-ku saza-ku added the good first issue Good for newcomers label Dec 29, 2023
@dierbei
Copy link
Contributor

dierbei commented Apr 22, 2024

@saza-ku I'd like to try to fix that.

@saza-ku
Copy link
Contributor Author

saza-ku commented Apr 24, 2024

Sure!

@dierbei
Copy link
Contributor

dierbei commented Apr 25, 2024

@saza-ku I'd like to wait for those two pull requests to merge before making changes.

Eventually they can be tested together.

@saza-ku
Copy link
Contributor Author

saza-ku commented Apr 27, 2024

Now they are merged!

@dierbei
Copy link
Contributor

dierbei commented Apr 30, 2024

@saza-ku I have a problem now.

The previous i32 was 4 bytes, the usize needs 8 bytes to be aligned, something needs to be changed if I switch to usize.

usize, on 64-bit platforms, has an alignment of 8, This was also my first exposure to the concept.

But at the moment I don't know what will happen.

// old code
const fd1 = @as(*usize, @ptrFromInt(4 + linear_memory_offset));

// new code
const fd1 = @as(*usize, @ptrFromInt(8 + linear_memory_offset));

I'll have to keep testing it.

@saza-ku
Copy link
Contributor Author

saza-ku commented Apr 30, 2024

Is this code in the integration tests?
If so we could just rearrange memory address.

Are there any other codes which need some fixes about alignment?

@dierbei
Copy link
Contributor

dierbei commented Apr 30, 2024

@saza-ku Yes, there are many areas that need to be modified.

But now I have a problem, the test functions(testClientSocket、testServerSocket) seem to be running at the same time, because their logs seem to be printing at the same time:

mewz/src/wasi.zig

Lines 644 to 650 in 0c6724f

if (!testClientSocket()) {
return;
}
if (!testServerSocket()) {
return;
}

Is this normal? I don't quite understand this test now.

My thought is that if they are running at the same time, will this cause the data to be overwritten.

@saza-ku
Copy link
Contributor Author

saza-ku commented May 18, 2024

@dierbei Sorry for being late! I had a paper due.

But now I have a problem, the test functions(testClientSocket、testServerSocket) seem to be running at the same time, because their logs seem to be printing at the same time:

Mewz is single thread, so it should be impossible.

You debug it on Codespaces, right? I think #65 would be related. I'll investigate the issue first.

@dierbei
Copy link
Contributor

dierbei commented May 20, 2024

Sorry for being late! I had a paper due.

It's okay, I'm always on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants