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

Implement showing Exceptions. #629

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Conversation

zherczeg
Copy link
Contributor

Please check that this how this feature should work. Regarding signals, the signal handler only receives a signal number, but no context pointer is passed. I could only access the debugger using a global variable. Shall I create a global variable for the debugger?

Signed-off-by: Zoltan Herczeg [email protected]
@ksh8281
Copy link
Contributor

ksh8281 commented Apr 14, 2020

I am sorry. I cannot get your intention from your PR message.
where do you want to get context?
where is the signal handler?

@zherczeg
Copy link
Contributor Author

There was a request for "Handling errors/segfaults" in #606. This patch catches errors in SandBox::run as requested and displays them on the client side. However there was a second part of the request that it should capture signals as well. I haven't implemented it yet, because I need some help how to do it. We can install signal handlers, but they only receive the signal number, so we cannot access the debugger since we have no context. How could we access the context? Shall we create a global variable?

@clover2123
Copy link
Contributor

It seems that there was a misunderstanding about handling errors.
We only need notifying the users for each exception, not for segfault cases.
This patch looks enough to handle the issue.

@zherczeg
Copy link
Contributor Author

Thank you for clarifying it. Please let me know if you need any changes.

@zherczeg
Copy link
Contributor Author

(Note: the patch limits the exception backtrace to 8 entries, we can extend this if more is needed)

Copy link
Contributor

@ksh8281 ksh8281 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 119 to +123
if (!m_debugger || !m_debugger->enabled()) {
return;
}
m_debugger->sendString(Debugger::ESCARGOT_MESSAGE_PRINT_8BIT, output);
m_debugger->sendType(Debugger::ESCARGOT_MESSAGE_PRINT);
if (m_debugger->enabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for enabled debugger is already done in the above.
So please remove this if statement.

Copy link
Contributor Author

@zherczeg zherczeg Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately during a send the connection can be dropped, so we need to check after each send whether we still have connection.

This could be reworked to return a true if sending is successful, or sending data does nothing if no connection is available. I think we should do it in a separate patch though because it affects other code.

Copy link
Contributor

@clover2123 clover2123 Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO the latter approach in which send checks the connection before actually sending the data looks better. In this approach, we don't need to explicitly check the enabled debugger everytime when sending the data.

String* message = error.toStringWithoutException(state);

debugger->sendType(Debugger::ESCARGOT_MESSAGE_EXCEPTION);
if (debugger->enabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

Copy link
Contributor

@clover2123 clover2123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@clover2123 clover2123 merged commit 161899b into Samsung:master Apr 14, 2020
@zherczeg zherczeg deleted the show_exception branch November 16, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants