From 7eebee2bf78e707eaf5400ccdfd411f64ced9c27 Mon Sep 17 00:00:00 2001
From: Jake Hillion <jakehillion@meta.com>
Date: Thu, 18 Jan 2024 16:24:15 +0000
Subject: [PATCH] type_graph: avoid overwriting explicitly set alignment

Previously AlignmentCalc calculates the alignment and sets packing for every
type except a member with explicit alignment. Change this to check whether an
alignment has been previously set for a type before calculating it. Use this in
ClangTypeParser where the full alignment of the type is available.

Remove explicitly aligning members by the type because that was previously
reserved for members with explicit alignment. AlignmentCalc will correctly
align a member to the underlying type without this. Explicit member alignment
is still missing, as before this change.

Test plan:
- CI
- Too little. Gets further into a production type than without this change.
---
 oi/type_graph/AlignmentCalc.cpp   | 30 ++++++++++++++++--------------
 oi/type_graph/ClangTypeParser.cpp |  4 ++--
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/oi/type_graph/AlignmentCalc.cpp b/oi/type_graph/AlignmentCalc.cpp
index cf23eff..9abad71 100644
--- a/oi/type_graph/AlignmentCalc.cpp
+++ b/oi/type_graph/AlignmentCalc.cpp
@@ -50,24 +50,26 @@ void AlignmentCalc::accept(Type& type) {
 void AlignmentCalc::visit(Class& c) {
   RecursiveVisitor::visit(c);
 
-  uint64_t alignment = 1;
-  for (auto& member : c.members) {
-    if (member.align == 0) {
-      // If the member does not have an explicit alignment, calculate it from
-      // the member's type.
-      accept(member.type());
-      member.align = member.type().align();
-    }
-    alignment = std::max(alignment, member.align);
+  if (c.align() == 0) {
+    uint64_t alignment = 1;
+    for (auto& member : c.members) {
+      if (member.align == 0) {
+        // If the member does not have an explicit alignment, calculate it from
+        // the member's type.
+        accept(member.type());
+        member.align = member.type().align();
+      }
+      alignment = std::max(alignment, member.align);
 
-    if (member.align != 0 && (member.bitOffset / 8) % member.align != 0) {
-      // Mark as packed if members are not aligned
-      c.setPacked();
+      if (member.align != 0 && (member.bitOffset / 8) % member.align != 0) {
+        // Mark as packed if members are not aligned
+        c.setPacked();
+      }
     }
+
+    c.setAlign(alignment);
   }
 
-  c.setAlign(alignment);
-
   if (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 40f3a69..5280aa1 100644
--- a/oi/type_graph/ClangTypeParser.cpp
+++ b/oi/type_graph/ClangTypeParser.cpp
@@ -178,7 +178,7 @@ Type& ClangTypeParser::enumerateClass(const clang::RecordType& ty) {
   if (auto* info = getContainerInfo(fqName)) {
     auto& c = makeType<Container>(ty, *info, size, nullptr);
     enumerateClassTemplateParams(ty, c.templateParams);
-    c.setAlign(ast->getTypeAlign(clang::QualType(&ty, 0)));
+    c.setAlign(ast->getTypeAlign(clang::QualType(&ty, 0)) / 8);
     return c;
   }
 
@@ -197,6 +197,7 @@ Type& ClangTypeParser::enumerateClass(const clang::RecordType& ty) {
 
   auto& c = makeType<Class>(
       ty, kind, std::move(name), std::move(fqName), size, virtuality);
+  c.setAlign(ast->getTypeAlign(clang::QualType(&ty, 0)) / 8);
 
   enumerateClassTemplateParams(ty, c.templateParams);
   // enumerateClassParents(type, c.parents);
@@ -317,7 +318,6 @@ 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 = decl->getASTContext().getTypeAlign(qualType) / 8;
     members.push_back(m);
   }