<div dir="ltr">I think everyone is in agreement that input should be sanitized, and ill-formed input should not trigger an abrupt termination if there's a way to recover (which is not always feasible). <div><br></div><div>The conversation started around fail-fast for inconsistent and unrecoverable state, where I think the math is very clear in this case. Using one the the comments about the long-lasting debugging sessions as example, let's say that we have a bug and the debugger state gets corrupted after 1h of debugging:</div><div>1. debugger crashes immediately: lost 1h of debugging -> angry bug report (or ideally automated bug report)</div><div>2. debugger limps along for a few more hours behaving erratically (not necessarily in an obvious fashion)</div><div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div>a. developer gives up after many hours or debugging without understanding what's going on</div></blockquote><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div>b. developer figures out that the debugger is unreliable</div><div><br></div></blockquote>While it is choosing between two evils, I think #1 is clearly the right choice: #2 results in more lost time and frustration, less chance of getting a bug report and can result in a general impression that the debugger is not to be trusted (which for a developer tool is infinitely worse than an unstable tool)<br></div><div><br></div><div>--------------------------------------</div><div><br></div><div>Going back to the original fix, I think most observations are very pertinent - yes, the result is not ideal. This is because the current logic, changing the state in Process::Resume() before the whole operation completes successfully is ... unfortunate. So here are the options I see:</div><div><br></div><div>1. Fix Process::Resume() so we only change the state late, after everything succeds</div><div>2. Rollback state change if anything fails while resuming</div><div>3. Patch the specific case of "resume is not supported"</div><div>4. Do nothing</div><div><br></div><div>The proposed fix is #3, since it's the less intrusive (ie. doesn't change the core resume logic) patch for a very specific and narrow case. IMO the "right" fix is #1, but it seems a more radical change. #2 seems tempting in terms of code changes, but I find it less compelling overall since rollbacks are generally fragile if the code is not designed with that in mind.</div><div><br></div><div>So, LLDB experts, what do you think? I'd be happy to go with #1 if someone with experience in this area can review the change. Anything else that I missed?</div><div><br></div><div>Thanks!</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Sep 10, 2017 at 1:52 PM, Jim Ingham via lldb-commits <span dir="ltr"><<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On Sep 9, 2017, at 7:38 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:</div><br class="m_-6161821770263706775Apple-interchange-newline"><div>Sure, but reading a core file involves handling user input.  Obviously that has to be sanitized and you can't crash on user input.  I don't think there's ever been any disagreement about that.  On the other hand, if you just type an expression in the debugger and expect an answer back, after clang says the ast is valid, the whole stack should be asserts all the way down, because everything is validated <br></div></blockquote><div><br></div></span><div>Yes, but even that ends up not matching with the facts on the ground, because the debug information for types as used by lldb has to be produced from debug info, and the debug info can be malformed and not infrequently is.  So at some point clang is going to ask for something that it assumes must always be available because if it had read the types in from header files it would have asserted at an early stage but when it is fed types from debug info incrementally - which is the only efficient way the debugger can do it - is going to cause something to look wrong at a point where that “shouldn’t be possible.”  clang's rigid structure means it can’t cope with this and will crash on something the user didn’t themselves get wrong.</div><div><br></div><div>According to your classification, we should really consider debug information “user input” as well.  But that means we’re wiring “user input” into a pretty deep place in clang and you’d probably have to weaken your strict assert model to handle that. And it makes me wonder if there’s anything the debugger consumes that wouldn’t fall into this category?</div><div><br></div><div>For instance, we also let plugins provide threads and their stacks, and we try pretty hard not to crash and burn if these plugins get it a little wrong.  So process state really needs to be treated as “user data” as well.  And since we use SPI’s for most of this info that we are pretty much the only clients of, they can be expected to be flakey as well, again a fact that we should not inflict on users of the debugger if we can help it.</div><div><br></div><div>The answer of trying to do everything dangerous out of process I think will make a  slow and fragile architecture.  And while I agree with the <a href="http://llvm.org" target="_blank">llvm.org</a> decision not to use C++ exceptions because they make reasoning about the program more difficult, replacing that with SIGSEGV as the exception mechanism of choice seems really weak.  You are not the first person to point out that we could use the crash-protected threads, but I’m remain very unenthusiastic about this approach...</div><div><br></div><div>Moreover, there are a bunch of problematic areas in lldb, but I don’t think any of them are problematic for the reasons that you are describing here.  There are areas of weak memory management in the data formatters & some places where we don’t manage locking right in handling process stops & the reconstruction of program state after that, and those cause the vast majority of the infrequent but annoying crashes we see.  I don’t myself believe that putting in this assert everywhere architecture would pay off in reduced crashes to compensate for the brittleness it would introduce.</div><div><br></div><div>Finally, and most crucially, no failure of any expression evaluation and no malformed type is worth taking down a debug session, and it’s our responsibility working on lldb to make sure it doesn't.  If the makes our job a little harder, so be it.  This was Jason’s point earlier, but it is worth reiterating because it makes lldb a very different kind of program from all the other programs in the suite collected under the llvm project.  If the compiler gets into an inconsistent state compiling a source file, it’s fine for it to just exit.  It wasn’t going to do any more useful work anyway.  That is definitely NOT true of lldb, and makes reasonable the notion that general solutions that seem appropriate for the other tools are not appropriate for lldb.</div><div><br></div><div><br></div><div>Jim</div><div><div class="h5"><div><br></div><br><blockquote type="cite"><div><div class="gmail_quote"><div dir="ltr">On Sat, Sep 9, 2017 at 6:53 PM Jim Ingham <<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">I think we are talking at cross purposes.  Seems to me you are saying “If we can assert that the answers to questions I ask must always copacetic, then we can express that in software, and that will make things so much easier".  I’m saying "my experience of the data debuggers have to deal with is such that assuming such assertions will lead you to over-react to such errors.  Instead you should write your code so that if somebody gives you bad data you don’t fall over allowing the people who called you to decide how important the error was.”  Every core file that was written out by OS X for years had a section that was ill-formed.  Asserting when you get an ill-formed object file might seem a good way to ensure that you don’t have to make guesses that might lead you astray.  But the people who need to load core files on OS X are rightly unmoved by such arguments if lldb disappears out from under them when reading in core files.<div> </div><div><div></div></div></div><div style="word-wrap:break-word"><div><div>Jim</div></div></div><div style="word-wrap:break-word"><div><div><br><div><br></div><div><br><div><blockquote type="cite"><div>On Sep 9, 2017, at 1:31 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:</div><br class="m_-6161821770263706775m_-1631942114208579788Apple-interchange-newline"><div><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sat, Sep 9, 2017 at 12:04 PM Jim Ingham <<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>I disagree here.  If you can reasonably unwind from an error you should do so even if you can’t figure out how you could have gotten an answer you didn’t expect.  If an API is returning a pointer to something you should assume it might return nullptr unless the API explicitly states otherwise.  </div></div></blockquote><div>But that's exactly what an assert is.  It's an explicit statement by the API about what should happen.  Which is why by adding them liberally, these assumptions can then be propagated all the way up through many layers of the code, vastly simplifying the codebase.</div><div><br></div><div>if you have</div><div><br></div><div>void *foo(int x) {</div><div>  // do some stuff</div><div><br></div><div>  assert(x < 0 || foo != nullptr);</div><div>}</div><div><br></div><div>Then you're documenting that if x is greater than 0, the caller doesn't need to check the return value for nullptr.  Now instead of this:</div><div><br></div><div>void *bar(unsigned x) {</div><div>  void *ptr = foo(x);</div><div>  if (!ptr) {</div><div>    // log an error</div><div>    return nullptr;</div><div>  }</div><div>  return ptr;</div><div>}</div><div><br></div><div>You just have</div><div><br></div><div>void *bar(unsigned x) {</div><div>  void *ptr = foo(x);</div><div>  assert(x);</div><div>  return x;</div><div>}</div><div><br></div><div>And now the caller of bar doesn't have to check either.  The code has greatly reduced complexity due to the butterfly efflect of propagating these assumptions up.</div><div><br></div><div>This is a simple example but the point is that building assumptions into your API is a good thing, because you can enforce them and it vastly simplifies the code.</div></div></div>
</div></blockquote></div><br></div></div></div></div></blockquote></div>
</div></blockquote></div></div></div><br></div><br>______________________________<wbr>_________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@lists.llvm.org">lldb-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/lldb-commits</a><br>
<br></blockquote></div><br></div>