RFR(s): 8186349: [windows] Centralize dbghelp handling code

[Reingruber, Richard]

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?

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) ? ", " : ""));

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?

### windows_decoder.cpp

The following line was deleted without replacement. Are the options not needed?

SymSetOptions(SYMOPT_UNDNAME | SYMOPT_DEFERRED_LOADS | SYMOPT_EXACT_SYMBOLS);


Cheers, Richard.

-----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