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

[Ioi Lam]

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?

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();
}
     }

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

   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?

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?

Thanks
- Ioi


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 
> <https://bugs.openjdk.java.net/browse/JDK-8186349>
> Webrev: 
> http://cr.openjdk.java.net/~stuefe/webrevs/8186349-centralize-dbghelp-handling/webrev.00/webrev/ 
> <http://cr.openjdk.java.net/%7Estuefe/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 
> <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