[wasi-common] Log string representation of WASI errno at the trace level #760

kubkon · 2020-01-05T22:41:43Z

This commit refactors Error enum, and adds logging of the WASI errno string representation at the trace level. Now, when tracing WASI syscalls, we will be greeted with a nicely formatted errno value after each syscall:

path_open(...)
     | *fd=5
     | errno=ESUCCESS

This commit gets rid of errno_from_nix, errno_from_win and errno_from_host helper fns in favour of direct From implementations for the relevant types such as yanix::Errno and winx::winerror::WinError. errno_from_host is replaced by a trait FromRawOsError.

This PR builds on #758, so please don't merge until the other one is merged first.

I'm not entirely sure whether this is the right direction to take, but I hope this PR will be a start. In particular, I left in the WasiError enum, but I'm wondering whether it is needed at all. Also, I'm wondering whether we should preserve the error backtrace in the form of Error enum, or whether outright converting everything to a type that maps one-to-one with __wasi_errno_t would be sufficient. I guess the debugging process could take a hit then, but would it be major, or could we live with it? Just to clarify, I don't suggest we should answer any of those questions in this PR; I'm simply trying to work out the "best" architecture for error handling in wasi-common.

I like the approach taken in bytecodealliance/wasi crate. @sunfishcode is that the format of strerror function you've had in mind?

alexcrichton · 2020-01-06T18:31:06Z

FWIW this seems like a good step to me! There does still seem to be what's probably a few too many error types in play, but this seems like a step forward regardless.

sunfishcode · 2020-01-07T17:20:08Z

Yes, wasi's strerror, where we get the human-readable strings from the witx, is what was was think we'd eventually want -- though it's more important to get the overall error type situation figured out first.

sunfishcode · 2020-01-07T17:29:32Z

I haven't looked at this in detail yet, but at first look, I'm confused about why the winx case is removed from the Error enum, when the yanix case is still there. It seems like these two should be handled the same way, whichever way we go.

More broadly, the top-level Error crate is something I've been finding confusing in wasi-common. I think it would be clearer if functions using it instead chose whether they're returning a WASI error, or a host error (yanix/winx/std::io::Error), and not both. Does that make sense?

marmistrz

Thanks for this change and cleaning up the old conversion functions. Is it needed to make changes to the snapshot? Two more nitpicks in my comments.

marmistrz · 2020-01-07T17:49:08Z

crates/wasi-common/src/macros.rs

+                    .err()
+                    .unwrap_or(crate::Error::ESUCCESS)
+                    .as_wasi_error();
+                log::trace!("\t | errno={}", ret);


The rest of the codebase is not using \t but . This is indeed much more readable, but should be consistent across the codebase. But it's ok to deal with it in a subsequent PR.

Great catch, thanks! I'll make sure to update it!

Agreed; regardless of the merit of tabs, using spaces would allow this to match the indentation level used for similar messages in wasmtime's logging.

Done in b6a4c4a.

marmistrz · 2020-01-07T17:50:18Z

crates/wasi-common/src/sys/windows/host_impl.rs

+
+impl From<WinError> for Error {
+    fn from(err: WinError) -> Self {
+        // TODO: implement error mapping between Windows and WASI


Is this TODO still to be done?

I left it there as a reminder since we don't cover all Windows error codes. Then again, we've got the vast majority of WASI syscalls mapped into Windows, so I'm happy to remove it. The point in favour of leaving it in would be that we still may encounter unmapped Windows-to-WASI error codes when adding new WASI tests in, etc. But I'm happy to go either way with this one :-)

marmistrz · 2020-01-07T17:55:51Z

More broadly, the top-level Error crate is something I've been finding confusing in wasi-common. I think it would be clearer if functions using it instead chose whether they're returning a WASI error, or a host error (yanix/winx/std::io::Error), and not both. Does that make sense?

That was the whole point of introducing the top-level Error type: to unify the error handling. Sometimes we need to pass the error returned from some host call (e.g. the underlying open), sometimes we need to explicitly return a WASI error. True, we could have separate error types, but this would greatly complicate the error handling in functions which need to do both - they'd need to manually convert instead of being just able to use the try! macro.

sunfishcode · 2020-01-08T23:11:50Z

I think the confusing thing is that I have a hard time remembering when we convert from host error codes to WASI error codes. With the Error enum, we have the potential to it in multiple places. It's not a blocker, it's just something I find a little subtle.

My bigger question for this PR is why this is removing the witx arm from Error and not the yanix arm. Is the intention to remove both?

kubkon · 2020-01-08T23:21:45Z

I think the confusing thing is that I have a hard time remembering when we convert from host error codes to WASI error codes. With the Error enum, we have the potential to it in multiple places. It's not a blocker, it's just something I find a little subtle.

My bigger question for this PR is why this is removing the witx arm from Error and not the yanix arm. Is the intention to remove both?

I think you meant winx and not witx, right? :-) Anyhow, winx crate contains a WinError type which only wraps a raw Windows error code, whereas yanix's Error type wraps three error types:

Errno which itself is a wrapper around errno code
NulError which is a thin wrapper around std::ffi::NulError
TryFromIntError which is a thin wrapper around std::num::TryFromIntError

Now, the reason why this stayed was because I saw more symmetry between winx::winerror::WinError and yanix::errno::Errno and both are used to implement From for WasiError. If we want to remove both arms, I can see two solutions:

in yanix convert NulError and TryFromIntError to an errno code corresponding to such behaviour (e.g., EILSEQ) and toss away the backtrace, or
in wasi-common, implement From for yanix::Error in addition to yanix::errno::Errno

