[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]

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)

@


https://reviews.llvm.org/D37651