deps: WASI-Virt with upstreamed walrus changes

[vados-cosmonic]

This branch serves an example of what WASI-Virt would look like with the recently upstreamed changes to walrus.

This branch is likely to change greatly/possibly be dropped in favor of a new branch that could be merged.

[guybedford]

Thanks for doing this, helps a huge amount to ensure we're on the right track with the Walrus updates before the release. Feeling a lot more confident about it seeing this, down to the builder suggestions.

[vados-cosmonic]

Hey @guybedford a bunch of changes in the last few commits here:

1455d27

0616c4f

5f86c12

aa65d3d

2799bdc

Want to pause before I keep going -- there's a bunch more that could be done here (the lists of functions being removed and stuff feel fairly static/certain), but the diff of this PR is growing so going to curtail it to the file moves I've done if you're happy with them (I'm also absolutely happy with moving the virt_deny back into one big file!)

[EDIT] To be clear on what I mean by "keep going", there is only replacing uses of stub_imported_func left, so I'm quite close to finished.

I'm definitely open to organizing the modules differently as well -- it feels like we could have something like:

src
├── bin
│   └── wasi-virt.rs
├── data.rs
├── lib.rs
├── stub_preview1.rs
├── deny
│   ├── exports
│               ├── clocks.rs
│               ├── ...
│               └── sockets.rs
│   └── imports
│               ├── http.rs
│               ├── ...
│               └── filesystem.rs
└── walrus_ops.rs

[EDIT2] it seems like there are 3 distinct cases:

• denying (replacing an export w/ unreachable)
• stripping (removing imports)
• stubbing (replacing an import w/ a unreachable)

Without breaking the downstreams we could refactor into a shape that fits this more, but I'm also open with leaving them as is (including of course undoing the breaking out of virt_deny)

[EDIT3] Welp, I didn't pause -- at this point the code should basically be ready for review -- there are some test failures so I'm double checking and debugging those but the overall structure of the PR should not change much from here

[guybedford]

I'm all for the abstracting of the import and export data here, that was something we needed to already better refactor in this project so thank you @vados-cosmonic for taking this one one.

Really nice to see the code deletions :)

The CI errors seem to me to be that the previous stub_imported_func had a last argument which when false would just silently ignore a failure of the get_func_by_name function. And in particular that was set for a lot of the fs and stream functions.

The fix might be to have this boolean as part of the data, or two have two lists, and then handle the error differences appropriately in the loop.

Once the CI is going would be great to get this merged, we already run on git dependencies, so there's no concern about that either (at least until we publish this project).

[vados-cosmonic]

Hey @guybedford thanks for taking the time to review, I felt that some of the changes might be gratuitous/a pain to review!

The CI errors seem to me to be that the previous stub_imported_func had a last argument which when false would just silently ignore a failure of the get_func_by_name function. And in particular that was set for a lot of the fs and stream functions.

The fix might be to have this boolean as part of the data, or two have two lists, and then handle the error differences appropriately in the loop.

Agreed here, what I opted to do was separate into lists of WASI_FS_IMPORTS and WASI_FS_IMPORTS_REQUIRED what do you think of that? It's not as clean as having it as part of the data -- if you'd prefer that I'm happy to change.

Also in the case that you're thinking we can go with having it as part of the data, what do you think about having an enum like this:

enum StubRequirement {
    /// The import/export that is stubbed/stripped must be present *and* must be replaced
    Required,
    /// The import/export that is stubbed/stripped is allowed to be missing
    Optional,
    /// It depends on external factors (use-case specific), in this case whether the filesystem is used or not
    RequiredDependingOnFsUsage,
}

Want to avoid having just a raw true/false in there.

One wrinkle to this scheme was that the FS stubbing actually depended on a variable coming in at runtime (uses_fs), so that's partly why I chose to handle break them out like that. I bet it's the interplay of that variable that's causing the test failure...

I'll look into this today

[vados-cosmonic]

Oh yeah and I'll probably add the module for virt_io similar to virt_deny!

[vados-cosmonic]

Hey @guybedford I tracked down the test failure and it's actually something rather insidious the same function ID is being used in two exports. I don' think the current walrus code deems that as possible!

Here's what I get when I print stuff out:

EXPORT FOR SYNC DATA? Export {
    id: Id {
        idx: 164,
    },
    name: "wasi:filesystem/types#sync-data",
    item: Function(
        Id {
            idx: 275,
        },
    ),
}
EXPORT? Export {
    id: Id {
        idx: 32,
    },
    name: "wasi:filesystem/types#sync",
    item: Function(
        Id {
            idx: 275,
        },
    ),
}

So the reason the code was breaking was that it looks like this:

    /// Delete an exported function by name from this module.
    pub fn delete_func_by_name(&mut self, name: impl AsRef<str>) -> Result<()> {
        let fid = self.get_func_by_name(name.as_ref()).with_context(|| {
            format!("failed to find exported func with name [{}]", name.as_ref())
        })?;
        self.delete(
            self.get_exported_func(fid)
                .with_context(|| format!("failed to find exported func with ID [{fid:?}]"))?
                .id(),
        );
        Ok(())
    }

We get the first function by name just fine -- in particular #sync-data comes first, so gets it gets done first. In particular:

• fid gets set to 275
• get_exported_func(275) evaluates to 32 (:boom: )
• ModuleImports::delete happily deletes the export w/ ID 32

When we come around the second time (for #sync), the first call fails (trying to get fid), because the export is already missing.

The culprit in this case seems to be this piece of code:

    /// Get a reference to a function export given its function id.
    pub fn get_exported_func(&self, f: FunctionId) -> Option<&Export> {
        self.iter().find(|e| match e.item {
            ExportItem::Function(f0) => f0 == f,
            _ => false,
        })
    }

This function seems to have been written at a time when it was impossible (?) for the same function ID to be used in two different exports.

Obviously, changing that function is going to be a big 'ol breaking change, so there are a few options I can think of to fix this:

1. Go back to finding the export by name (in delete_func_by_name we use the name to get the ID, then use get_exported_func)
2. Reorder the exports that we're going through (this works but is obviously a terrible solution ™️ )
3. Two exports of the same function is a bug? (is it?)

I'm going to make a PR to walrus ASAP to fix this, but wanted to bring it up.

[EDIT] PR is up (approach 1 is clearly the best there so went ahead and did that)

[vados-cosmonic]

OK, this is the final face of the refactor, code wise.

There is one more change to be made, which is updating the walrus dep to the main branch revision once rustwasm/walrus#251 merges.

Upstream walrus changes have been merged in & rebased/squashed. This PR should be ready for a final review!

[vados-cosmonic]

Just updated to match the changes made to walrus

[EDIT] Going to rename the commit to match the title of this PR

[guybedford]

Thanks @vados-cosmonic for the persistence in this one!

[vados-cosmonic]

Thanks for all the reviews and patience, @guybedford 🙇 !