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

avm2: Implement Error.getStackTrace #7786

Merged
merged 7 commits into from
Sep 7, 2022
Merged

Conversation

Bale001
Copy link
Member

@Bale001 Bale001 commented Aug 28, 2022

Implements getStackTrace for Error. Also does a few other things:

  1. Made a small fix to build_playerglobal where it generated invalid code for custom instance allocators for classes without a package name.
  2. TObject::to_string & TObject::to_locale_string now take an activation, rather than mutationcontext.

Copy link
Member

@kmeisthax kmeisthax left a comment

Choose a reason for hiding this comment

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

I probably missed it deep in a stack of Discord messages, or am staring at the answer but not getting it, but... is there any particular reason why 'getStackTrace' is only implemented for avm_debug builds?

@Bale001
Copy link
Member Author

Bale001 commented Aug 30, 2022

I probably missed it deep in a stack of Discord messages, or am staring at the answer but not getting it, but... is there any particular reason why 'getStackTrace' is only implemented for avm_debug builds?

There are 2 reasons:

  1. Flash also only implements getStackTrace on debug builds, so this matches flash. See the last line in the docs here.
  2. Not implementing getStackTrace on release builds means we also don't have to take the time to copy the call stack and store it in every Error object. This saves time and memory.

@Herschel
Copy link
Member

Herschel commented Sep 5, 2022

Hmm, is avm_debug the right feature for this? Or should it be cfg(debug_assertions) to be in any debug build? I ask because avm_debug is used more for internal debugging of the VM itself, at least in AVM1. Not sure if it makes a huge difference.

Have you noticed getStackTrace() working in any non-debug FP? The docs say they changed it in 11.4, but when I try in the standalone non-debug FP 32 I still get null. So possibly the docs are incorrect? Or maybe this only worked in the browser plugin. My memory is that it only ever printed a trace in the debug player.

@Bale001
Copy link
Member Author

Bale001 commented Sep 5, 2022

Hmm, is avm_debug the right feature for this?

I'm fine with switching it to use cfg(debug_assertions) instead, that make's sense.

I do think that it could also make sense to use avm_debug, because from my perspective that's our equivalent to a flash debug build (as in, has release optimizations, but also has AVM debug features). Doing this means we technically have 2 types of debug builds: One that means we don't have release optimizations, and another that means we have AVM debug features. It might be easier to just say that any debug build should also have AVM debug features, though. I'm really not sure.

Have you noticed getStackTrace() working in any non-debug FP?

It returned null for me as well, I think its either the docs being wrong, or maybe they just started disabling it again in a later version of flash player

@Aaron1011
Copy link
Member

I'm in favor of keeping this behind avm_debug

Copy link
Member

@Herschel Herschel left a comment

Choose a reason for hiding this comment

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

Thought about this a little more an avm_debug makes sense, and debug_assertions would make the tests fail if they were compiled in release mode. I was a little worried that the AVM1 logging was going to hurt our test speed but it's all disabled by a bool by default anyway.

Thank you!

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.

4 participants