[lldb][ClangExpressionParser] Log and assert on failure to create TargetInfo

[Michael137]

CreateTargetInfo can return a nullptr in a couple cases. So we should log that and let the user know something is wrong (hence the lldbassert).

I didn't actually run into this. Just stumbled upon it from reading the code.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

CreateTargetInfo can return a nullptr in a couple cases. So we should log that and let the user know something is wrong (hence the lldbassert).

I didn't actually run into this. Just stumbled upon it from reading the code.

---

Full diff: https://github.com/llvm/llvm-project/pull/101697.diff

1 Files Affected:

• (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+17-11)

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 303e88feea20b..9d9403ebb9051 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -465,18 +465,24 @@ ClangExpressionParser::ClangExpressionParser(
   // A value of 0 means no limit for both LLDB and Clang.
   m_compiler->getDiagnostics().setErrorLimit(target_sp->GetExprErrorLimit());
 
-  auto target_info = TargetInfo::CreateTargetInfo(
-      m_compiler->getDiagnostics(), m_compiler->getInvocation().TargetOpts);
-  if (log) {
-    LLDB_LOGF(log, "Target datalayout string: '%s'",
-              target_info->getDataLayoutString());
-    LLDB_LOGF(log, "Target ABI: '%s'", target_info->getABI().str().c_str());
-    LLDB_LOGF(log, "Target vector alignment: %d",
-              target_info->getMaxVectorAlign());
-  }
-  m_compiler->setTarget(target_info);
+  if (auto *target_info = TargetInfo::CreateTargetInfo(
+          m_compiler->getDiagnostics(),
+          m_compiler->getInvocation().TargetOpts)) {
+    if (log) {
+      LLDB_LOGF(log, "Target datalayout string: '%s'",
+                target_info->getDataLayoutString());
+      LLDB_LOGF(log, "Target ABI: '%s'", target_info->getABI().str().c_str());
+      LLDB_LOGF(log, "Target vector alignment: %d",
+                target_info->getMaxVectorAlign());
+    }
+    m_compiler->setTarget(target_info);
+  } else {
+    if (log)
+      LLDB_LOGF(log, "Failed to create TargetInfo for '%s'",
+                m_compiler->getTargetOpts().Triple.c_str());
 
-  assert(m_compiler->hasTarget());
+    lldbassert(false && "Failed to create TargetInfo.");
+  }
 
   // 4. Set language options.
   lldb::LanguageType language = expr.Language().AsLanguageType();

[adrian-prantl]

Based on that there used to be an assert here, should this return?
Same comment as in the previous review — ideally this would return an llvm::Error here if it weren't a constructor.

[Michael137]

It's a bit awkward because, as you also noted, this all happens in the constructor. And stopping the construction half-way probably won't buy us much. I think this is one of those unusual circumstances where we want to assert at runtime. I guess we could have a separate Init method for the ClangExpressionParser. Which returns Expected. Then update all the users to check the return of that.

But that's a larger refactor that we can do at some later point.

What used to happen in release builds (tbh I'm not sure if this ever really gets exercised in practice), is that target_info was a nullptr. And we just happily continued with the construction of the object.