Stream: git-wasmtime

Topic: wasmtime / issue #3033 Give accessors to `wasmtime::Trap`...

Wasmtime GitHub notifications bot (Jun 25 2021 at 10:14):

github-actions[bot] commented on issue #3033:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Jun 25 2021 at 15:01):

alexcrichton commented on issue #3033:

Could you describe a bit more the use case for getting the user-generated error message? Is the main goal to print the error without printing the backtrace?

Otherwise for implementing source the main thing to account for is that we want to make sure that error messages aren't double-printed. We print out a full chain of errors and if an error is returned from source then it shouldn't be printed by Display for Trap because that would otherwise mean the trap error message would get printed twice.

Wasmtime GitHub notifications bot (Jun 25 2021 at 15:15):

bnjbvr commented on issue #3033:

Could you describe a bit more the use case for getting the user-generated error message? Is the main goal to print the error without printing the backtrace?

Yes, this is exactly my goal. Actually in our embedding the backtrace is post-processed for display, mainly for function names demangling, but now I'm seeing that Wasmtime does that for us. Our display tries to be minimal and display a bit less information, though. So getting the user-generated error message allows us to customize the error message that we display.

it shouldn't be printed by Display for Trap

It seems that Display for Trap doesn't iterate on an error sequence, but only shows the TrapReason and the backtrace generated from the frames. Am I missing something? In any case, I'm happy to provide an alternative error() accessor that returns the plain error (and keep the source() implementation as it was before this PR), or just to remove this last commit (if we can agree on getting the user message accessor).

Wasmtime GitHub notifications bot (Jun 25 2021 at 15:22):

alexcrichton commented on issue #3033:

One option to fix this is to simply remove the backtrace printing from Trap, but personally I feel like it's the right default (I could be convinced otherwise though).

Another possibility to do this would be to implement something like fn display(&self) -> impl fmt::Display which returns basically what Display does minus the backtrace.

For printing twice, it's not actually Display for Trap that prints the error twice. A Display implementation for an error should print the message for just that error, and higher level libraries which generate reports, such as anyhow::Error, will do the iteration. I think that if you print out anyhow::Error where there's a trap inside (printed using {:?} which is anyhow's way of printing the full error backtrace) then you'll see the error printed twice.

I do think it's fine to implement source , though, it just involves not printing the error itself when we're of that variant. Instead a placeholder like "user-generated error" would be printed instead.

Wasmtime GitHub notifications bot (Jun 25 2021 at 15:56):

bnjbvr commented on issue #3033:

We've discussed this a bit, and turns out that displaying the error reason is sufficient for my use case, while remaining abstract enough. Thanks Alex for the suggestions!

Wasmtime GitHub notifications bot (Jun 28 2021 at 10:14):

bnjbvr commented on issue #3033:

After some thinking and discussion, another reasonable path would be to invert the default:

• have Display only print the oneliner reason,
• and have display_with_backtrace return an impl Display that showed the reason and the backtrace

This would be a (small) breaking change, and I'm not sure how people are using it, so I'm tempted to just keep the same default as it is right now, but could be convinced otherwise.

Wasmtime GitHub notifications bot (Jun 28 2021 at 14:59):

alexcrichton commented on issue #3033:

Indeed I agree that's a possibility! (sorry meant to explain that before but it was brief)

I think the best way to figure this out thought might be to follow the Rust community norms in this regard (since Rust error libraries can often carry backtraces as well). Looking at anyhow it looks like explicit opt-in is required to enable backtraces (via env vars). This means that backtraces wouldn't be captured by default, but you could opt-in on a per-execution basis to doing so.

Would something like that be reasonable for y'all? It would be nice to somewhere have a note: use WASMTIME_BACKTRACE=1 to get a backtrace, but I'm not sure if impl Display for Trap is the best location to put that. I'm not sure where it would go...

Even if we go that route though I feel like we may still want this method so, even when backtraces are enabled, there's a specific way to print just the reason without the backtrace.

Wasmtime GitHub notifications bot (Jun 29 2021 at 12:24):

bnjbvr commented on issue #3033:

This would work fine for our use case; curious to see other people's thoughts!

Wasmtime GitHub notifications bot (Jun 29 2021 at 14:45):

alexcrichton commented on issue #3033:

Would you be up for sketching out what it would look like? Ideally we would actually avoid capturing a backtrace entirely on traps unless WASMTIME_BACKTRACE=1 I think?

Wasmtime GitHub notifications bot (Jun 30 2021 at 00:27):

alexcrichton commented on issue #3033:

Hm actually on second thought after talking with some other folks I'm convinced that printing and capturing the backtrace by default is the right thing to do here. I think one thing we could do, though, is configure at a Store level whether backtraces are captured or not, but otherwise I think we'd want something like this method to print the reason of the trap.

Would you be ok with merging this and we can figure out other details later as-needed?

Wasmtime GitHub notifications bot (Jun 30 2021 at 07:42):

bnjbvr commented on issue #3033:

Yes, sounds good to me!

Last updated: Nov 22 2024 at 16:03 UTC