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

[Thomas Stüfe]

Hi Ioi,

Thanks for the review! Please find my answers inline.

On Mon, Aug 21, 2017 at 8:30 PM, Ioi Lam <ioi.lam at oracle.com> wrote:

> Hi Thomas,
>
> The changes look good. I have a few style nit picks:
>
> CritSect is currently only allocated statically (once), but I wonder if it
> makes sense, for completeness, to add a destructor that calls
> DeleteCriticalSection, just in case someone may use it dynamically in the
> future?
>

I do not want that, because it would introduce a window of opportunity
where this code would not work anymore during shutdown. C++ objects are
destructed in any order, and errors still may happen at this point. I like
this code to be able to function right to the end and not stumble over a
de-initialized critical section.

But I see your point about introducing a something which looks like a
general purpose class with an own header and then not following the design
through. I removed this header and moved the critical section handling
inside the DbgHelpLoader implementation.


>
> Also, there's repetition of this pattern without explanation:
>
>  131   CritSectLocker lck(&g_cs);
>  132   if (initialize_if_needed()) {
>
> I wonder if it's better to do this, so we can put the comments at a place
> that's specific to DbgHelper (and also remove one level of nested "if"):
>
>     DbgHelperLocker lock;
>     if (lock.is_initialized() && g_pfn_SymGetSearchPath != NULL) {
>        .....
>     }
>
>     // Functions from dbghelp.dll are not threadsafe; calls to them must
> be synchronized.
>     class DbgHelperLocker ..... {
>           ....
>       bool is_initialized() {
>         return initialize_if_needed();
>       }
>     }
>
>

You are right, that is a bit ugly. I did what you suggested. I added a
"EntryGuard" RAII object which takes care of locking and
initializing-on-demand.


> Also, can DbgHelpLoader be renamed to WindowsDbgHelp, since it doesn't do
> just loading the DLL but also handles the invocation?
>

Okay. DbgHelpLoader->WindowsDbgHelp, and
"dbghelp_loader.cpp/hpp"->"windbghelp.cpp/hpp" too. Sorry, that means no
incremental webrev, too much hassle due to the renaming :)


>
>   57 #define FOR_ALL_FUNCTIONS(DO) \
>   58  DO(ImagehlpApiVersion) \
>   59  DO(SymSetOptions) \
>   60  DO(SymInitialize) \
>   61  DO(SymGetSymFromAddr64) \
>   62  DO(UnDecorateSymbolName) \
>   63  DO(SymSetSearchPath) \
>   64  DO(SymGetSearchPath) \
>   65  DO(StackWalk64) \
>   66  DO(SymFunctionTableAccess64) \
>   67  DO(SymGetModuleBase64) \
>   68  DO(MiniDumpWriteDump) \
>   69  DO(SymGetLineFromAddr64)
>
> Are the xxx64 versions of the functions also available on 32-bit windows?
> Have you tested with win32 build?
>
>
Yes and yes.

In fact, in our port, we have a function similar to
os::platform_print_native_stack() - which walks the stack with native APIs
only like StackWalk64 - since many years or so for all CPUs (x86,,x64,
ia64).

There is no technical reason not to implement
os::platform_print_native_stack() for 32bit too. But Oracle seems to prefer
using VMError::print_native_stack() (the generic stack walker using the
Frame class) wherever possible to get mixed callstacks. In our port, we
print callstacks using both methods.



> Maybe add a comment that these functions will not be called on 32-bit
> windows:
>
>   79   pfn_StackWalk64              _pfnStackWalk64;
>   80   pfn_SymFunctionTableAccess64 _pfnSymFunctionTableAccess64;
>   81   pfn_SymGetModuleBase64       _pfnSymGetModuleBase64;
>
> and put an assert in the corresponding wrapper function?
>

No reason to, these functions work fine in 32bit.


>
> Thanks
> - Ioi
>

Thanks!

I'll post an updated webrev once I worked in Richards change requests.

Kind Regards, Thomas


>
>
> On 8/18/17 12:23 AM, Thomas Stüfe wrote:
>
> 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-c
> entralize-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/p
> ipermail/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
>
>
>