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

[Thomas Stüfe]

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