Integrate stack walking on Windows

[wizche]

Howdy, I managed to add stack walking for Windows using the library https://github.com/JochenKalmbach/StackWalker Now I was thinking on doing a pull request and add that as API to TinyInst. Something simple like a string GetCallstack() that one can call directly from the exception handler (e.g. OnCrashed()) and even combine with the translate address we discussed in #56 to have proper addresses. Would make sense to have it upstreamed?

[ifratric]

Interesting. What would be the difference of StackWalker vs using StackWalkEx function from Windows, https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-stackwalkex?redirectedfrom=MSDN (In general, I'm trying to keep the number of third-party dependencies in TinyInst small to keep it, well, Tiny :-))

Some additional notes

1. if this is the reason why you were asking for mapping between translated and original code, then having a full map is unnecessary - it's instead sufficient to have the map of just return addresses. TinyInst already collects that information because it's used as a part of WinUnwindGenerator, see https://github.com/googleprojectzero/TinyInst/blob/92ee0cea83c0b671d0e3b5a01002eb9788a503f0/Windows/winunwind.cpp#L654

2. The entire WinUnwindGenerator might be relevant here. If you pass -generate_unwind to TinyInst, it will generate unwinding (stack walking) information for the translated code. Are you already taking advantage of that?

[wizche]

Interesting. What would be the difference of StackWalker vs using StackWalkEx function from Windows, https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-stackwalkex?redirectedfrom=MSDN (In general, I'm trying to keep the number of third-party dependencies in TinyInst small to keep it, well, Tiny :-))

I completely agree with your reasoning, that's what I like about TinyInst the most, its tininess :) In the end the library is just a wrapper around the StackWalk64 (and some other dbghelp APIs). It handles symbol resolutions and some peculiarities of those APIs. I guess that in a way or the other we need the same code to perform the task. I wasn't really thinking at including it as a third-party library (adding the original source to the third_party folder) but to include and adapt the source code (with proper attribution of course) to the TinyInst core. Defining a generic StackWalker interface and a specific windows implementation that contains the same code with different public methods (like string GetCallstack() instead of the void showCallstack()). What do you think?

Some additional notes

1. if this is the reason why you were asking for mapping between translated and original code, then having a full map is unnecessary - it's instead sufficient to have the map of just return addresses. TinyInst already collects that information because it's used as a part of WinUnwindGenerator, see https://github.com/googleprojectzero/TinyInst/blob/92ee0cea83c0b671d0e3b5a01002eb9788a503f0/Windows/winunwind.cpp#L654

No, the need was to get the precise crash location. We could definitely use that information (if unwinding enabled) in the stack walker to have original return addresses without having the full map enabled.

2. The entire WinUnwindGenerator might be relevant here. If you pass -generate_unwind to TinyInst, it will generate unwinding (stack walking) information for the translated code. Are you already taking advantage of that?

Honestly I didn't spent much time on it. I tried once to enable it on a complex target and was getting many exception that I didn't investigate.

[ifratric]

Oh, I see, I thought StackWalker re-implemented the StackWalk64 behavior instead of using it. If we are integrating StackWalker, would it be possible to add it as is as a third_party library, but have a special cmake flag to actually compile it in? (the benefit of this would be to be able to integrate StackWalker patches easier in the future, if needed). Or does the StackWalker code need to be modified in order to be usable from TinyInst?

[wizche]

StackWalker only offer a way to print/show the callstack via its main function BOOL ShowCallstack(...) I guess we should at least add a new public method like

string GetCallstack(...);
// or
list<string> GetCallstack(...);

Eventually we could also extend StackWalk class and overwrite its OnOutput method to construct the callstack string ourself but feels like an hacky solution...

Not sure if its better to submit a PR directly to StackWalker to add a GetCallstack or just adapt its code to fit into TinyInst core.

[ifratric]

I see. I think, to comment further, it would probably be good to have an idea how the code would look like (both on StackWalker and TinyInst side). If you have an in-progress patch, or something that you could share I'd be happy to take a look. You can share the in-progress diffs with me via email [email protected] if you don't think it's ready for pull request yet.