[lldb] Define lldbassert based on NDEBUG instead of LLDB_CONFIGURATION_DEBUG

[JDevlieghere]

Summary

Whether assertions are enabled or not is orthogonal to the build type
which could lead to surprising behavior for lldbassert. Previously, when
doing a debug build with assertions disabled, lldbassert would become a
NOOP, rather than printing an error like it does in a release build. By
definining lldbassert in terms of NDEBUG, it behaves like a regular
assert when assertions are enabled, and like a soft assert.

[JDevlieghere]

This patch also uses __FILE_NAME__ (Clang-specific extension that functions similar to FILE but only renders the last path component (the filename) instead of an invocation dependent full path to that file.) when building with clang.

[rupprecht]

+1 to being surprised this is not already the case

Some other places should be updated after this, e.g. lldb/source/Symbol/SymbolFile.cpp also has a use that can be trivially updated:

#if defined(LLDB_CONFIGURATION_DEBUG)
  // We assert that we have to module lock by trying to acquire the lock from a
  // different thread. Note that we must abort if the result is true to
  // guarantee correctness.
  assert(std::async(
             std::launch::async,
             [this] {
               return this->GetModuleMutex().try_lock();
             }).get() == false &&
         "Module is not locked");
#endif

-> just wrap it in lldbassert

In D150639#4344695, @JDevlieghere wrote:

This patch also uses __FILE_NAME__ (Clang-specific extension that functions similar to FILE but only renders the last path component (the filename) instead of an invocation dependent full path to that file.) when building with clang.

Can you put this comment in LLDBAssert.h itself?

[JDevlieghere]

In D150639#4346009, @rupprecht wrote:

+1 to being surprised this is not already the case

Some other places should be updated after this, e.g. lldb/source/Symbol/SymbolFile.cpp also has a use that can be trivially updated:

#if defined(LLDB_CONFIGURATION_DEBUG)
  // We assert that we have to module lock by trying to acquire the lock from a
  // different thread. Note that we must abort if the result is true to
  // guarantee correctness.
  assert(std::async(
             std::launch::async,
             [this] {
               return this->GetModuleMutex().try_lock();
             }).get() == false &&
         "Module is not locked");
#endif

-> just wrap it in lldbassert

This code actually has a comment right above it explaining that it's too expensive to do in release builds, so this isn't a good candidate for lldbassert. But this logic can be improved: it duplicates checking the LLDB_CONFIGURATION_DEBUG in both ASSERT_MODULE_LOCK and AssertModuleLock and should use NDEBUG too. I'll address that in a separate patch.

In D150639#4344695, @JDevlieghere wrote:

This patch also uses __FILE_NAME__ (Clang-specific extension that functions similar to FILE but only renders the last path component (the filename) instead of an invocation dependent full path to that file.) when building with clang.

Can you put this comment in LLDBAssert.h itself?

Good idea.

[rupprecht]

In D150639#4346649, @JDevlieghere wrote:

In D150639#4346009, @rupprecht wrote:

+1 to being surprised this is not already the case

Some other places should be updated after this, e.g. lldb/source/Symbol/SymbolFile.cpp also has a use that can be trivially updated:

#if defined(LLDB_CONFIGURATION_DEBUG)
  // We assert that we have to module lock by trying to acquire the lock from a
  // different thread. Note that we must abort if the result is true to
  // guarantee correctness.
  assert(std::async(
             std::launch::async,
             [this] {
               return this->GetModuleMutex().try_lock();
             }).get() == false &&
         "Module is not locked");
#endif

-> just wrap it in lldbassert

This code actually has a comment right above it explaining that it's too expensive to do in release builds, so this isn't a good candidate for lldbassert. But this logic can be improved: it duplicates checking the LLDB_CONFIGURATION_DEBUG in both ASSERT_MODULE_LOCK and AssertModuleLock and should use NDEBUG too. I'll address that in a separate patch.

