[lldb] Replace debug-only assert in AppleObjCTypeEncodingParser::BuildObjCObjectPointerType with lldbassert

[JDevlieghere]

Summary

This assert is only checked in Debug builds but ignored in all other builds. This replaces this code with lldbassert
which should print a warning to the user in release builds and actually asserts in debug builds.

[teemperor]

• Upload correct diff.

[davide]

Please don't do this. I've seen that assertion triggering. If anything, you might want to make a lldbassert.

[JDevlieghere]

In D76697#1939634, @davide wrote:

Please don't do this. I've seen that assertion triggering. If anything, you might want to make a lldbassert.

Did you investigate why the assertion triggered? Was it because of the forward declaration or was it one of the "other weird cases". To me this change seems perfectly in line with https://lldb.llvm.org/resources/contributing.html

[davide]

I don't recall the details.

Soft assertions. LLDB provides lldb_assert() as a soft alternative to cover the middle ground of situations that indicate a recoverable bug in LLDB. In a Debug configuration lldb_assert() behaves like assert(). In a Release configuration it will print a warning and encourage the user to file a bug report, similar to LLVM’s crash handler, and then return execution. Use these sparingly and only if error handling is not otherwise feasible. Specifically, new code should not be using lldb_assert() and existing uses should be replaced by other means of error handling.

This is a recoverable bug, in my books.

[davide]

Also, this was asserting already -- so it's not introducing any new assert().

[JDevlieghere]

According to the comment there is at least one valid reason for this code path to trigger. An lldb_assert will encourage users to file bugs and I think we have enough real issues that we can do without the additional noise.

[davide]

In D76697#1939731, @JDevlieghere wrote:

According to the comment there is at least one valid reason for this code path to trigger. An lldb_assert will encourage users to file bugs and I think we have enough real issues that we can do without the additional noise.

I do believe users should file bugs if this triggers, yes. In the recent past, I looked at ObjC more than most, so these bugs can land on me.

[teemperor]

• moved to lldbassert

[kwk]

Let's summarize this.

1. In the old and the new version, the program is aborted with an assert() if the configuration is LLDB_CONFIGURATION_DEBUG.
2. In the old version, nothing happens, when the configuration is non-Debug.
3. In the new version, the assert in non-Debug configurations will be turned into a warning and *print a warning and encourage the user to file a bug report* (@JDevlieghere that is the noise you're referring to, right?)

[teemperor]

@kkleine yes, that's correct.

So what should happen with this?

[vsk]

The change looks reasonable to me. @davide?

[davide]

LGTM

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp

if (for_expression && !name.empty()) {
     }
 
     DeclVendor *decl_vendor = m_runtime.GetDeclVendor();
     if (!decl_vendor)
       return clang::QualType();
 
     auto types = decl_vendor->FindTypes(ConstString(name), /*max_matches*/ 1);
 
     // The user can forward-declare something that has no definition.  The runtime
     // doesn't prohibit this at all. This is a rare and very weird case.  We keep
     // this assert in debug builds so we catch other weird cases.
-#ifdef LLDB_CONFIGURATION_DEBUG
-    assert(!types.empty());
-#else
+    lldbassert(!types.empty());
     if (types.empty())
       return ast_ctx.getObjCIdType();
-#endif
 
     return ClangUtil::GetQualType(types.front().GetPointerType());
   } else {
     // We're going to resolve this dynamically anyway, so just smile and wave.
     return ast_ctx.getObjCIdType();
   }
 }