Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Jim Ingham via lldb-commits]

> On Sep 8, 2017, at 11:45 PM, Zachary Turner  wrote:
> 
> 
> On Fri, Sep 8, 2017 at 8:19 PM Jason Molenda  > wrote:
> Also keep in mind that debug sessions have a tendency to be long lived.  I 
> may be working through a problem for half an hour -- or this may be the one 
> rare instance where a bug reproduces -- and crashing because some bogus piece 
> of debug info that I don't care about is invalid is not acceptable.  I've 
> gotten those bug reports, where someone has spent all day getting to just the 
> point where they have the problem in front of them, only to have the debugger 
> crash, and they are rightfully enraged at the debugger for this.
> 
> An interactive tool like the debugger cannot use a model where asserting is 
> acceptable.  Even if lldb is out-of-process, you're losing all of the work 
> you'd done until then, and complex bugs can involve extraordinarily long 
> running debug sessions.  The fact that llvm/clang can assert out from under 
> us is to the detriment of lldb's quality.
> 
> J
> Fwiw i think we all agree that it shouldn't crash.  What we disagree on is 
> the best way of making it crash less.  It's easy to see why someone would 
> argue that adding an assert increases the incidence of crashes.  I mean on 
> the surface it seems ridiculous to argue otherwise.  
> 
> But otherwise is precisely what we all continue to argue.
> 
> Your position is that by going out of your way to avoid crashes, it will 
> crash less and be more stable.  Pretty intuitive position if I'm being 
> honest.  But I don't agree.  
> 
> I think you are trading short term metrics for long term technical debt
> You fix one crash by adding a null check, without figuring out why null ever 
> got in there in the first place.  
> 
> You get fewer bug reports because instead of crashing, which would hopefully 
> trigger some automatic crash uploader in Xcode that automatically files a 
> rdar, you just never find out about the bug in the first place.
> 
> You introduce even more bugs, because when you go to edit a function, its 
> complexity and branching is through the roof, and you overlook something some 
> corner case.
> 
> You have less test coverage because you introduce another untested branch in 
> the code, reducing the already abysmal code coverage even further.
> 
> The way to reduce crashing is to *increase* the code coverage of your test 
> suite.  That is the solution.  And you don't do that by adding null checks 
> everywhere, you do it by removing them and asserting.
> 
> Yes, it means Xcode might crash.  But you know the bug exists!  How many bugs 
> are out there right now that nobody even knows about because they are hidden 
> by a null pointer check and instead of the user trying to figure out what's 
> going and filing a bug report, they just give up and use gdb instead.  That's 
> one more person you're never going to get back.
> 
> Crashing Xcode is annoying, but it's fixable.  But when your technical debt 
> is going up and to the right, and your test coverage is going down and to the 
> right, that's an existential threat to the project .
> 
> Btw, i still don't understand why asserts cause anything to crash, given that 
> they're supposed to be turned off in a release build

 I was objecting to the use of llvm_report_fatal_error.  That crashes no matter 
what.  And I was suggesting replacing that with an lldb_assert.  So I’m not 
sure we are so much in disagreement about that.

I was also objecting to the approach where instead of figuring out why the 
errors from one call - DoResmume - were not getting handled properly - we add 
ANOTHER earlier check - CanResume - and then we have to deal with what happens 
when there’s a code path that doesn’t get caught by the added check.  That is 
just adding weakness, and a chance that some obscure programmer error gets us 
confused.

I also am skeptical about the "just assert if things aren’t quite right" 
approach, because it removes the responsibility for thinking about how you 
would get to that state and how you would get out of it safely.  But more 
crucially, it exaggerates the importance of subsections of lldb’s 
functionality.  For instance, to the JIT, being asked to handle a relocation 
that it doesn’t support is a fatal error, so it seems okay by the fail soon 
lights to just fatal error.  But to lldb no expression is important enough to 
quit if it doesn’t work, so we would have really very much preferred if the JIT 
had been forced to back out from the error.  Ditto with llvm & clang and 
handling types.  Even if the types from one CU are horked up beyond belief, 
there’s lots of useful debugging you can still do.

