[lldb][TypeSystemClang] Create VLAs of explicitly 0-size as ConstantArrayType

[Michael137]

Depends on #100674

Currently, we treat VLAs declared as int[] and int[0] identically. I.e., we create them as IncompleteArrayTypes. However, the DW_AT_count for int[0] does exist, and is retrievable without an execution context. This patch decouples the notion of "has 0 elements" from "has no known DW_AT_count".

This aligns with how Clang represents int[0] in the AST (it treats it as a ConstantArrayType of 0 size).

This issue was surfaced when adapting LLDB to #93069. There, the __compressed_pair_padding type has a char[0] member. If we previously got the __compressed_pair_padding out of a module (where clang represents char[0] as a ConstantArrayType), and try to merge the AST with one we got from DWARF (where LLDB used to represent char[0] as an IncompleteArrayType), the AST structural equivalence check fails, resulting in silent ASTImporter failures. This manifested in a failure in TestNonModuleTypeSeparation.py.

Implementation

1. Adjust ParseChildArrayInfo to store the element counts of each VLA dimension as an optional<uint64_t>, instead of a regular uint64_t. So when we pass this down to CreateArrayType, we have a better heuristic for what is an IncompleteArrayType.
2. In TypeSystemClang::GetBitSize, if we encounter a ConstantArrayType simply return the size that it was created with. If we couldn't determine the authoritative bound from DWARF during parsing, we would've created an IncompleteArrayType. This ensures that GetBitSize on arrays with DW_AT_count 0x0 returns 0 (whereas previously it would've returned a std::nullopt, causing that FieldDecl to just get dropped during printing)

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

Depends on #100674

Currently, we treat VLAs declared as int[] and int[0] identically. I.e., we create them as IncompleteArrayTypes. However, the DW_AT_count for int[0] does exist, and is retrievable without an execution context. This patch decouples the notion of "has 0 elements" from "has no known DW_AT_count".

This aligns with how Clang represents int[0] in the AST (it treats it as a ConstantArrayType of 0 size).

This issue was surfaced when adapting LLDB to #93069. There, the __compressed_pair_padding type has a char[0] member. If we previously got the __compressed_pair_padding out of a module (where clang represents char[0] as a ConstantArrayType), and try to merge the AST with one we got from DWARF (where LLDB used to represent char[0] as an IncompleteArrayType), the AST structural equivalence check fails, resulting in silent ASTImporter failures. This manifested in a failure in TestNonModuleTypeSeparation.py.

---

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

7 Files Affected:

• (modified) lldb/include/lldb/Symbol/SymbolFile.h (+1-1)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.cpp (+3-2)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+6-6)
• (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp (+84-80)
• (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h (+5-1)
• (modified) lldb/test/API/lang/c/struct_types/main.c (+2-1)
• (added) lldb/test/Shell/SymbolFile/DWARF/vla.cpp (+80)

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 f70efe5ed57e4..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(
@@ -4725,67 +4726,70 @@ 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::ConstantArray:
+  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;
 }
 
@@ -5456,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 d67b7a4c9fe72..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,
@@ -1172,6 +1173,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;
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();
+}

[Michael137]

This is also broken without this patch. E.g., accessing elements of a VLA with non-constant bounds will return garbage. My hunch is that we just calculate the stride incorrectly (or something related to calculating children count).

[medismailben]

@Michael137 This broke the windows bot: https://lab.llvm.org/buildbot/#/builders/141/builds/1214 Should we skip it on that platform or do you see an easy fix ?

[Michael137]

@Michael137 This broke the windows bot: https://lab.llvm.org/buildbot/#/builders/141/builds/1214 Should we skip it on that platform or do you see an easy fix ?

Thanks for pinging. This is the failure:

 | error: no variable named 'f' found in this frame
# `-----------------------------
# executed command: 'c:\users\tcwg\llvm-worker\lldb-aarch64-windows\build\bin\filecheck.exe' 'C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\Shell\SymbolFile\DWARF\vla.cpp'
# .---command stderr------------
# | C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\Shell\SymbolFile\DWARF\vla.cpp:26:17: error: CHECK-NEXT: expected string not found in input
# |  // CHECK-NEXT: (Foo) f = {
# |                 ^
# | <stdin>:16:32: note: scanning from here
# | (lldb) frame var --show-types f
# |                                ^
# | 
# | Input file: <stdin>
# | Check file: C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\Shell\SymbolFile\DWARF\vla.cpp
# | 
# | -dump-input=help explains the following input dump.
# | 
# | Input was:
# | <<<<<<
# |          .
# |          .
# |          .
# |         11: vla.cpp.tmp`main: 
# |         12: -> 0x7ff66e41eb18 <+396>: ldur x8, [x29, #-0x28] 
# |         13:  0x7ff66e41eb1c <+400>: mov sp, x8 
# |         14:  0x7ff66e41eb20 <+404>: ldur w0, [x29, #-0x4] 
# |         15:  0x7ff66e41eb24 <+408>: mov sp, x29 
# |         16: (lldb) frame var --show-types f 
# | next:26                                    X error: no match found
# | >>>>>>
# `-----------------------------

Looks strange. Not sure why it's not finding the symbols properly.

I'm tempted to skip this on Windows since I don't have an LLDB setup to test this on. @DavidSpickett Am I remembering correctly that you might have an AArch64 Windows setup? If so, does this reproduce for you? Would be greatly appreciated! I'm confused as to why the debug symbols don't seem to be found.

[DavidSpickett]

Linaro does yes, I think @omjavaid is currently dealing with that bot.

[DavidSpickett]

I'm going to look into this now.

[DavidSpickett]

Still need to confirm but judging by 6cfac49, this test just needs to be unsupported on Windows until we use lld to link there.

[DavidSpickett]

Yeah, won't work with link.exe, so I've just disabled it. Nothing more for you to do here.

[Michael137]

Yeah, won't work with link.exe, so I've just disabled it. Nothing more for you to do here.

Awesome, thanks for the fix!

Out of curiosity, what's stopping us from using lld on Windows?

[DavidSpickett]

I don't know, it never occurred to me to do so before now. @omjavaid mentioned it so maybe he knows something about it.

[labath]

Out of curiosity, what's stopping us from using lld on Windows?

It's not the default configuration of clang -- anywhere. The difference is that on linux/elf, the choice of linker has very little impact on the debug info.