WDYT about just replacing LLDB_CONFIGURATION_DEBUG with LLVM's EXPENSIVE_CHECKS in this case?

In D150639#4344695, @JDevlieghere wrote:

This patch also uses __FILE_NAME__ (Clang-specific extension that functions similar to FILE but only renders the last path component (the filename) instead of an invocation dependent full path to that file.) when building with clang.

Can you put this comment in LLDBAssert.h itself?

Good idea.

lldb/docs/resources/contributing.rst

* Assertions.  Assertions (from ``assert.h``) should be used liberally
   what allows us to refactor and evolve the LLDB code base.
 
 * Logging. LLDB provides a very rich logging API. When recoverable
   errors cannot reasonably be surfaced to the end user, the error may
   be written to a topical log channel.
 
 * Soft assertions.  LLDB provides ``lldbassert()`` as a soft
   alternative to cover the middle ground of situations that indicate a
-  recoverable bug in LLDB.  In a Debug configuration ``lldbassert()``
-  behaves like ``assert()``. In a Release configuration it will print a
+  recoverable bug in LLDB.  When asserts are enabled ``lldbassert()``
+  behaves like ``assert()``. When asserts are disabled, 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 ``lldbassert()`` and existing
   uses should be replaced by other means of error handling.
 
 * 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()
   <https://llvm.org/doxygen/Support_2ErrorHandling_8h.html>`_ for
   actually unreachable code such as the default in an otherwise
   exhaustive switch statement.
 
 Overall, please keep in mind that the debugger is often used as a last
 resort, and a crash in the debugger is rarely appreciated by the
 end-user.

lldb/include/lldb/Utility/LLDBAssert.h

 //===----------------- LLDBAssert.h ------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_LLDBASSERT_H
 #define LLDB_UTILITY_LLDBASSERT_H
 
-#ifdef LLDB_CONFIGURATION_DEBUG
+#ifndef NDEBUG
 #define lldbassert(x) assert(x)
 #else
+#if defined(__clang__)
+// __FILE_NAME__ is a Clang-specific extension that functions similar to
+// __FILE__ but only renders the last path component (the filename) instead of
+// an invocation dependent full path to that file.
+#define lldbassert(x)                                                          \
+  lldb_private::lldb_assert(static_cast<bool>(x), #x, __FUNCTION__,            \
+                            __FILE_NAME__, __LINE__)
+#else
 #define lldbassert(x)                                                          \
   lldb_private::lldb_assert(static_cast<bool>(x), #x, __FUNCTION__, __FILE__,  \
                             __LINE__)
 #endif
+#endif
 
 namespace lldb_private {
 void lldb_assert(bool expression, const char *expr_text, const char *func,
                  const char *file, unsigned int line);
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_LLDBASSERT_H

lldb/source/Utility/LLDBAssert.cpp

 using namespace lldb_private;
 
 void lldb_private::lldb_assert(bool expression, const char *expr_text,
                                const char *func, const char *file,
                                unsigned int line) {
   if (LLVM_LIKELY(expression))
     return;
 
-  // If asserts are enabled abort here.
-  assert(false && "lldb_assert failed");
-
 #if LLVM_SUPPORT_XCODE_SIGNPOSTS
   if (__builtin_available(macos 10.12, iOS 10, tvOS 10, watchOS 3, *)) {
     os_log_fault(OS_LOG_DEFAULT,
                  "Assertion failed: (%s), function %s, file %s, line %u\n",
                  expr_text, func, file, line);
   }
 #endif
 
-  // 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.
+  // Print a warning and encourage the user to file a bug report, similar to
+  // LLVM’s crash handler, and then return execution.
   errs() << format("Assertion failed: (%s), function %s, file %s, line %u\n",
                    expr_text, func, file, line);
   errs() << "backtrace leading to the failure:\n";
   llvm::sys::PrintStackTrace(errs());
   errs() << "please file a bug report against lldb reporting this failure "
             "log, and as many details as possible\n";
 }