In areas like this we generally put in lldb asserts and then ALSO try to handle 
the error as best we can.  If you trigger the assert running the test suite, 
great, you have a way to figure out how not to do that.  Finding the bug in the 
field is getting somebody else to pay for work we s

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Zachary Turner via lldb-commits]

On Sat, Sep 9, 2017 at 11:18 AM Jim Ingham  wrote:

> On Sep 8, 2017, at 11:45 PM, Zachary Turner  wrote:
>
> On Fri, Sep 8, 2017 at 8:19 PM Jason Molenda  wrote:
>
>> Also keep in mind that debug sessions have a tendency to be long lived.
>> I may be working through a problem for half an hour -- or this may be the
>> one rare instance where a bug reproduces -- and crashing because some bogus
>> piece of debug info that I don't care about is invalid is not acceptable.
>> I've gotten those bug reports, where someone has spent all day getting to
>> just the point where they have the problem in front of them, only to have
>> the debugger crash, and they are rightfully enraged at the debugger for
>> this.
>>
>> An interactive tool like the debugger cannot use a model where asserting
>> is acceptable.  Even if lldb is out-of-process, you're losing all of the
>> work you'd done until then, and complex bugs can involve extraordinarily
>> long running debug sessions.  The fact that llvm/clang can assert out from
>> under us is to the detriment of lldb's quality.
>>
>> J
>
> Fwiw i think we all agree that it shouldn't crash.  What we disagree on is
> the best way of making it crash less.  It's easy to see why someone would
> argue that adding an assert increases the incidence of crashes.  I mean on
> the surface it seems ridiculous to argue otherwise.
>
> But otherwise is precisely what we all continue to argue.
>
> Your position is that by going out of your way to avoid crashes, it will
> crash less and be more stable.  Pretty intuitive position if I'm being
> honest.  But I don't agree.
>
> I think you are trading short term metrics for long term technical debt
> You fix one crash by adding a null check, without figuring out why null
> ever got in there in the first place.
>
> You get fewer bug reports because instead of crashing, which would
> hopefully trigger some automatic crash uploader in Xcode that automatically
> files a rdar, you just never find out about the bug in the first place.
>
> You introduce even more bugs, because when you go to edit a function, its
> complexity and branching is through the roof, and you overlook something
> some corner case.
>
> You have less test coverage because you introduce another untested branch
> in the code, reducing the already abysmal code coverage even further.
>
> The way to reduce crashing is to *increase* the code coverage of your test
> suite.  That is the solution.  And you don't do that by adding null checks
> everywhere, you do it by removing them and asserting.
>
> Yes, it means Xcode might crash.  But you know the bug exists!  How many
> bugs are out there right now that nobody even knows about because they are
> hidden by a null pointer check and instead of the user trying to figure out
> what's going and filing a bug report, they just give up and use gdb
> instead.  That's one more person you're never going to get back.
>
> Crashing Xcode is annoying, but it's fixable.  But when your technical
> debt is going up and to the right, and your test coverage is going down and
> to the right, that's an existential threat to the project .
>
> Btw, i still don't understand why asserts cause anything to crash, given
> that they're supposed to be turned off in a release build
>
>
>  I was objecting to the use of llvm_report_fatal_error.  That crashes no
> matter what.  And I was suggesting replacing that with an lldb_assert.  So
> I’m not sure we are so much in disagreement about that.
>
You're right that crashes no matter what, but when an lldb_assert fires,
you only find out about it if someone is kind enough to submit a bug
report.  I'm assuming that if it were to crash you would find out about it
with an automatically filed rdar with an attached core file.  (I might be
wrong about this)


