The Motivating Case
On a recent change of mine it was pointed out that asserts like this
(llvm-project/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp at 24060db3a93297ca77abef8feae301af2e9163b5 · llvm/llvm-project · GitHub)
were potentially dangerous.
This assert basically checks that our register information is not corrupted
in such a way that we would read (or in the case of a live process, write)
outside of the buffer we have the register stored in.
If this happened with assert, an asserts build would fail at that point.
However a release build would simply carry on, which would return bogus results
or in the case of a write, make the situation worse.
Initally I proposed a new macro LLDB_ASSERT_OR.
#ifdef NDEBUG
#define LLDB_ASSERT_OR_RETURN(expr, retval) \
if (!(expr)) \
return retval;
#else
#define LLDB_ASSERT_OR_RETURN(expr, retval) assert(expr);
#endif
If we have asserts on, use the assert. If not, check the condition and if it does
not hold, return some value. In the case of ReadRegister, a Status, elsewhere it
might be false, nullptr, etc.
However, some did argue that given that we know state is corrupted, we shouldn’t
continue at all even in release mode.
Which led me to llvm::report_fatal_error(), but then, looking at our guidelines
(Contributing - 🐛 LLDB)
we have:
Assertions. Assertions (from assert.h) should be used liberally to assert internal consistency.
So in some sense, assert for the mentioned cases is fine because register information
isn’t a user input (not usually at least) and should be consistent.
Also:
Fatal errors. Aborting LLDB’s process using llvm::report_fatal_error() or abort() should be avoided at all costs. It’s acceptable to use llvm_unreachable() for actually unreachable code such as the default in an otherwise exhaustive switch statement.
On the general theme of don’t crash the debugger when you could continue and still be
of some use.
I could argue that if you can’t read a register, the debugger is probably not much use
though.
The guidelines for lldbassert say:
New code should not be using lldbassert() and existing uses should be replaced by other means of error handling.
This was added in ⚙ D59911 Don't abort() in lldb_assert and document why. where there was some previous discussion.
Previous Discussion
Some points from there (some editorialisation when abbreviating):
- Pavel is annoyed that he has to write
lldbassertof a condition, then immediately follow
it withifthat same condition. And wonders if lldbassert can be made to combine the two. - Returning error/result types is better if it can be done. Especially with anything
that’s user input. - Having a way to assert in development builds but log a message on release builds is useful.
The example given is a new DWARF item, this way a user will at least get some hint that
their version of lldb doesn’t support this. - Overusing that would cause a lot of noise, Pavel suggests a construct that reports only once to address this.
lldbassertis for things we can recover from.- Adrian points out that even when
lldbassertprints the message, it’s not in a place you
can easily see it if it’s behind an IDE.
My Current Understanding
Half baked pseudocode ahead. I mostly align with the existing docs except that lldbassert is still hazy for me.
Result Types
Use these when parsing user input, opening files, etc.
llvm::Expected<SomeDWARFAttribute> GetSomeDwarfAttribute(...) {
return llvm error "DWARF DIE should have had SomeDwarfAttribute but nothing was found"
Assertions
Used for internal state, things that lldb makes and constructs all on its own so they should
be correct.
void DoThingWithTarget(TargetSP target) {
assert(target && "Oops, this is supposed to be non-null but couldn't be a ref for some reason")
I would put the aforementioned asserts in core file here too. The things they assert are 90% built into lldb, they are checking that lldb’s logic is sound (that 10% is things like variable sized notes).
I suppose one could craft a strange corefile and trip these asserts, but I don’t think that proves much. Said corefile wouldn’t be produced by any standard system.
Logging
Non fatal issues that are easily recovered from and do not result in an unusable debugger.
/* ... Parse some fields from XML register information ... */
if (!some_required_field)
log("Dropping XML entity foo because some_required_field was missing");
/* ... carry on procesing the rest of the XML info ... */
I do this in the register field parsing. Since it is an extra bonus feature it’s fine to drop it.
Soft Assertions
Some situation that should be obvious to a developer using an asserts build but will only
log in release builds. Like the example from Phab about some new DWARF type being added.
default:
lldbassert("Found type unknown type value %d", type);
return {};
Where this new type being present wouldn’t cause everything else to break. For example
if we expected to see things in a very specific order, then we might use an assert
or Expected to raise a “proper” error to the user.
This situation seems like the most vague and case by case to me.
Questions
- Should the asserts like the ones in llvm-project/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp at main · llvm/llvm-project · GitHub remain
assert? What would you replace them with if not? - What is the status of
lldbassertand its eventual replacement? - What would be the goals of said replacement?
I’d like to put a few code/pseudocode examples in the docs eventually, feel free to use some to back up your comments.
I know we are all busy and asserts aren’t the top of anyone’s TODO list but at minimum I can record the conclusions into a tracking issue that can give context to the comments we already have in the documentation.
