RFR(s): 8186349: [windows] Centralize dbghelp handling code
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Thu Aug 24 09:14:16 UTC 2017
Looks perfect now, Reviewed.
Best,
Goetz.
> -----Original Message-----
> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> Sent: Donnerstag, 24. August 2017 10:51
> To: Ioi Lam <ioi.lam at oracle.com>; Reingruber, Richard
> <richard.reingruber at sap.com>; Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com>
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(s): 8186349: [windows] Centralize dbghelp handling code
>
> 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
> <mailto: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/
> <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