RFR(s): 8186349: [windows] Centralize dbghelp handling code
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Aug 22 12:33:08 UTC 2017
Hi Richard,
thank you for the review! Please find my remarks inline.
On Tue, Aug 22, 2017 at 9:44 AM, Reingruber, Richard <
richard.reingruber at sap.com> wrote:
> Hi Thomas,
>
> thanks for the refactoring work!
>
> I had a look at your changes, but please note that I'm not a reviewer.
>
> ### dbghelp_loader.cpp:
>
> Should globalDefinitions.hpp be included?
>
Currently I do not need it, so I'd rather not.
>
> Little inconsistency: opening curly braces of method bodies should be on
> the same line as the end of the parameter list, I guess.
>
> 294: st->print("%s" #functionname, ((num_missing > 0) ? ", " : ""));
>
Are you sure? Sorry, I cannot spot an error here.
>
> Format string is incomplete.
>
> 196 BOOL DbgHelpLoader::stackWalk64(DWORD MachineType,
> 197 HANDLE hProcess,
> 198 HANDLE hThread,
> 199 LPSTACKFRAME64 StackFrame,
> 200 PVOID ContextRecord)
> 201 {
> 202 CritSectLocker lck(&g_cs);
> 203 if (initialize_if_needed()) {
> 204 if (g_pfn_StackWalk64 != NULL) {
> 205 return g_pfn_StackWalk64(MachineType, hProcess, hThread,
> StackFrame,
> 206 ContextRecord,
> 207 NULL, // ReadMemoryRoutine
> 208 g_pfn_SymFunctionTableAccess64, //
> FunctionTableAccessRoutine,
> 209 g_pfn_SymGetModuleBase64, //
> GetModuleBaseRoutine
> 210 NULL // TranslateAddressRoutine
> 211 );
> 212 }
> 213 }
> 214 return FALSE;
> 215 }
>
> Lines 208, 209: is it ok to pass NULL?
>
>
Good question. Documentation says parameters are required. I tested calling
this with NULL for both functions and stack walking worked just fine. I
leave it as it is, because I think at worst we risk StackWalk64 failing,
and at best we get a callstack nevertheless.
> ### windows_decoder.cpp
>
> The following line was deleted without replacement. Are the options not
> needed?
>
> SymSetOptions(SYMOPT_UNDNAME | SYMOPT_DEFERRED_LOADS |
> SYMOPT_EXACT_SYMBOLS);
>
>
Good catch! Will fix.
>
> Cheers, Richard.
>
>
Thanks!
Thomas
> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of Thomas Stüfe
> Sent: Freitag, 18. August 2017 09:24
> To: hotspot-runtime-dev at openjdk.java.net
> Subject: RFR(s): 8186349: [windows] Centralize dbghelp handling code
>
> Dear all,
>
> may I please have a review for this change:
>
> Issue: https://bugs.openjdk.java.net/browse/JDK-8186349
> Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/
> 8186349-centralize-dbghelp-handling/webrev.00/webrev/
>
> This is a part of an ongoing work I do to make error reporting (especially
> callstacks) on Windows more reliable.
>
> At first I did a rather large patch, see: https://bugs.openjdk.java.net/
> browse/JDK-8185712 and
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/
> 2017-August/024286.html
> . But after discussing this patch with Ioi, I saw that this patch is better
> split up into multiple parts for easier reviewing.
>
> So this is the first split up patch.
>
> --
>
> This patch here centralizes handling of the dbghelp.dll (loading the
> library, resolving function pointers and synchronizing access).
>
> Which solves the problem that accesses to functions exported from the
> dbghelp.dll need to be synchronized, as stated by the MSDN. We somehow
> never really cared. I guess it never caused visible trouble, because most
> of the time (not always) the functions are accessed from VMError::report(),
> and chances of parallel access from other non-error-reporting threads are
> slim. Even if it were to crash, secondary error handling would step in and
> write an "Error occurred during error reporting" or "Another thread had an
> error too" message and we would probably just shrug it off.
>
> But as this whole effort is about increasing the chance of useful
> callstacks in hs-err files, I'd like to fix this.
>
> In addition to the fix, I think this is also a nice cleanup and removes
> duplicate code.
>
> Notes:
>
> 1) Robustness: We may or may not find a dbghelp.dll on the target system.
> If we find it, it may be old or new (it is not tightly coupled with the OS,
> may be part of other installation packages, may exist multiple times etc).
> We should handle older versions of the dbghelp dll gracefully and hide all
> that complexity from the caller.
>
> 2) The new DbgHelpLoader class does not export any state indicating whether
> or not it successfully loaded, and if it loaded which functions are
> actually available. That was a deliberate decision, there is no need for
> the caller to know this. Caller should invoke the DbgHelpLoader functions
> as if they were the equivalent OS functions and handle return errors.
> DbgHelpLoader should never crash or assert; missing functions should behave
> like failing functions.
>
> 3) However, I added a one liner to the hs-err file indicating the state of
> the dbghelp dll - version info, what functions were missing etc. This may
> help understanding weird or missing callstacks.
>
> 4) I removed the implementation for shutdown (WindowsDecoder::shutdown). I
> think there is no valid reason to ever shutdown the decoder. For one, we
> may crash right at the end, and still it would be nice to have callstacks.
> And then, why spend cycles shutting down the decoder when we could just let
> it end with the process?
>
> 5) This code gets used during error reporting. So no VM infrastructure must
> be used to avoid circular crashes and VM initialization dependencies. So,
> to synchronize, this code uses raw windows CriticalSection objects.
>
> --
>
> Next step will be revamping handling of the Symbol APIs. This will involve
> removing the WindowsDecoder class, which introduces other errors and really
> makes no sense if the underlying dbghelp layer does its own
> synchronization.
>
>
> Thanks for reviewing!
>
> Kind Regards, Thomas
>
More information about the hotspot-runtime-dev
mailing list