>
> I was also objecting to the approach where instead of figuring out why the
> errors from one call - DoResmume - were not getting handled properly - we
> add ANOTHER earlier check - CanResume - and then we have to deal with what
> happens when there’s a code path that doesn’t get caught by the added
> check.  That is just adding weakness, and a chance that some obscure
> programmer error gets us confused.
>
I agree with you here.


>
> I also am skeptical about the "just assert if things aren’t quite right"
> approach, because it removes the responsibility for thinking about how you
> would get to that state and how you would get out of it safely.  But more
> crucially, it exaggerates the importance of subsections of lldb’s
> functionality.  For instance, to the JIT, being asked to handle a
> relocation that it doesn’t support is a fatal error, so it seems okay by
> the fail soon lights to just fatal error.  But to lldb no expression is
> important enough to quit if it doesn’t work, so we would have really very
> much preferred if the JIT had been forced to back out from the error.
> Ditto with llvm & clang and handling types.  Even if the types from one CU
> are horked up b

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Jim Ingham via lldb-commits]

> On Sep 9, 2017, at 11:36 AM, Zachary Turner  wrote:
> 
> 
> 
> On Sat, Sep 9, 2017 at 11:18 AM Jim Ingham  > wrote:
>> On Sep 8, 2017, at 11:45 PM, Zachary Turner > > wrote:
>> 
> 
>> On Fri, Sep 8, 2017 at 8:19 PM Jason Molenda > > wrote:
>> Also keep in mind that debug sessions have a tendency to be long lived.  I 
>> may be working through a problem for half an hour -- or this may be the one 
>> rare instance where a bug reproduces -- and crashing because some bogus 
>> piece of debug info that I don't care about is invalid is not acceptable.  
>> I've gotten those bug reports, where someone has spent all day getting to 
>> just the point where they have the problem in front of them, only to have 
>> the debugger crash, and they are rightfully enraged at the debugger for this.
>> 
>> An interactive tool like the debugger cannot use a model where asserting is 
>> acceptable.  Even if lldb is out-of-process, you're losing all of the work 
>> you'd done until then, and complex bugs can involve extraordinarily long 
>> running debug sessions.  The fact that llvm/clang can assert out from under 
>> us is to the detriment of lldb's quality.
>> 
>> J
>> Fwiw i think we all agree that it shouldn't crash.  What we disagree on is 
>> the best way of making it crash less.  It's easy to see why someone would 
>> argue that adding an assert increases the incidence of crashes.  I mean on 
>> the surface it seems ridiculous to argue otherwise.  
>> 
>> But otherwise is precisely what we all continue to argue.
>> 
>> Your position is that by going out of your way to avoid crashes, it will 
>> crash less and be more stable.  Pretty intuitive position if I'm being 
>> honest.  But I don't agree.  
>> 
>> I think you are trading short term metrics for long term technical debt
>> You fix one crash by adding a null check, without figuring out why null ever 
>> got in there in the first place.  
>> 
>> You get fewer bug reports because instead of crashing, which would hopefully 
>> trigger some automatic crash uploader in Xcode that automatically files a 
>> rdar, you just never find out about the bug in the first place.
>> 
>> You introduce even more bugs, because when you go to edit a function, its 
>> complexity and branching is through the roof, and you overlook something 
>> some corner case.
>> 
>> You have less test coverage because you introduce another untested branch in 
>> the code, reducing the already abysmal code coverage even further.
>> 
>> The way to reduce crashing is to *increase* the code coverage of your test 
>> suite.  That is the solution.  And you don't do that by adding null checks 
>> everywhere, you do it by removing them and asserting.
>> 
>> Yes, it means Xcode might crash.  But you know the bug exists!  How many 
>> bugs are out there right now that nobody even knows about because they are 
>> hidden by a null pointer check and instead of the user trying to figure out 
>> what's going and filing a bug report, they just give up and use gdb instead. 
>>  That's one more person you're never going to get back.
>> 
>> Crashing Xcode is annoying, but it's fixable.  But when your technical debt 
>> is going up and to the right, and your test coverage is going down and to 
>> the right, that's an existential threat to the project .
>> 
>> Btw, i still don't understand why asserts cause anything to crash, given 
>> that they're supposed to be turned off in a release build
> 
> 
>  I was objecting to the use of llvm_report_fatal_error.  That crashes no 
> matter what.  And I was suggesting replacing that with an lldb_assert.  So 
> I’m not sure we are so much in disagreement about that.
> You're right that crashes no matter what, but when an lldb_assert fires, you 
> only find out about it if someone is kind enough to submit a bug report.  I'm 
> assuming that if it were to crash you would find out about it with an 
> automatically filed rdar with an attached core file.  (I might be wrong about 
> this)

