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

[Wasm][AOT] Add the ability to the display the native stack frames #49828

Merged
merged 2 commits into from
Mar 21, 2021

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Mar 18, 2021

Another approach for PR #45124

How it will be visible in console:
image

@ghost
Copy link

ghost commented Mar 18, 2021

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

Another approach for PR #45124

How it will be visible in console:
image

Author: thaystg
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@radical
Copy link
Member

radical commented Mar 19, 2021

Just wondering if we should make mono_runtime_printf* use logger functions (at least on wasm?), so they will all end up with traces in the js console?

@thaystg
Copy link
Member Author

thaystg commented Mar 19, 2021

mono_runtime_printf* already prints in the js console.

@radical
Copy link
Member

radical commented Mar 19, 2021

mono_runtime_printf* already prints in the js console.

oh, then why the change to use g_printerr in case of wasm? If we want it to show up as an error, then maybe mono_runtime_printf_err?

@thaystg
Copy link
Member Author

thaystg commented Mar 19, 2021

Because I want to be able to show the callstack when click on the arrow and not only a message.

@radical
Copy link
Member

radical commented Mar 19, 2021

Oh, sorry, I meant, show up in the js console with a trace. I guess, console.error would do that.

And mono_runtime_printf_err doesn't do that? Maybe that needs to be changed? console.error would give you the stack trace with it, right?

@thaystg
Copy link
Member Author

thaystg commented Mar 19, 2021

I didn't test mono_runtime_printf_err, if it work I would have to do the same ifdef but instead of using g_printerr
I would use mono_runtime_printf_err. I can test you you think this is important for this PR. :)

@radical
Copy link
Member

radical commented Mar 19, 2021

I didn't test mono_runtime_printf_err, if it work I would have to do the same ifdef but instead of using g_printerr
I would use mono_runtime_printf_err. I can test you you think this is important for this PR. :)

I'm not insisting on anything! I was just thinking that if mono_runtime_printf_err would be changed to use the logger, thus writing with console.error, and showing up in the console with a stack trace, then all other instances of mono_runtime_printf_err callers would get the same benefit. Just a thought :)

@thaystg
Copy link
Member Author

thaystg commented Mar 19, 2021

@radical I will test right now.

@radical
Copy link
Member

radical commented Mar 19, 2021

... if it work I would have to do the same ifdef but instead of using g_printerr
I would use mono_runtime_printf_err...

Oh, and I meant replacing the current mono_runtime_printf with mono_runtime_printf_err, and no ifdef in this location. Sorry, I missed mentioning that!

@thaystg
Copy link
Member Author

thaystg commented Mar 19, 2021

Then I would change the behavior in the case of non wasm, I'm not sure if we want it...

@radical
Copy link
Member

radical commented Mar 19, 2021

Then I would change the behavior in the case of non wasm, I'm not sure if we want it...

True, and that was my question.. if changing that behavior makes sense. If it doesn't, then this change looks fine. Sorry, for being so confusing 😬

@thaystg
Copy link
Member Author

thaystg commented Mar 19, 2021

Yes, mono_runtime_printf_err, shows the stack trace in js console.
image

@radical
Copy link
Member

radical commented Mar 19, 2021

Ok. So, this message itself is just a debug message, but IIUC, on wasm:

  • mono_runtime_printf - doesn't show this with a trace
  • mono_runtime_printf_err - does

.. but since the message is just meant as debug message, we wouldn't want to use mono_runtime_printf_err.

So, your current change looks good to me! 👍

Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

LGTM. My only question is if there is a reason we want to preserve the existing behavior on other platforms or was this a bit arbitrary to begin with?

cc @lambdageek @CoffeeFlux

@lewing lewing merged commit c07e1cf into dotnet:main Mar 21, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 20, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants