RFR(s): 8186349: [windows] Centralize dbghelp handling code
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Aug 22 12:48:57 UTC 2017
Hi all,
please see new Webrev:
http://cr.openjdk.java.net/~stuefe/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>
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> 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-bo
>> unces 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/2
>> 017-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