[lldb][DataFormatter] VectorType: fix format for arrays with size not a power-of-2 #68907

Michael137 · 2023-10-12T16:09:06Z

To get the number of children for a VectorType (i.e.,
a type declared with a vector_size/ext_vector_type attribute)
LLDB previously did following calculation:

Get byte-size of the vector container from Clang (getTypeInfo).
Get byte-size of the element type we want to interpret the array as.
(e.g., sometimes we want to interpret an unsigned char vec[16]
as a float32[]).
numChildren = containerSize / reinterpretedElementSize

However, for step 1, clang will return us the aligned container byte-size.
So for a type such as float __attribute__((ext_vector_type(3)))
(which is an array of 3 4-byte floats), clang will round up the
byte-width of the array to 16.
(see here)

This means that for vectors where the size isn't a power-of-2, LLDB
will miscalculate the number of elements.

Solution

This patch changes step 1 such that we calculate the container size
as numElementsInSource * byteSizeOfElement.

…helper function

…unction In preparation for changing this function I cleaned it up so the conditions under which the function returns what are clearer.

To get the number of children for a VectorType (i.e., a type declared with a `vector_size`/`ext_vector_type` attribute) LLDB previously did following calculation: 1. Get byte-size of the vector container from Clang (`getTypeInfo`). 2. Get byte-size of the element type we want to interpret the array as. (e.g., sometimes we want to interpret an `unsigned char vec[16]` as a `float32[]`). 3. `numChildren = containerSize / reinterpretedElementSize` However, for step 1, clang will return us the *aligned* container byte-size. So for a type such as `float __attribute__((ext_vector_type(3)))` (which is an array of 3 4-byte floats), clang will round up the bit-width of the array to `16`. (see [here](https://github.com/llvm/llvm-project/blob/ab6a66dbec61654d0962f6abf6d6c5b776937584/clang/lib/AST/ASTContext.cpp#L1987-L1992)) This means that for vectors where the size isn't a power-of-2, LLDB will miscalculate the number of elements. **Solution** This patch changes step 1 such that we calculate the container size as `numElementsInSource * byteSizeOfElement`. This mimicks the typical `sizeof(array) / sizeof(array[0])` calculation in C.

llvmbot · 2023-10-12T16:10:13Z

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

To get the number of children for a VectorType (i.e.,
a type declared with a vector_size/ext_vector_type attribute)
LLDB previously did following calculation:

Get byte-size of the vector container from Clang (getTypeInfo).
Get byte-size of the element type we want to interpret the array as.
(e.g., sometimes we want to interpret an unsigned char vec[16]
as a float32[]).
numChildren = containerSize / reinterpretedElementSize

However, for step 1, clang will return us the aligned container byte-size.
So for a type such as float __attribute__((ext_vector_type(3)))
(which is an array of 3 4-byte floats), clang will round up the
bit-width of the array to 16.
(see here)

This means that for vectors where the size isn't a power-of-2, LLDB
will miscalculate the number of elements.

Solution

This patch changes step 1 such that we calculate the container size
as numElementsInSource * byteSizeOfElement. This mimicks the typical
sizeof(array) / sizeof(array[0]) calculation in C.

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

3 Files Affected:

(modified) lldb/source/DataFormatters/VectorType.cpp (+47-17)
(modified) lldb/test/API/functionalities/data-formatter/vector-types/TestVectorTypesFormatting.py (+7)
(modified) lldb/test/API/functionalities/data-formatter/vector-types/main.cpp (+3-1)

diff --git a/lldb/source/DataFormatters/VectorType.cpp b/lldb/source/DataFormatters/VectorType.cpp
index 4afcfa2e8e490e0..93eea9507242a9a 100644
--- a/lldb/source/DataFormatters/VectorType.cpp
+++ b/lldb/source/DataFormatters/VectorType.cpp
@@ -169,21 +169,49 @@ static lldb::Format GetItemFormatForFormat(lldb::Format format,
   }
 }
 
-static size_t CalculateNumChildren(
-    CompilerType container_type, CompilerType element_type,
-    lldb_private::ExecutionContextScope *exe_scope =
-        nullptr // does not matter here because all we trade in are basic types
-    ) {
-  std::optional<uint64_t> container_size =
-      container_type.GetByteSize(exe_scope);
-  std::optional<uint64_t> element_size = element_type.GetByteSize(exe_scope);
-
-  if (container_size && element_size && *element_size) {
-    if (*container_size % *element_size)
-      return 0;
-    return *container_size / *element_size;
-  }
-  return 0;
+/// \brief Returns the number of elements stored in a container
+/// (with element type 'container_elem_type') as if it had elements
+/// of type 'element_type'.
+///
+/// For example, a container of type
+/// `uint8_t __attribute__((vector_size(16)))` has 16 elements.
+/// But calling `CalculateNumChildren` with an 'element_type'
+/// of `float` (4-bytes) will return `4` because we are interpreting
+/// the byte-array as a `float32[]`.
+///
+/// \param[in] container_elem_type The type of the elements stored
+/// in the container we are calculating the children of.
+///
+/// \param[in] num_elements Number of 'container_elem_type's our
+/// container stores.
+///
+/// \param[in] element_type The type of elements we interpret
+/// container_type to contain for the purposes of calculating
+/// the number of children.
+///
+/// If size of the container is not a multiple of 'element_type'
+/// returns 0.
+///
+/// On error, returns 0.
+static size_t CalculateNumChildren(CompilerType container_elem_type,
+                                   uint64_t num_elements,
+                                   CompilerType element_type) {
+  std::optional<uint64_t> container_elem_size =
+      container_elem_type.GetByteSize(/* exe_scope */ nullptr);
+  if (!container_elem_size)
+    return 0;
+
+  auto container_size = *container_elem_size * num_elements;
+
+  std::optional<uint64_t> element_size =
+      element_type.GetByteSize(/* exe_scope */ nullptr);
+  if (!element_size || !*element_size)
+    return 0;
+
+  if (container_size % *element_size)
+    return 0;
+
+  return container_size / *element_size;
 }
 
 namespace lldb_private {
@@ -221,11 +249,13 @@ class VectorTypeSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
     m_parent_format = m_backend.GetFormat();
     CompilerType parent_type(m_backend.GetCompilerType());
     CompilerType element_type;
-    parent_type.IsVectorType(&element_type);
+    uint64_t num_elements;
+    parent_type.IsVectorType(&element_type, &num_elements);
     m_child_type = ::GetCompilerTypeForFormat(
         m_parent_format, element_type,
         parent_type.GetTypeSystem().GetSharedPointer());
-    m_num_children = ::CalculateNumChildren(parent_type, m_child_type);
+    m_num_children =
+        ::CalculateNumChildren(element_type, num_elements, m_child_type);
     m_item_format = GetItemFormatForFormat(m_parent_format, m_child_type);
     return false;
   }
diff --git a/lldb/test/API/functionalities/data-formatter/vector-types/TestVectorTypesFormatting.py b/lldb/test/API/functionalities/data-formatter/vector-types/TestVectorTypesFormatting.py
index 4103d62878c70f3..1839c28aeb29fc6 100644
--- a/lldb/test/API/functionalities/data-formatter/vector-types/TestVectorTypesFormatting.py
+++ b/lldb/test/API/functionalities/data-formatter/vector-types/TestVectorTypesFormatting.py
@@ -86,3 +86,10 @@ def cleanup():
         v.SetFormat(lldb.eFormatVectorOfFloat32)
         oldValueAgain = v.GetChildAtIndex(0).GetValue()
         self.assertEqual(oldValue, oldValueAgain, "same format but different values")
+
+        # Test formatter for vector types whose size is not a power-of-2
+        f3 = self.frame().FindVariable("f3")
+        self.assertEqual(f3.GetNumChildren(), 3)
+        self.assertEqual(f3.GetChildAtIndex(0).GetData().float[0], 1.25)
+        self.assertEqual(f3.GetChildAtIndex(1).GetData().float[0], 2.50)
+        self.assertEqual(f3.GetChildAtIndex(2).GetData().float[0], 2.50)
diff --git a/lldb/test/API/functionalities/data-formatter/vector-types/main.cpp b/lldb/test/API/functionalities/data-formatter/vector-types/main.cpp
index ef0a227560bc253..7f2309e776bc27e 100644
--- a/lldb/test/API/functionalities/data-formatter/vector-types/main.cpp
+++ b/lldb/test/API/functionalities/data-formatter/vector-types/main.cpp
@@ -1,8 +1,10 @@
 typedef float float4 __attribute__((ext_vector_type(4)));
-typedef  unsigned char vec __attribute__((ext_vector_type(16)));
+typedef unsigned char vec __attribute__((ext_vector_type(16)));
+typedef float float3 __attribute__((ext_vector_type(3)));
 
 int main() {
     float4 f4 = {1.25, 1.25, 2.50, 2.50};
     vec v = (vec)f4;
+    float3 f3 = f4.gba;
     return 0; // break here
 }

medismailben · 2023-10-13T04:53:36Z

lldb/source/DataFormatters/VectorType.cpp

+/// of `float` (4-bytes) will return `4` because we are interpreting
+/// the byte-array as a `float32[]`.
+///
+/// \param[in] container_type The type of the container We


nit: We -> we

medismailben · 2023-10-13T05:00:01Z

lldb/source/DataFormatters/VectorType.cpp

+/// container_type to contain for the purposes of calculating
+/// the number of children.
+///
+/// If size of the container is not a multiple of 'element_type'


nit: You could use \return block here

adrian-prantl · 2023-10-13T16:12:29Z

lldb/source/DataFormatters/VectorType.cpp

-    return *container_size / *element_size;
-  }
-  return 0;
+/// \brief Returns the number of elements stored in a container


We run Doxygen with autobrief, so this \brief should be redundant.

adrian-prantl · 2023-10-13T16:14:43Z

lldb/source/DataFormatters/VectorType.cpp

+/// returns 0.
+///
+/// On error, returns 0.
+static size_t CalculateNumChildren(CompilerType container_elem_type,


Could you, just out of principle, return optional<size_t> here even if you the use it with getValueOr(0) at the call site?

yup looks better!

github-actions · 2023-10-13T18:40:39Z

✅ With the latest revision this PR passed the C/C++ code formatter.

… a power-of-2 (llvm#68907) To get the number of children for a VectorType (i.e., a type declared with a `vector_size`/`ext_vector_type` attribute) LLDB previously did following calculation: 1. Get byte-size of the vector container from Clang (`getTypeInfo`). 2. Get byte-size of the element type we want to interpret the array as. (e.g., sometimes we want to interpret an `unsigned char vec[16]` as a `float32[]`). 3. `numChildren = containerSize / reinterpretedElementSize` However, for step 1, clang will return us the *aligned* container byte-size. So for a type such as `float __attribute__((ext_vector_type(3)))` (which is an array of 3 4-byte floats), clang will round up the byte-width of the array to `16`. (see [here](https://github.com/llvm/llvm-project/blob/ab6a66dbec61654d0962f6abf6d6c5b776937584/clang/lib/AST/ASTContext.cpp#L1987-L1992)) This means that for vectors where the size isn't a power-of-2, LLDB will miscalculate the number of elements. **Solution** This patch changes step 1 such that we calculate the container size as `numElementsInSource * byteSizeOfElement`. (cherry picked from commit 0dfcfb5)

Michael137 added 3 commits October 12, 2023 17:07

[lldb][DataFormatter][NFC] VectorType: remove redundant parameter to …

769cbd0

…helper function

[lldb][DataFormatter][NFC] VectorType: clean up and document helper f…

c3eeef2

…unction In preparation for changing this function I cleaned it up so the conditions under which the function returns what are clearer.

Michael137 requested review from adrian-prantl and jimingham October 12, 2023 16:09

Michael137 requested a review from JDevlieghere as a code owner October 12, 2023 16:09

llvmbot added the lldb label Oct 12, 2023

Michael137 changed the title ~~[lldb][DataFormatter] Fix GetNumChildren for VectorType formatter~~ [lldb][DataFormatter] VectorType: fix format for arrays with size not a power-of-2 Oct 12, 2023

medismailben reviewed Oct 13, 2023

View reviewed changes

Michael137 requested a review from clayborg October 13, 2023 09:41

adrian-prantl reviewed Oct 13, 2023

View reviewed changes

adrian-prantl approved these changes Oct 13, 2023

View reviewed changes

Michael137 added 2 commits October 13, 2023 19:04

fixup! fix doxygen comment and use std::optional

d0d6425

fixup! fix doxygen

bdcad25

Michael137 merged commit 0dfcfb5 into llvm:main Oct 13, 2023

madhur13490 mentioned this pull request Oct 20, 2023

Revert commit ba8565fbcb975e2d067ce3ae5a7dbaae4953edd3 madhur13490/llvm-project#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb][DataFormatter] VectorType: fix format for arrays with size not a power-of-2 #68907

[lldb][DataFormatter] VectorType: fix format for arrays with size not a power-of-2 #68907

Uh oh!

Michael137 commented Oct 12, 2023 •

edited

Loading

Uh oh!

llvmbot commented Oct 12, 2023

Uh oh!

medismailben Oct 13, 2023

Uh oh!

medismailben Oct 13, 2023

Uh oh!

adrian-prantl Oct 13, 2023

Uh oh!

Michael137 Oct 13, 2023

Uh oh!

adrian-prantl Oct 13, 2023

Uh oh!

Michael137 Oct 13, 2023

Uh oh!

github-actions bot commented Oct 13, 2023 •

edited

Loading

Uh oh!

Uh oh!

[lldb][DataFormatter] VectorType: fix format for arrays with size not a power-of-2 #68907

[lldb][DataFormatter] VectorType: fix format for arrays with size not a power-of-2 #68907

Uh oh!

Conversation

Michael137 commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 12, 2023

Uh oh!

medismailben Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

medismailben Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

adrian-prantl Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

Michael137 Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

adrian-prantl Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

Michael137 Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Michael137 commented Oct 12, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023 •

edited

Loading