RFR(s): 8186349: [windows] Centralize dbghelp handling code
Ioi Lam
ioi.lam at oracle.com
Wed Aug 23 14:26:34 UTC 2017
Hi Thomas, thanks for addressing my concerns. I can sponsor the change
after you get OK from the other reviewers.
Thanks
- Ioi
On 8/22/17 5:48 AM, Thomas Stüfe wrote:
> Hi all,
>
> please see new Webrev:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8186349-centralize-dbghelp-handling/webrev.01/webrev/
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186349-centralize-dbghelp-handling/webrev.01/webrev/>
>
> I worked in proposed changes by Ioi and Richard. Sorry, because on
> file name changed, I did not do an incremental webrev. Here are the
> changes:
>
> - Renamed DbgHelpLoader to WindowsDbgHelp and renamed the files too.
> - I moved the critical section code to the implementation of
> WindowsDbgHelp. I discarded the general "CritSectLocker" object in
> favour of an object specialized for this case, see the EntryGuard class.
> - I readded the SymSetOptions call I accidentally discarded.
> - In decoder_windows.cpp, I set the value for _can_decode_in_vm to
> true. I plan to remove this flag in one of the next changes (see
> also JDK-8144855)
>
>
> As Richard is no full reviewer, I'll need a second Reviewer and a sponsor.
>
> Thanks!
>
> Kind Regards, Thomas
>
>
> On Tue, Aug 22, 2017 at 2:33 PM, Thomas Stüfe <thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>> wrote:
>
> 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 <mailto: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
> <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
> <mailto: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
> <https://bugs.openjdk.java.net/browse/JDK-8186349>
> Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/
> <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 and
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-August/024286.html
> <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