[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)
Leonard Mosescu via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Sep 8 18:01:03 PDT 2017
lemo added a comment.
Thanks everyone for the feedback.
I see a common question is why doesn't DoResume() fail-fast itself, or why don't the callers propagate the error. It's a good question, since the knowledge if a subclass can resume or not is almost codified in there. The problem is that the state is changed prior to calling DoResume() - see Process::Resume(), ... m_public_run_lock.TrySetRunning() ..., thus the need for an earlier check.
@sas: Yes, a flip approach is possible and I briefly considered the default CanResume() to return false exactly as you're suggesting. The downside is a more brittle composability - subclasses can't call the base implementation, which is usually a reasonable expectation if one wants to build additional functionality on top of the base one. So I prefer the version I sent out, but I could be persuaded either way.
@jingham: 1. I'm not sure I understand the comment about "over-determining the problem", can you please elaborate? If this is about having two methods instead of just DoResume(), I explained above the rationale. 2. Sorry, but defensive programming is exactly the wrong thing, and the root cause of what I'm fixing here. If the internal state is inconsistent the _only_ valid solution is to fail-fast. I've seen people arguing around this for everything from embedded systems to distributed services and trying to limp along pretending things are not too bad only lead to more pain.
@amccarth: I agree that DoResume() can fail for reasons that CanResume() can't predict, and that's prefectly fine - the error is returned (and hopefully it's propagated correctly). Having DoResume() called for a core or minidump is never ok though, and that's the fail-fast path.
(thanks' for the clang-format tip, I'll do that shortly)
More information about the lldb-commits