RFR(s): 8186349: [windows] Centralize dbghelp handling code
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Aug 24 08:50:55 UTC 2017
Hi guys,
thanks for the reviews!
@Ioi: thanks for sponsoring!
New Webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8186349-centralize-dbghelp-handling/webrev.02/webrev/
Delta to last:
http://cr.openjdk.java.net/~stuefe/webrevs/8186349-centralize-dbghelp-handling/webrev.01-to-02/webrev/
Nothing exciting changed. I addressed all of Goetz and Richards concerns
and added a Summary line to the change.
Change built and tested (gtests only) on Windows x86, x64.
Kind Regards, Thomas
On Wed, Aug 23, 2017 at 4:26 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 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/
>
> 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