[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you for writing this up. I think there is a lot of confusion around this topic, and a document like this is a good step towards clearing it up.

Not really related to this patch, but since we're talking about lldb_assert... The thing that has always bugged me about this function is that it is really hard to use the way it's supposed to be used. Usually, you have to repeat the condition you are asserting twice. For instance, you often have to write something like this

  lldb_assert(thing_that_should_not_happen);
  // Now I cannot just assume assertion is true, because that may crash. So I still need to handle the situation somehow.
  if(!thing_that_should_not_happen)
    return default_value_or_error_or_something;

At that point I just give up, and don't write the lldb_assert line at all.
Maybe if lldb_assert returned a value (possibly even marked with warn_unused_result), then one could "inline" the assert into the if statement, and things would look less awkward (?)

In D59911#1445392 <https://reviews.llvm.org/D59911#1445392>, @JDevlieghere wrote:

> (https://lldb.llvm.org/new-www/)


Hmm.. cool. I didn't know about this. Is there a timeline for moving this to the main website (and getting rid of the html docs)?



================
Comment at: lldb/docs/resources/source.rst:73-78
+* Invalid input. To deal with invalid input, such as malformed DWARF,
+  missing object files, or otherwise inconsistent debug info, LLVM's
+  error handling types such as `llvm::Expected<T>
+  <http://llvm.org/doxygen/classllvm_1_1Expected.html>`_ should be
+  used. Functions that may fail should return an `llvm::Optional<T>
+  <http://llvm.org/doxygen/classllvm_1_1Optional.html>`_ result.
----------------
This looks like it tries to specify when `Expected<T>` and when `Optional<T>` should be used, but its not really clear to me what the distinction is.

The way I see it, both "Expected" and "Optional" can be used for "invalid input" and "functions that may fail", and the difference is whether there is a need to attach a error message or error code to explain why the failure happened.


================
Comment at: lldb/docs/resources/source.rst:81-82
+* Soft assertions. LLDB provides lldb_assert() as a soft alternative
+  to cover the middle ground of situations that really indicate a bug
+  in LLDB, but could conceivably happen. In a Debug configuration
+  lldb_assert() behaves like assert(). In a Release configuration it
----------------
I am not sure this really explains the difference between lldb_assert and assert. A "failed internal consistency check" is definitely a "bug in lldb", and it can certainly "happen". Perhaps an example would help make things clearer?
I tried making one up, but I realized I couldn't (maybe that's the intention since you say they should be used sparingly?). The one place where I'd consider using lldb_assert is for things that are definitely a "bug", but they are not an "lldb bug", such as a compiler emitting DWARF that is obviously broken. However, this document makes it pretty clear this falls into the "invalid input" case.


================
Comment at: lldb/docs/resources/source.rst:88
+  feasible.
+
+Overall, please keep in mind that the debugger is often used as a last
----------------
I am wondering whether we should also mention logging here. What I often do when there is no reasonable way to bubble an error up the stack or display it to the user is that I write it to the log. In that sense it is a form of error handling. WDYT?


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

https://reviews.llvm.org/D59911