[Lldb-commits] [lldb] [lldb][TypeSystemClang] Create VLAs of explicitly 0-size as ConstantArrayType (PR #100710)

Michael Buch via lldb-commits lldb-commits at lists.llvm.org
Fri Jul 26 03:00:50 PDT 2024


https://github.com/Michael137 updated https://github.com/llvm/llvm-project/pull/100710

>From 9136653363bb7fc58e0b7f55a714d36d4613fe24 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Fri, 26 Jul 2024 01:19:02 +0100
Subject: [PATCH 1/3] [lldb][TypeSystemClang][NFC] Clean up
 TypeSystemClang::GetBitSize

This patch does following NFC changes to TypeSystemClang::GetBitSize:
* Factor out the Objective-C logic into a helper function.
* We had a redundant check for `GetCompleteType` in the `RecordType`
  case. We can remove that switch case entirely and rely on the
  `default` case.
* Introduce a new case for `FunctionProto`. I don't see a good reason
  for special-casing this in the `default` case.
* Introduce a new case for `IncompleteArray`.

The motivation for this is that there are a few issues around VLAs
(i.e., `Type::IncompleteArray`) whose fixes will be easier to reason
about after this refactor.
---
 .../TypeSystem/Clang/TypeSystemClang.cpp      | 114 +++++++++---------
 .../TypeSystem/Clang/TypeSystemClang.h        |   3 +
 2 files changed, 61 insertions(+), 56 deletions(-)

diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index f70efe5ed57e4..3f7a6344542f4 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -4725,67 +4725,69 @@ TypeSystemClang::GetFloatTypeSemantics(size_t byte_size) {
   return llvm::APFloatBase::Bogus();
 }
 
+std::optional<uint64_t>
+TypeSystemClang::GetObjCBitSize(QualType qual_type,
+                                ExecutionContextScope *exe_scope) {
+  assert(qual_type->isObjCObjectOrInterfaceType());
+  ExecutionContext exe_ctx(exe_scope);
+  Process *process = exe_ctx.GetProcessPtr();
+  if (process) {
+    if (ObjCLanguageRuntime *objc_runtime =
+            ObjCLanguageRuntime::Get(*process)) {
+      if (std::optional<uint64_t> bit_size =
+              objc_runtime->GetTypeBitSize(GetType(qual_type)))
+        return *bit_size;
+    }
+  } else {
+    static bool g_printed = false;
+    if (!g_printed) {
+      StreamString s;
+      DumpTypeDescription(qual_type.getAsOpaquePtr(), s);
+
+      llvm::outs() << "warning: trying to determine the size of type ";
+      llvm::outs() << s.GetString() << "\n";
+      llvm::outs() << "without a valid ExecutionContext. this is not "
+                      "reliable. please file a bug against LLDB.\n";
+      llvm::outs() << "backtrace:\n";
+      llvm::sys::PrintStackTrace(llvm::outs());
+      llvm::outs() << "\n";
+      g_printed = true;
+    }
+  }
+
+  return getASTContext().getTypeSize(qual_type) +
+         getASTContext().getTypeSize(getASTContext().ObjCBuiltinClassTy);
+}
+
 std::optional<uint64_t>
 TypeSystemClang::GetBitSize(lldb::opaque_compiler_type_t type,
                             ExecutionContextScope *exe_scope) {
-  if (GetCompleteType(type)) {
-    clang::QualType qual_type(GetCanonicalQualType(type));
-    const clang::Type::TypeClass type_class = qual_type->getTypeClass();
-    switch (type_class) {
-    case clang::Type::Record:
-      if (GetCompleteType(type))
-        return getASTContext().getTypeSize(qual_type);
-      else
-        return std::nullopt;
-      break;
+  if (!GetCompleteType(type))
+    return std::nullopt;
 
-    case clang::Type::ObjCInterface:
-    case clang::Type::ObjCObject: {
-      ExecutionContext exe_ctx(exe_scope);
-      Process *process = exe_ctx.GetProcessPtr();
-      if (process) {
-        if (ObjCLanguageRuntime *objc_runtime =
-                ObjCLanguageRuntime::Get(*process)) {
-          if (std::optional<uint64_t> bit_size =
-                  objc_runtime->GetTypeBitSize(GetType(qual_type)))
-            return *bit_size;
-        }
-      } else {
-        static bool g_printed = false;
-        if (!g_printed) {
-          StreamString s;
-          DumpTypeDescription(type, s);
-
-          llvm::outs() << "warning: trying to determine the size of type ";
-          llvm::outs() << s.GetString() << "\n";
-          llvm::outs() << "without a valid ExecutionContext. this is not "
-                          "reliable. please file a bug against LLDB.\n";
-          llvm::outs() << "backtrace:\n";
-          llvm::sys::PrintStackTrace(llvm::outs());
-          llvm::outs() << "\n";
-          g_printed = true;
-        }
-      }
-    }
-      [[fallthrough]];
-    default:
-      const uint32_t bit_size = getASTContext().getTypeSize(qual_type);
-      if (bit_size == 0) {
-        if (qual_type->isIncompleteArrayType())
-          return getASTContext().getTypeSize(
-              qual_type->getArrayElementTypeNoTypeQual()
-                  ->getCanonicalTypeUnqualified());
-      }
-      if (qual_type->isObjCObjectOrInterfaceType())
-        return bit_size +
-               getASTContext().getTypeSize(getASTContext().ObjCBuiltinClassTy);
-      // Function types actually have a size of 0, that's not an error.
-      if (qual_type->isFunctionProtoType())
-        return bit_size;
-      if (bit_size)
-        return bit_size;
-    }
+  clang::QualType qual_type(GetCanonicalQualType(type));
+  const clang::Type::TypeClass type_class = qual_type->getTypeClass();
+  switch (type_class) {
+  case clang::Type::FunctionProto:
+  case clang::Type::Record:
+    return getASTContext().getTypeSize(qual_type);
+  case clang::Type::ObjCInterface:
+  case clang::Type::ObjCObject:
+    return GetObjCBitSize(qual_type, exe_scope);
+  case clang::Type::IncompleteArray: {
+    const uint64_t bit_size = getASTContext().getTypeSize(qual_type);
+    if (bit_size == 0)
+      return getASTContext().getTypeSize(
+          qual_type->getArrayElementTypeNoTypeQual()
+              ->getCanonicalTypeUnqualified());
+
+    return bit_size;
+  }
+  default:
+    if (const uint64_t bit_size = getASTContext().getTypeSize(qual_type))
+      return bit_size;
   }
+
   return std::nullopt;
 }
 
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
index d67b7a4c9fe72..70722eb375ab7 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -1172,6 +1172,9 @@ class TypeSystemClang : public TypeSystem {
   /// on creation of a new instance.
   void LogCreation() const;
 
+  std::optional<uint64_t> GetObjCBitSize(clang::QualType qual_type,
+                                         ExecutionContextScope *exe_scope);
+
   // Classes that inherit from TypeSystemClang can see and modify these
   std::string m_target_triple;
   std::unique_ptr<clang::ASTContext> m_ast_up;

>From 4a0c7552531d23ba573dc7b9fdd5eb66e478bc84 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Fri, 26 Jul 2024 01:58:54 +0100
Subject: [PATCH 2/3] [lldb][TypeSystemClang] Don't create VLAs of explicitly
 0-size as IncompleteArrayTypes

---
 lldb/include/lldb/Symbol/SymbolFile.h         |  2 +-
 .../SymbolFile/DWARF/DWARFASTParser.cpp       |  5 +-
 .../SymbolFile/DWARF/DWARFASTParserClang.cpp  | 12 +--
 .../TypeSystem/Clang/TypeSystemClang.cpp      | 50 ++++++------
 .../TypeSystem/Clang/TypeSystemClang.h        |  3 +-
 lldb/test/API/lang/c/struct_types/main.c      |  3 +-
 lldb/test/Shell/SymbolFile/DWARF/vla.cpp      | 80 +++++++++++++++++++
 7 files changed, 120 insertions(+), 35 deletions(-)
 create mode 100644 lldb/test/Shell/SymbolFile/DWARF/vla.cpp

diff --git a/lldb/include/lldb/Symbol/SymbolFile.h b/lldb/include/lldb/Symbol/SymbolFile.h
index d20766788192f..a050a04a5573b 100644
--- a/lldb/include/lldb/Symbol/SymbolFile.h
+++ b/lldb/include/lldb/Symbol/SymbolFile.h
@@ -211,7 +211,7 @@ class SymbolFile : public PluginInterface {
   /// The characteristics of an array type.
   struct ArrayInfo {
     int64_t first_index = 0;
-    llvm::SmallVector<uint64_t, 1> element_orders;
+    llvm::SmallVector<std::optional<uint64_t>, 1> element_orders;
     uint32_t byte_stride = 0;
     uint32_t bit_stride = 0;
   };
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
index 409e9bb37ab28..b3d231604aa44 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
@@ -37,7 +37,7 @@ DWARFASTParser::ParseChildArrayInfo(const DWARFDIE &parent_die,
     if (attributes.Size() == 0)
       continue;
 
-    uint64_t num_elements = 0;
+    std::optional<uint64_t> num_elements;
     uint64_t lower_bound = 0;
     uint64_t upper_bound = 0;
     bool upper_bound_valid = false;
@@ -66,6 +66,7 @@ DWARFASTParser::ParseChildArrayInfo(const DWARFDIE &parent_die,
               }
           } else
             num_elements = form_value.Unsigned();
+
           break;
 
         case DW_AT_bit_stride:
@@ -91,7 +92,7 @@ DWARFASTParser::ParseChildArrayInfo(const DWARFDIE &parent_die,
       }
     }
 
-    if (num_elements == 0) {
+    if (!num_elements || *num_elements == 0) {
       if (upper_bound_valid && upper_bound >= lower_bound)
         num_elements = upper_bound - lower_bound + 1;
     }
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 85c59a605c675..a4dcde1629fc2 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1395,20 +1395,20 @@ DWARFASTParserClang::ParseArrayType(const DWARFDIE &die,
   uint64_t array_element_bit_stride = byte_stride * 8 + bit_stride;
   CompilerType clang_type;
   if (array_info && array_info->element_orders.size() > 0) {
-    uint64_t num_elements = 0;
     auto end = array_info->element_orders.rend();
     for (auto pos = array_info->element_orders.rbegin(); pos != end; ++pos) {
-      num_elements = *pos;
-      clang_type = m_ast.CreateArrayType(array_element_type, num_elements,
-                                         attrs.is_vector);
+      clang_type = m_ast.CreateArrayType(
+          array_element_type, /*element_count=*/*pos, attrs.is_vector);
+
+      uint64_t num_elements = pos->value_or(0);
       array_element_type = clang_type;
       array_element_bit_stride = num_elements
                                      ? array_element_bit_stride * num_elements
                                      : array_element_bit_stride;
     }
   } else {
-    clang_type =
-        m_ast.CreateArrayType(array_element_type, 0, attrs.is_vector);
+    clang_type = m_ast.CreateArrayType(
+        array_element_type, /*element_count=*/std::nullopt, attrs.is_vector);
   }
   ConstString empty_name;
   TypeSP type_sp =
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 3f7a6344542f4..411778f490c4b 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -2233,30 +2233,31 @@ TypeSystemClang::CreateBlockPointerType(const CompilerType &function_type) {
 
 #pragma mark Array Types
 
-CompilerType TypeSystemClang::CreateArrayType(const CompilerType &element_type,
-                                              size_t element_count,
-                                              bool is_vector) {
-  if (element_type.IsValid()) {
-    ASTContext &ast = getASTContext();
+CompilerType
+TypeSystemClang::CreateArrayType(const CompilerType &element_type,
+                                 std::optional<size_t> element_count,
+                                 bool is_vector) {
+  if (!element_type.IsValid())
+    return {};
 
-    if (is_vector) {
-      return GetType(ast.getExtVectorType(ClangUtil::GetQualType(element_type),
-                                          element_count));
-    } else {
+  ASTContext &ast = getASTContext();
 
-      llvm::APInt ap_element_count(64, element_count);
-      if (element_count == 0) {
-        return GetType(
-            ast.getIncompleteArrayType(ClangUtil::GetQualType(element_type),
-                                       clang::ArraySizeModifier::Normal, 0));
-      } else {
-        return GetType(ast.getConstantArrayType(
-            ClangUtil::GetQualType(element_type), ap_element_count, nullptr,
-            clang::ArraySizeModifier::Normal, 0));
-      }
-    }
-  }
-  return CompilerType();
+  // Unknown number of elements; this is an incomplete array
+  // (e.g., variable length array with non-constant bounds, or
+  // a flexible array member).
+  if (!element_count)
+    return GetType(
+        ast.getIncompleteArrayType(ClangUtil::GetQualType(element_type),
+                                   clang::ArraySizeModifier::Normal, 0));
+
+  if (is_vector)
+    return GetType(ast.getExtVectorType(ClangUtil::GetQualType(element_type),
+                                        *element_count));
+
+  llvm::APInt ap_element_count(64, *element_count);
+  return GetType(ast.getConstantArrayType(ClangUtil::GetQualType(element_type),
+                                          ap_element_count, nullptr,
+                                          clang::ArraySizeModifier::Normal, 0));
 }
 
 CompilerType TypeSystemClang::CreateStructForIdentifier(
@@ -4768,6 +4769,7 @@ TypeSystemClang::GetBitSize(lldb::opaque_compiler_type_t type,
   clang::QualType qual_type(GetCanonicalQualType(type));
   const clang::Type::TypeClass type_class = qual_type->getTypeClass();
   switch (type_class) {
+  case clang::Type::ConstantArray:
   case clang::Type::FunctionProto:
   case clang::Type::Record:
     return getASTContext().getTypeSize(qual_type);
@@ -5458,9 +5460,9 @@ TypeSystemClang::GetNumChildren(lldb::opaque_compiler_type_t type,
   case clang::Type::IncompleteArray:
     if (auto array_info =
             GetDynamicArrayInfo(*this, GetSymbolFile(), qual_type, exe_ctx))
-      // Only 1-dimensional arrays are supported.
+      // FIXME: Only 1-dimensional arrays are supported.
       num_children = array_info->element_orders.size()
-                         ? array_info->element_orders.back()
+                         ? array_info->element_orders.back().value_or(0)
                          : 0;
     break;
 
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
index 70722eb375ab7..56a5c0a516706 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -498,7 +498,8 @@ class TypeSystemClang : public TypeSystem {
   // Array Types
 
   CompilerType CreateArrayType(const CompilerType &element_type,
-                               size_t element_count, bool is_vector);
+                               std::optional<size_t> element_count,
+                               bool is_vector);
 
   // Enumeration Types
   CompilerType CreateEnumerationType(llvm::StringRef name,
diff --git a/lldb/test/API/lang/c/struct_types/main.c b/lldb/test/API/lang/c/struct_types/main.c
index e683c49108976..70217c57bec5f 100644
--- a/lldb/test/API/lang/c/struct_types/main.c
+++ b/lldb/test/API/lang/c/struct_types/main.c
@@ -1,3 +1,4 @@
+// clang-format off
 struct things_to_sum {
     int a;
     int b;
@@ -18,7 +19,7 @@ int main (int argc, char const *argv[])
     }; //% self.expect("frame variable pt.padding[0]", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["pt.padding[0] = "])
        //% self.expect("frame variable pt.padding[1]", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["pt.padding[1] = "])
        //% self.expect_expr("pt.padding[0]", result_type="char")
-       //% self.expect("image lookup -t point_tag", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ['padding[]'])
+       //% self.expect("image lookup -t point_tag", DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ['padding[0]'])
 
     struct {} empty;
     //% self.expect("frame variable empty", substrs = ["empty = {}"])
diff --git a/lldb/test/Shell/SymbolFile/DWARF/vla.cpp b/lldb/test/Shell/SymbolFile/DWARF/vla.cpp
new file mode 100644
index 0000000000000..344b100efd9f9
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/vla.cpp
@@ -0,0 +1,80 @@
+// RUN: %clangxx_host -gdwarf -std=c++11 -o %t %s
+// RUN: %lldb %t \
+// RUN:   -o run \
+// RUN:   -o "frame var --show-types f" \
+// RUN:   -o "frame var vla0" \
+// RUN:   -o "frame var fla0" \
+// RUN:   -o "frame var fla1" \
+// RUN:   -o "frame var vla01" \
+// RUN:   -o "frame var vla10" \
+// RUN:   -o "frame var vlaN" \
+// RUN:   -o "frame var vlaNM" \
+// RUN:   -o exit | FileCheck %s
+
+struct Foo {
+  static constexpr int n = 1;
+  int m_vlaN[n];
+
+  int m_vla0[0];
+};
+
+int main() {
+  Foo f;
+  f.m_vlaN[0] = 60;
+
+  // CHECK:      (lldb) frame var --show-types f
+  // CHECK-NEXT: (Foo) f = {
+  // CHECK-NEXT:   (int[1]) m_vlaN = {
+  // CHECK-NEXT:     (int) [0] = 60
+  // CHECK-NEXT:   }
+  // CHECK-NEXT:   (int[0]) m_vla0 = {}
+  // CHECK-NEXT: }
+
+  int vla0[0] = {};
+
+  // CHECK:      (lldb) frame var vla0
+  // CHECK-NEXT: (int[0]) vla0 = {}
+
+  int fla0[] = {};
+
+  // CHECK:      (lldb) frame var fla0
+  // CHECK-NEXT: (int[0]) fla0 = {}
+
+  int fla1[] = {42};
+
+  // CHECK:      (lldb) frame var fla1
+  // CHECK-NEXT: (int[1]) fla1 = ([0] = 42)
+
+  int vla01[0][1];
+
+  // CHECK:      (lldb) frame var vla01
+  // CHECK-NEXT: (int[0][1]) vla01 = {}
+
+  int vla10[1][0];
+
+  // CHECK:      (lldb) frame var vla10
+  // CHECK-NEXT: (int[1][0]) vla10 = ([0] = int[0]
+
+  int n = 3;
+  int vlaN[n];
+  for (int i = 0; i < n; ++i)
+    vlaN[i] = -i;
+
+  // CHECK:      (lldb) frame var vlaN
+  // CHECK-NEXT: (int[]) vlaN = ([0] = 0, [1] = -1, [2] = -2)
+
+  int m = 2;
+  int vlaNM[n][m];
+  for (int i = 0; i < n; ++i)
+    for (int j = 0; j < m; ++j)
+      vlaNM[i][j] = i + j;
+
+  // FIXME: multi-dimensional VLAs aren't well supported
+  // CHECK:      (lldb) frame var vlaNM
+  // CHECK-NEXT: (int[][]) vlaNM = {
+  // CHECK-NEXT:   [0] = ([0] = 0, [1] = 1, [2] = 1)
+  // CHECK-NEXT:   [1] = ([0] = 1, [1] = 1, [2] = 2)
+  // CHECK-NEXT: }
+
+  __builtin_debugtrap();
+}

>From 4cc93e1735286da3962976d65c1b3b827eeeb0fc Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Fri, 26 Jul 2024 11:00:32 +0100
Subject: [PATCH 3/3] fixup! add doxygen comment to element_orders member;
 revert redundant whitespace change

---
 lldb/include/lldb/Symbol/SymbolFile.h                   | 8 ++++++++
 lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp | 1 -
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/lldb/include/lldb/Symbol/SymbolFile.h b/lldb/include/lldb/Symbol/SymbolFile.h
index a050a04a5573b..8419495da73a2 100644
--- a/lldb/include/lldb/Symbol/SymbolFile.h
+++ b/lldb/include/lldb/Symbol/SymbolFile.h
@@ -211,6 +211,14 @@ class SymbolFile : public PluginInterface {
   /// The characteristics of an array type.
   struct ArrayInfo {
     int64_t first_index = 0;
+
+    ///< Each entry belongs to a distinct DW_TAG_subrange_type.
+    ///< For multi-dimensional DW_TAG_array_types we would have
+    ///< an entry for each dimension. An entry represents the
+    ///< optional element count of the subrange.
+    ///
+    ///< The order of entries follows the order of the DW_TAG_subrange_type
+    ///< children of this DW_TAG_array_type.
     llvm::SmallVector<std::optional<uint64_t>, 1> element_orders;
     uint32_t byte_stride = 0;
     uint32_t bit_stride = 0;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
index b3d231604aa44..4ed523bbb9e76 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp
@@ -66,7 +66,6 @@ DWARFASTParser::ParseChildArrayInfo(const DWARFDIE &parent_die,
               }
           } else
             num_elements = form_value.Unsigned();
-
           break;
 
         case DW_AT_bit_stride:



More information about the lldb-commits mailing list