From 34a35cd41827186d6631b1f6690d7fc5ea6aa0d6 Mon Sep 17 00:00:00 2001
From: Jonathan Haslam <jonhaslam@meta.com>
Date: Thu, 15 Feb 2024 10:18:55 -0800
Subject: [PATCH] Correct packing calculation (#485)

Summary:

Correct identification of packing.

Tests were modified to reflect the new behaviour. One test was removed as it was bogus - the flattener pass runs before the alignmentcalc pass and therefore the layout in the test could never happen (i.e. it has a hierarchy).

Reviewed By: JakeHillion

Differential Revision: D53815661
---
 oi/type_graph/AlignmentCalc.cpp   |  2 +-
 oi/type_graph/ClangTypeParser.cpp |  1 +
 test/test_alignment_calc.cpp      | 26 ++------------------------
 3 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/oi/type_graph/AlignmentCalc.cpp b/oi/type_graph/AlignmentCalc.cpp
index 9abad71..aac9aca 100644
--- a/oi/type_graph/AlignmentCalc.cpp
+++ b/oi/type_graph/AlignmentCalc.cpp
@@ -70,7 +70,7 @@ void AlignmentCalc::visit(Class& c) {
     c.setAlign(alignment);
   }
 
-  if (c.size() % c.align() != 0) {
+  if (c.align() == 1 || c.size() % c.align() != 0) {
     // Mark as packed if there is no tail padding
     c.setPacked();
   }
diff --git a/oi/type_graph/ClangTypeParser.cpp b/oi/type_graph/ClangTypeParser.cpp
index db486e6..c9e52cc 100644
--- a/oi/type_graph/ClangTypeParser.cpp
+++ b/oi/type_graph/ClangTypeParser.cpp
@@ -330,6 +330,7 @@ void ClangTypeParser::enumerateClassMembers(const clang::RecordType& ty,
 
     auto& mtype = enumerateType(*qualType);
     Member m{mtype, std::move(member_name), offset_in_bits, size_in_bits};
+    m.align = field->getMaxAlignment() / 8;
     members.push_back(m);
   }
 
diff --git a/test/test_alignment_calc.cpp b/test/test_alignment_calc.cpp
index 2526cbe..e69cfee 100644
--- a/test/test_alignment_calc.cpp
+++ b/test/test_alignment_calc.cpp
@@ -131,7 +131,7 @@ TEST(AlignmentCalcTest, RecurseClassParam) {
             Primitive: int64_t
 )",
        R"(
-[0] Class: MyClass (size: 0, align: 1)
+[0] Class: MyClass (size: 0, align: 1, packed)
       Param
 [1]     Class: ClassA (size: 16, align: 8)
           Member: a (offset: 0, align: 1)
@@ -141,28 +141,6 @@ TEST(AlignmentCalcTest, RecurseClassParam) {
 )");
 }
 
-TEST(AlignmentCalcTest, RecurseClassParent) {
-  test(AlignmentCalc::createPass(),
-       R"(
-[0] Class: MyClass (size: 0)
-      Parent (offset: 0)
-[1]     Class: ClassA (size: 16)
-          Member: a (offset: 0)
-            Primitive: int8_t
-          Member: b (offset: 8)
-            Primitive: int64_t
-)",
-       R"(
-[0] Class: MyClass (size: 0, align: 1)
-      Parent (offset: 0)
-[1]     Class: ClassA (size: 16, align: 8)
-          Member: a (offset: 0, align: 1)
-            Primitive: int8_t
-          Member: b (offset: 8, align: 8)
-            Primitive: int64_t
-)");
-}
-
 TEST(AlignmentCalcTest, RecurseClassMember) {
   test(AlignmentCalc::createPass(),
        R"(
@@ -197,7 +175,7 @@ TEST(AlignmentCalcTest, RecurseClassChild) {
             Primitive: int64_t
 )",
        R"(
-[0] Class: MyClass (size: 0, align: 1)
+[0] Class: MyClass (size: 0, align: 1, packed)
       Child
 [1]     Class: ClassA (size: 16, align: 8)
           Member: a (offset: 0, align: 1)