-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Debugging: Add a description of how to handle exceptions. #11073
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
Debugging: Add a description of how to handle exceptions. #11073
Conversation
b633e2f to
b23e2a4
Compare
sbc100
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding more docs!
| ... | ||
| } | ||
| It's important to notice that this code will work only for thrown statically allocated exceptions. If your code throws other objects, such as strings or dynamically allocated exceptions, the handling code will need to take that into account. For example, in order to handle thrown strings use: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little confusing because in your example it is a string that is thrown and std::string is dynamically allocated.
Perhaps something like: this code will work only for throwing char* pointers. If you throw other types your getExceptionMessage might need to do different things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you were confused then the description is confusing, I'm not gonna argue with that.
The thing is that I didn't give an example in which a C++ string is thrown, so I'd like to understand what's the source of the confusion. Can you please explain what made you think that std::string is thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused by the because getExceptionMessage returns a std::string which is dynamically allocated.
On second reading I think I understand better now. How about something like in this example the thing being thrown in char*. The getExceptionMessage function would need to be different if some other C++ object was thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rephrased the docs. Are they clearer now?
| Printing exception messages | ||
| =========================== | ||
|
|
||
| Exceptions are thrown from WebAssembly using exception pointers, which means that try/catch/finally blocks in JavaScript will only receive a number, which represents a pointer into linear memory. In order to get the exception message, the user will need to create some WASM code which will extract the meaning from the exception, for example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind wrapping these paragraphs at 80 chars? I know that other documentation mostly doesn't do this but I'm trying to move in that direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
| std::string getExceptionMessage(int exceptionPtr) { | ||
| return std::string(reinterpret_cast<std::exception *>(exceptionPtr)->what()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you do this without embind (which is fairly heavyweight construct).
How about:
EMSCRIPTEN_KEEPALIVE
const char* getExceptionMessage(int exceptionPtr) {
 return reinterpret_cast<const char*>(exceptionPtr)->what());
}
Then have JS call UTF8ToString(getExceptionMessage(exception))?
Then you don't need EMSCRIPTEN_BINDINGS either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what sense is embind heavyweight? The doc seems to hint that it's reasonable, performance-wise.
This question isn't relevant to the PR - I'd just like to know, since I use embind throughout my project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me to demonstrate one feature in isolation, when possible. In order to understand this code one has to also know about how embind work. It might lead the reader to think that embind is somehow essential to what is going on here.
IMHO, just returning a const char * would make the code here simpler, with less boilderplate. And there is no need to for the extra --bind link flag.
I think the reason I say heavyweight is because embind a whole bunch of C++ template magic that few people (myself included) have a good understanding of. I don't know how costly it is a runtime (maybe its not), but it is pretty complex at compile time. I'm not saying its bad or shouldn't be used, I'd just rather not use it when showing how other/simpler things work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel super strongly about not using embind here BTW. This is just my opinion. If you feel strongly that it helps here I'm not going block this PR. I do appreciate contributions to the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried recreating the code in your way, but couldn't get it to run.
I'm sorry - I assume I'm doing something wrong, but I can't really dig into it right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think embind here is ok, if it makes things easier.
I can't seem to get this to work with embind, though. A full testcase might be helpful.
|
round. |
| .. code-block:: javascript | ||
| function getExceptionMessage(exception: any): any { | ||
| return typeof exception === 'number' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But since the exception is a pointer won't it always appear here as a number (i32) , regardless of what one throws? What other type could this be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That depends on the code you wrap. I think that if you use embind you might also receive JS exceptions, and also - if your try clause wraps non-WASM code, you'll need some way to differentiate the errors.
AFAIK, actual exceptions thrown from WASM will always be number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is assuming that no JS code throws numbers as exceptions I guess?
The implicit meaning of this function I guess is if (is_cxx_exception) getExceptionMessage() else ...
|
|
||
| .. code-block:: javascript | ||
| function getExceptionMessage(exception: any): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we use these type annotations elsewhere in the docs do we? I'm typescript or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad, copied the code from my TS repo.
| statically allocated exceptions. If your code throws other objects, such as | ||
| strings or dynamically allocated exceptions, the handling code will need to | ||
| take that into account. For example, if your code throws either exception | ||
| pointers or strings, the javascript code can handle these situations like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if your code throws either exception pointers or strings.. won't throwing strings will also end up calling getExceptionMessage since a C++ strings will look like i32 too. Or do you mean a throw of a JS string from outside of wasm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I meant that if you throw something besides std::exception the example code can't handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might make ti more clear:
For example, if your code needs to handle both native C++ exceptions and JavaScript exceptions you could use the following code to distinguish between them:
I was confused by the or strings part I think because I wasn't sure if you might have meant C++ strings.. but you did mean JS strings right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Since c++ allows you to throw a string, some people use strings as the standard thrown object, and then parse them for meaning in the catch scope.
But your explanation is also good, since I assume that throwing strings will only work from embind.
| } | ||
|
|
||
|
|
||
| Printing exception messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write C++ exception here and on the next line. I think it's important to avoid confusion with JS or wasm exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| std::string getExceptionMessage(int exceptionPtr) { | ||
| return std::string(reinterpret_cast<std::exception *>(exceptionPtr)->what()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think embind here is ok, if it makes things easier.
I can't seem to get this to work with embind, though. A full testcase might be helpful.
| #include <bind.h> | ||
| std::string getExceptionMessage(int exceptionPtr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int could be void*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I try to use void* I get the following error when calling the function:
Cannot call getExceptionMessage due to unbound types: Pv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, interesting, I guess embind doesn't support that. But it should just work without embind.
As mentioned in another comment, I can't seem to get this to work with or without embind, so I must be doing something wrong - if you can provide a full testcase with an example that would be helpful. I can try to convert that to non-embind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted a link, below, for a test case - https://github.com/Shachlan/wasm-exception-example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It embind won't lets use void* here (that is sad) then at least lets use intptr_t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
@kripken @sbc100 I created an example repo, to demonstrate. https://github.com/Shachlan/wasm-exception-example Emscripten's results are already pre-built in the bin folder, but they can be rebuilt using |
5cb2452 to
1492a77
Compare
|
Thanks @Shachlan ! Ok, here is how this could be done without embind: #include <exception>
#include <string>
#include <emscripten.h>
extern "C" {
EMSCRIPTEN_KEEPALIVE
void throwException() {
throw std::runtime_error("Some C++ message text!");
}
EMSCRIPTEN_KEEPALIVE
const char* getExceptionMessage(int exceptionPtr) {
// Save the message so it exists after we exit.
static std::string temp;
temp = reinterpret_cast<std::exception *>(exceptionPtr)->what();
return temp.c_str();
}
} // extern "C"
int main() {
EM_ASM({
try {
Module._throwException();
} catch (exception) {
console.log("A C++ exception was thrown of a pointer", exception, "whose C++ message is:", UTF8ToString(Module._getExceptionMessage(exception)));
}
});
return 0;
}It is a little unfortunate to need to keep the message around like that. What do you think @sbc100, is it better this way or with embind? |
|
Seems reasonable to me. I think the embed version would be OK too assuming we can include a complete and working example like this. I also think this section should perhaps be called "Handling C++ exceptions in JavaScript" since none of this is needed if you just handle the exception in native code right? |
|
@sbc100 I'd be happy to include either my or @kripken's version. |
|
I'm starting to think perhaps we don't need example code at all here? Could we not just include a note that about how C++ exceptions, when caught by JavaScript are represented points to C++ objects in the HEAP? And that its likely that you will need to call back into native code if you want print exception of turn it into a string? What do you think? Do you think including a full example is needed/useful here? |
|
I think that a code sample would make the explanation simpler, since the situation isn't so straight-forward. |
|
OK, we can do ahead with including code. Regarding passing as integer rather than void pointer, that seems very embind specific and nothing to do with exceptions at all. It also seems like maybe a bug in embind that it doesn't work. The natural way to send an address to native code really is a pointer. At the very least it should be an |
| represents a pointer into linear memory. In order to get the exception message, | ||
| the user will need to create some WASM code which will extract the meaning from | ||
| the exception. In the example code below we created a function that receives an | ||
| ``int`` which is pointer to an ``std::exception``, and by casting the pointer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just receives the address of a std::exception should be fine. Sending at as an inptr_t only applies to the embind care, other users can use void* on the native side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| } | ||
|
|
||
|
|
||
| Printing C++ exception messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling C++ exceptions from javascript? (printing isn't the only thing one might want to do rigth?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| #include <bind.h> | ||
| std::string getExceptionMessage(int exceptionPtr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It embind won't lets use void* here (that is sad) then at least lets use intptr_t.
| .. code-block:: javascript | ||
| function getExceptionMessage(exception: any): any { | ||
| return typeof exception === 'number' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is assuming that no JS code throws numbers as exceptions I guess?
The implicit meaning of this function I guess is if (is_cxx_exception) getExceptionMessage() else ...
| statically allocated exceptions. If your code throws other objects, such as | ||
| strings or dynamically allocated exceptions, the handling code will need to | ||
| take that into account. For example, if your code throws either exception | ||
| pointers or strings, the javascript code can handle these situations like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might make ti more clear:
For example, if your code needs to handle both native C++ exceptions and JavaScript exceptions you could use the following code to distinguish between them:
I was confused by the or strings part I think because I wasn't sure if you might have meant C++ strings.. but you did mean JS strings right?
1492a77 to
44fa69e
Compare
|
@sbc100, fixed your ocmments. |
44fa69e to
f83b400
Compare
f83b400 to
94fe6b8
Compare
kripken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, lgtm.
No description provided.