Well, not a core file since those are huge on OS X and in my experience seldom 
add enough value to be worth the extra cost.  But you would also have crashed.

>  
> 
> I was also objecting to the approach where instead of figuring out why the 
> errors from one call - DoResmume - were not getting handled properly - we add 
> ANOTHER earlier check - CanResume - and then we have to deal with what 
> happens when there’s a code path that doesn’t get caught by the added check.  
> That is just adding weakness, and a chance that some obscure programmer error 
> gets us confused.
> I agree with you here.
>  
> 
> I also am skeptical about the "just assert if things aren’t quite right" 
> approach, because it removes the responsibility for thinking about how you 
> would get to that state and how you would get out of it safely.  But more 
> crucially, it exaggerates the importance of subsections of lldb’s 
> functionality.  For 

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Zachary Turner via lldb-commits]

On Sat, Sep 9, 2017 at 12:04 PM Jim Ingham  wrote:

>
> 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.
>
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.

if you have

void *foo(int x) {
  // do some stuff

  assert(x < 0 || foo != nullptr);
}

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:

void *bar(unsigned x) {
  void *ptr = foo(x);
  if (!ptr) {
// log an error
return nullptr;
  }
  return ptr;
}

You just have

void *bar(unsigned x) {
  void *ptr = foo(x);
  assert(x);
  return x;
}

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.

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.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Jim Ingham via lldb-commits]

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.
 
Jim


> On Sep 9, 2017, at 1:31 PM, Zachary Turner  wrote:
> 
> 
> 
> On Sat, Sep 9, 2017 at 12:04 PM Jim Ingham  > wrote:
> 
> 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.  
> 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.
> 
> if you have
> 
> void *foo(int x) {
>   // do some stuff
> 
>   assert(x < 0 || foo != nullptr);
> }
> 
> 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:
> 
> void *bar(unsigned x) {
>   void *ptr = foo(x);
>   if (!ptr) {
> // log an error
> return nullptr;
>   }
>   return ptr;
> }
> 
> You just have
> 
> void *bar(unsigned x) {
>   void *ptr = foo(x);
>   assert(x);
>   return x;
> }
> 
> 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.
> 
> 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.

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Zachary Turner via lldb-commits]

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
On Sat, Sep 9, 2017 at 6:53 PM Jim Ingham  wrote:

> 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.
>
> Jim
>
>
>
> On Sep 9, 2017, at 1:31 PM, Zachary Turner  wrote:
>
>
>
> On Sat, Sep 9, 2017 at 12:04 PM Jim Ingham  wrote:
>
>>
>> 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.
>>
> 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.
>
> if you have
>
> void *foo(int x) {
>   // do some stuff
>
>   assert(x < 0 || foo != nullptr);
> }
>
> 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:
>
> void *bar(unsigned x) {
>   void *ptr = foo(x);
>   if (!ptr) {
> // log an error
> return nullptr;
>   }
>   return ptr;
> }
>
> You just have
>
> void *bar(unsigned x) {
>   void *ptr = foo(x);
>   assert(x);
>   return x;
> }
>
> 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.
>
> 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.
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits