[Lldb-commits] [PATCH] D18646: Fix DWO breakage in r264909

[Jim Ingham via lldb-commits]

I don't agree that asserts are good in released code unless you have no way of backing out of the situation you find yourself in.  After all, you are saying to some unlucky user out there that they can't use the debugger on their app and in general there's nothing they can do about it.  Greg's suggestion is for this low-level API to say "I couldn't find this DIE" and then if that's something higher layers can work around - by saying "Yeah I couldn't find that type" then you've allowed the user to continue their debug session instead of stopping them cold.

Not asserting prematurely is particularly important for handling debug information; since we don't control the compiler we need to handle as much junk information as gracefully as possible.

Also, asserts, especially for debug information, don't tend to be very helpful in the field.  You get a crash trace which really doesn't tell you the important stuff - what debug file was this, what DIE was bad, etc...  And given the nature of life, this error is going to occur for a user who can't give you their project to repro the bug and can't reduce it to a smaller test case.  Logs are pretty much all you have to go on.  So an un-annotated assert like this is not a good idea.

So orthogonal to the assert issue, if you find something not copacetic in the debug information, you should log out as much local information as you can regardless of what you are going to do with the error.

Jim


> On Mar 31, 2016, at 9:46 AM, Tamas Berghammer via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> 
> tberghammer added inline comments.
> 
> ================
> Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp:136
> @@ +135,3 @@
> +{
> +    assert(m_base_dwarf_cu->GetOffset() == die_ref.cu_offset);
> +    return DebugInfo()->GetDIEForDIEOffset(die_ref.die_offset);
> ----------------
> clayborg wrote:
>> I would prefer to not use an assertion here, we can't crash if things go south for some reason. Can we change this to something like:
>> 
>> ```
>> lldbassert(m_base_dwarf_cu->GetOffset() == die_ref.cu_offset);
>> if (m_base_dwarf_cu->GetOffset() == die_ref.cu_offset)
>>    return DebugInfo()->GetDIEForDIEOffset(die_ref.die_offset);
>> else
>>    return DWARFDIE();
>> ```
>> 
>> The lldbassert will be in debug builds but not release builds so it can fire during testing, but won't crash a release build. assert() is dangerous as it is us to the builders to ensure DEBUG or NDEBUG is defined and if the assert is left in it can crash your lldb, IDE, or any program directly loading LLDB which isn't acceptable.
> I asked Pavel to add the assert in as if the condition isn't met that means a pretty serious problem and will cause incorrect behavior or crash at a later stage. I think it is better to crash at the location where we detect the error instead of crashing at a random point or displaying incorrect data to the user.
> 
> If you don't want LLDB to take down you full IDE then I think you have 2 option:
> * Compile with assert turned off and hope that we don't crash
> * Move LLDB to a separate process and handle the case from the IDE when LLDB crashes
> 
> We have a lot of assert in different parts of LLDB and in clang/llvm as well and usually they help a lot in finding strange bugs. I would prefer to continue adding more assert to the code so if somebody start miss-using an API we detect it immediately. Even if we remove every assert from LLDB (I strongly against it) clang and llvm will contain a large number of asserts what can take down your IDE the same way.
> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D18646
> 
> 
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits