jingham added a comment.

In D59911#1446946 <https://reviews.llvm.org/D59911#1446946>, @zturner wrote:

> In D59911#1446942 <https://reviews.llvm.org/D59911#1446942>, @jingham wrote:
>
> > In D59911#1446745 <https://reviews.llvm.org/D59911#1446745>, @davide wrote:
> >
> > > My (maybe unpopolar) opinion on the subject is that "soft assertions" are 
> > > a way to cleanse your conscience of guilt, but they don't work really 
> > > well in practice.
> > >  When I started working on lldb, I was a fairly strong proponent of 
> > > assertions everywhere. My view changed somewhat radically over the course 
> > > of the past 18 months, and I would like to summarize some points here.
> > >
> > > 1. Vendors (some) ship a release or two of debuggers every 6 months. Even 
> > > if you just look at open source, llvm gets released every 6 months and 
> > > distributions downstream package just the release version. Not everybody 
> > > runs gentoo or exherbo or compiles his toolchain from svn. For these 
> > > people the debugger has just to work. Assertions are always compiled out 
> > > in release mode so this is not an issue. I strongly agree that for 
> > > internal interfaces they have to be used as liberally as possible, mostly 
> > > because they catch bugs in development. llvm libraries tend to use 
> > > assertions galore, and developers put them in "just in case", and I think 
> > > this is really not something we should do in lldb.
> > > 2. Errors have to be handled, properly. I don't think returning a default 
> > > constructed object in case something goes bad is better than throwing an 
> > > error. We now have rich/proper error handling in llvm to make sure the 
> > > process blows up if the error is not checked. This is a good thing.
> > > 3. The above points are IMHO the only two needed ways of handling 
> > > invariants/invalid input. Anything else in the middle is just going to 
> > > cause confusion to new and existing developer. Adrian (or anybody else), 
> > > do you have any real example where something wasn't either a user error 
> > > or a strong invariant that couldn't be violated? I personally didn't, 
> > > hence I don't understand the need for "soft" assertions here.
> >
> >
> > Here is a concrete example of the sort of thing I thought lldbassert was 
> > for.   In DWARFExpression::AddressRangeForLocationListEntry if we come 
> > across a LocationList format we don't understand, we do:
> >
> >   default:
> >     // Not supported entry type
> >     lldbassert(false && "Not supported location list type");
> >     return false;
>
>
> IMO, this example should be changed to
>
>   return llvm::make_error<DWARFError>("Unsupported location list type");
>
>
> and let someone higher up handle this.


We may be talking at cross-purposes...

I think it would be fine to convert this FUNCTION to return an llvm::Error that 
enforces the error gets handled properly.  But that's orthogonal to whether we 
do an lldbassert before returning the make_error (or "return false").

The whole point of lldbasserts as I understood it was is to catch problems that 
we can and should always recover from IRL as efficiently as possible in the 
testsuite.

If this function returned an error, and all the code above was doing the right 
thing, then some one in the call stack will deal with the return and finally 
end up producing some user-visible failure (i.e. "can't view variable X").   
However, you will only catch the failure in the testsuite if the place where we 
ran across this bad format happened to be a variable that a test asserted.  
Then we would see a test case failure and would go debug it and figure out what 
we needed to do, and all would be good.

But if we put an lldbassert before the return, then we wouldn't have to rely on 
what exactly the test checked.  As long as we passed through this code with the 
unexpected format we would assert, the test would fail at that point, and we  
would know something went wrong and that we should go fix it.  So the chance 
that the testsuite would catch this problem for us is increased by the 
lldbassert.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59911/new/

https://reviews.llvm.org/D59911



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

Reply via email to