[NativeAOT-LLVM] Initial version of the managed stack traces on Browser #2677
Conversation
SingleAccretion
force-pushed
the
Browser-StackTrace-Initial
branch
2 times, most recently
from
9bc7323 to
1ab08e7
Compare
Missing features: 1) Safari/WebKit/JSC support (https://bugs.webkit.org/show_bug.cgi?id=278991). JSC will fall back to using the JS frames. 2) Proper hiding of funclets. 3) Method name display for native methods. We could easily do this if we were ok with relying on the implementation detail of how wasm-ld lays out functions, but that's (obviously) not something we can really do.
SingleAccretion
force-pushed
the
Browser-StackTrace-Initial
branch
from
1ab08e7 to
93016f1
Compare
SingleAccretion
force-pushed
the
Browser-StackTrace-Initial
branch
from
93016f1 to
309b95b
Compare
|
@dotnet/nativeaot-llvm |
maraf
added
the
area-NativeAOT-LLVM
|
FYI @kg |
| @@ -546,21 +546,24 @@ The .NET Foundation licenses this file to you under the MIT license. | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <_WasiComponentImports Include="$(IlcFrameworkNativePath)*.wit" /> | |||
There was a problem hiding this comment.
Is this required for this PR, or just general goodness?
There was a problem hiding this comment.
The latter. I tried changing a few things while chasing down the bug that would end up being references below (dotnet/msbuild#5937).
| @@ -310,6 +313,9 @@ private static void RhpPopUnwoundVirtualFrames() | |||
| [RuntimeExport("RhpHandleUnhandledException")] | |||
| private static void HandleUnhandledException(object exception) | |||
| { | |||
| // Append JS frames to this unhandled exception for better diagnostics. | |||
There was a problem hiding this comment.
What happens in Wasi here, or does it never get here?
There was a problem hiding this comment.
This eventually calls down into the WASI version of AppendStack, which is just a no-op for now.
Edit: in the eventual work that will bring stack traces to WASI, it will be just filtered out with a null IP check, or I'll ifdef it out for browser only. Not sure yet what will look better.
| _pEHTable = (void*)((nuint)pUnwindTable & ~FormatMask); | ||
| _format = (nuint)pUnwindTable & FormatMask; | ||
| _pEHTable = (byte*)pUnwindTable; | ||
| _isLargeFormat = (*(byte*)pUnwindTable & MetadataLargeFormat) != 0; |
There was a problem hiding this comment.
I suppose using _pEHTable would remove a cast?
There was a problem hiding this comment.
Yes, but at the cost of a field load (which is likely to get removed, but I like 'efficiency by construction' in system code).
| // at S_P_CoreLib_System_Diagnostics_StackTrace__InitializeForCurrentThread (wasm-function[12314]:275) | ||
| // at S_P_CoreLib_System_Diagnostics_StackTrace___ctor_0(wasm-function[12724]:118) | ||
| skipFrames += 2; // METHODS_TO_SKIP is a constant so just change here | ||
| // Our callers may pass us values that have overflown. |
There was a problem hiding this comment.
when does it overflow, not sure I have understood how that happens?
There was a problem hiding this comment.
It turns out that it is legal to call new StackTrace(skipFrames: int.MinValue) and such, and there are even tests verifying the behavior of that. Such values are probably the result of overflow.
Also, the initial version of this code just did skipFrames += SystemDiagnosticsStackDepth; (like in StackFrame.NativeAot.Browser.cs) instead of SkipSystemFrames, which would overflow for new StackTrace(skipFrames: int.MaxValue).
(Q: should the stack frame logic should also use SkipSystemFrames? A: probably not, since it would mean that in case of JSC or modified binaries it would start yielding system frames only. Which is not too bad for new StackTrace().ToString(), but makes new StackFrame() basically useless.)
We use the JS stack trace API to get a list of frames, either WASM or JS, and use this data to construct the various managed objects representing frames. The JS stack trace format provides us with the WASM function index, which we also store in the stack trace metadata, as our "IP". The result is full fidelity support for managed stack at no additional size cost.
The main execution cost is that the JS stack trace API only supports snapshotting the whole stack, which forces us to both spend more cycles parsing this trace and allocating more memory for it.
Missing features:
What implementation detail?
Wasm-ld lays out code in the exact same order as specified on the command line for the passed-in object files (important note: only actual object files, not LTO bitcode). So we could surround the ILC-produced files by marker objects that define one function. With that, determining whether a given WASM function index is "managed" becomes as simple as a range check.
Contributes to #2404.