-
-
Notifications
You must be signed in to change notification settings - Fork 122
Add support for modules loaded at runtime #247
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
Conversation
For now, this function is only implemented for Windows when CPPTRACE_GET_SYMBOLS_WITH_DBGHELP is defined. For other configurations, it does nothing. load_symbols_for_file will load symbols from the given file with SymLoadModuleEx. It should be called when a module was loaded at runtime with LoadLibrary. Otherwise, the symbols for frames inside that loaded module will not be resolvable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for taking the time to contribute to cpptrace and put together a thoughtful implementation! I've left some comments below
Thanks for the comments! If we make |
I think that's reasonable, it would simplify implementation a lot. In the header what we'd want is the |
The function now takes an HMODULE handle to the DLL instead of the filename. This should make it more explicit that this is a Windows-only function. It also reduces the (rather minor) risk of passing a different filename into the function that doesn't actually refer to a loaded module. It is still necessary to get the filename with GetModuleFileName, but as long as the module handle is valid, this should rarely be an issue.
Unfortunate, because <windef.h> would have been more lightweight, but apparently expects additional defines to be already set before being included.
Having the HMODULE data type in the public interface requires including <Windows.h>, since there doesn't seem to be a "proper" way of forward declaring HMODULE. However, including <Windows.h> comes with a host of problems. For one, its sheer size. For another, it defines some popular names like `min` as macros, which breaks `std::min` and similar. Passing a filename instead of the module handle seems *slightly* less safe due to the risk of passing a string that doesn't refer to a module, but it's also much less hassle.
I think it should be possible to do load_symbols_for_module without including windows.h (I appreciate your thought about this #include). HMODULE is just a HANDLE which is just a |
Ha! I had the same idea after perusing this page. But the doc is wrong. A static assertion DECLARE_HANDLE(HINSTANCE);
typedef HINSTANCE HMODULE; /* HMODULEs can be used in place of HINSTANCEs */ And struct HINSTANCE__ {
int unused;
}; typedef struct HINSTANCE__* HINSTANCE Now, if you really want to, we could still use the
|
Thanks, I see. That's unfortunate that the doc is wrong and foils the nicer solution here. It's also unfortunate that the filename is still needed, with that I'd say it definitely makes sense to use the string API instead of trying to make a handle API work |
I think this looks good to me, I took the liberty of pushing a few minor changes and changing the base to dev. I might move this out of |
I noticed that cpptrace could not resolve symbols from DLLs that were loaded with
LoadLibrary
after already generating at least one stacktrace. According to the Microsoft documentation for Symbol Handler Initialization, one should call SymLoadModuleEx for this case.I've implemented a new function
cpptrace::experimental::load_symbols_for_file(const std::string& name)
that does this.