sunfishcode · 2020-01-09T18:50:48Z

Can you give an example of a function where it's particularly useful to return both host errors and WASI errors?

I sometimes find myself wanting to think about "when do we resolve host errors into WASI errors?" and with the Error enum holding either one, it means that we can sometimes resolve them early and sometimes later.

marmistrz · 2020-01-09T19:56:35Z

When deciding to use the two-level error hierarchy, I had mostly debugging in mind. Conversion to the errno's is usually lossy, especially for io::ErrorKind::Other which can keep some payload which is lost while converted to the errno. Even if we eventually return an error code, it may be useful to be able to access the original error while debugging, so we want to keep it around as long as possible.

marmistrz · 2020-01-14T15:21:36Z

I see one good reason to convert to the WASI errno immediately: that std::io::Error doesn't implement Clone, which sometimes is really a pain in the neck. Alternatively we could use https://crates.io/crates/arc-io-error instead of std::io::Error to be able to implement Clone.

kubkon · 2020-01-15T20:12:27Z

Hey guys, sorry I didn't look into this in a while but got preoccupied elsewhere. Anyhow, I'm happy to flatten all errors into one wasi_common::Error which is a direct mapping into WASI errno values. Let me know if that is the consensus, and if so, I'll be happy to update the PR to reflect this :-)

sunfishcode

We can continue to think about error handling; this PR looks good to more forward to me (modulo the tabs ;-)). Hooray for logging the syscall results!

This commit refactors `Error` enum, and adds logging of the WASI errno string representation at the trace level. Now, when tracing WASI syscalls, we will be greeted with a nicely formatted errno value after each syscall: ``` path_open(...) | *fd=5 | errno=ESUCCESS ``` This commit gets rid of `errno_from_nix`, `errno_from_win` and `errno_from_host` helper fns in favour of direct `From` implementations for the relevant types such as `yanix::Errno` and `winx::winerror::WinError`. `errno_from_host` is replaced by a trait `FromRawOsError`.

kubkon · 2020-01-16T20:51:53Z

I guess we're good to go with one so I'm gonna go ahead and merge this to unblock for instance #552.

kubkon added the wasi:impl Issues pertaining to WASI implementation in Wasmtime label Jan 5, 2020

kubkon mentioned this pull request Jan 7, 2020

Trace the syscall return values #767

Closed

marmistrz approved these changes Jan 7, 2020

View reviewed changes

kubkon force-pushed the error_to_string branch 2 times, most recently from 96417d4 to 720d23d Compare January 8, 2020 15:42

sunfishcode reviewed Jan 16, 2020

View reviewed changes

Jakub Konka added 3 commits January 16, 2020 11:00

Back port changes to snapshot0

927a406

Fix indentation in logs

b6a4c4a

kubkon force-pushed the error_to_string branch from 5bbb0fd to b6a4c4a Compare January 16, 2020 10:02

marmistrz mentioned this pull request Jan 16, 2020

Minimal viable implementation of poll_oneoff for Windows #552

Merged

kubkon merged commit e474a9e into bytecodealliance:master Jan 16, 2020

kubkon deleted the error_to_string branch January 16, 2020 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasi-common] Log string representation of WASI errno at the trace level #760

[wasi-common] Log string representation of WASI errno at the trace level #760

kubkon commented Jan 5, 2020 •

edited

Loading

alexcrichton commented Jan 6, 2020

sunfishcode commented Jan 7, 2020

sunfishcode commented Jan 7, 2020

marmistrz left a comment •

edited

Loading

marmistrz Jan 7, 2020

kubkon Jan 15, 2020

sunfishcode Jan 16, 2020

kubkon Jan 16, 2020

marmistrz Jan 7, 2020

kubkon Jan 15, 2020

marmistrz commented Jan 7, 2020

sunfishcode commented Jan 8, 2020

kubkon commented Jan 8, 2020

sunfishcode commented Jan 9, 2020

marmistrz commented Jan 9, 2020

marmistrz commented Jan 14, 2020

kubkon commented Jan 15, 2020

sunfishcode left a comment

kubkon commented Jan 16, 2020

[wasi-common] Log string representation of WASI errno at the trace level #760

[wasi-common] Log string representation of WASI errno at the trace level #760

Conversation

kubkon commented Jan 5, 2020 • edited Loading

alexcrichton commented Jan 6, 2020

sunfishcode commented Jan 7, 2020

sunfishcode commented Jan 7, 2020

marmistrz left a comment • edited Loading

Choose a reason for hiding this comment

marmistrz Jan 7, 2020

Choose a reason for hiding this comment

kubkon Jan 15, 2020

Choose a reason for hiding this comment

sunfishcode Jan 16, 2020

Choose a reason for hiding this comment

kubkon Jan 16, 2020

Choose a reason for hiding this comment

marmistrz Jan 7, 2020

Choose a reason for hiding this comment

kubkon Jan 15, 2020

Choose a reason for hiding this comment

marmistrz commented Jan 7, 2020

sunfishcode commented Jan 8, 2020

kubkon commented Jan 8, 2020

sunfishcode commented Jan 9, 2020

marmistrz commented Jan 9, 2020

marmistrz commented Jan 14, 2020

kubkon commented Jan 15, 2020

sunfishcode left a comment

Choose a reason for hiding this comment

kubkon commented Jan 16, 2020

kubkon commented Jan 5, 2020 •

edited

Loading

marmistrz left a comment •

edited

Loading