From 7c44ee8e1937c7402a106f3fa6a356caa73a14e8 Mon Sep 17 00:00:00 2001
From: Eric Astor <epastor@google.com>
Date: Mon, 14 Sep 2020 14:11:29 -0400
Subject: [PATCH] [ms] [llvm-ml] Fix struct padding logic

MASM structs are end-padded to have size a multiple of the smaller of the requested alignment and the size of their largest field (taken recursively, if they have a field of STRUCT type).

This matches the behavior of ml.exe and ml64.exe. Our original implementation followed the MASM 6.0 documentation, which instead specified that MASM structs were padded to a multiple of their requested alignment.

Reviewed By: thakis

Differential Revision: https://reviews.llvm.org/D87248
---
 llvm/lib/MC/MCParser/MasmParser.cpp           | 22 +++++++++-----
 llvm/test/tools/llvm-ml/struct_alignment.test | 44 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 8 deletions(-)
 create mode 100644 llvm/test/tools/llvm-ml/struct_alignment.test

diff --git a/llvm/lib/MC/MCParser/MasmParser.cpp b/llvm/lib/MC/MCParser/MasmParser.cpp
index ea18cf8..c1917d7 100644
--- a/llvm/lib/MC/MCParser/MasmParser.cpp
+++ b/llvm/lib/MC/MCParser/MasmParser.cpp
@@ -124,10 +124,12 @@ struct StructInfo {
   bool IsUnion = false;
   size_t Alignment = 0;
   size_t Size = 0;
+  size_t AlignmentSize = 0;
   std::vector<FieldInfo> Fields;
   StringMap<size_t> FieldsByName;
 
-  FieldInfo &addField(StringRef FieldName, FieldType FT, size_t FieldSize);
+  FieldInfo &addField(StringRef FieldName, FieldType FT,
+                      size_t FieldAlignmentSize);
 
   StructInfo() = default;
 
@@ -331,7 +333,7 @@ struct FieldInfo {
 };
 
 FieldInfo &StructInfo::addField(StringRef FieldName, FieldType FT,
-                                size_t FieldSize) {
+                                size_t FieldAlignmentSize) {
   if (!FieldName.empty())
     FieldsByName[FieldName] = Fields.size();
   Fields.emplace_back(FT);
@@ -339,9 +341,10 @@ FieldInfo &StructInfo::addField(StringRef FieldName, FieldType FT,
   if (IsUnion) {
     Field.Offset = 0;
   } else {
-    Size = llvm::alignTo(Size, std::min(Alignment, FieldSize));
+    Size = llvm::alignTo(Size, std::min(Alignment, FieldAlignmentSize));
     Field.Offset = Size;
   }
+  AlignmentSize = std::max(AlignmentSize, FieldAlignmentSize);
   return Field;
 }
 
@@ -3973,7 +3976,8 @@ bool MasmParser::emitStructValues(const StructInfo &Structure) {
 // Declare a field in the current struct.
 bool MasmParser::addStructField(StringRef Name, const StructInfo &Structure) {
   StructInfo &OwningStruct = StructInProgress.back();
-  FieldInfo &Field = OwningStruct.addField(Name, FT_STRUCT, Structure.Size);
+  FieldInfo &Field =
+      OwningStruct.addField(Name, FT_STRUCT, Structure.AlignmentSize);
   StructFieldInfo &StructInfo = Field.Contents.StructInfo;
 
   StructInfo.Structure = Structure;
@@ -4101,8 +4105,10 @@ bool MasmParser::parseDirectiveEnds(StringRef Name, SMLoc NameLoc) {
     return Error(NameLoc, "mismatched name in ENDS directive; expected '" +
                               StructInProgress.back().Name + "'");
   StructInfo Structure = StructInProgress.pop_back_val();
-  // Pad to make the structure's size divisible by its alignment.
-  Structure.Size = llvm::alignTo(Structure.Size, Structure.Alignment);
+  // Pad to make the structure's size divisible by the smaller of its alignment
+  // and the size of its largest field.
+  Structure.Size = llvm::alignTo(
+      Structure.Size, std::min(Structure.Alignment, Structure.AlignmentSize));
   Structs[Name.lower()] = Structure;
 
   if (parseToken(AsmToken::EndOfStatement))
@@ -4147,8 +4153,8 @@ bool MasmParser::parseDirectiveNestedEnds() {
     else
       ParentStruct.Size += Structure.Size;
   } else {
-    FieldInfo &Field =
-        ParentStruct.addField(Structure.Name, FT_STRUCT, Structure.Size);
+    FieldInfo &Field = ParentStruct.addField(Structure.Name, FT_STRUCT,
+                                             Structure.AlignmentSize);
     StructFieldInfo &StructInfo = Field.Contents.StructInfo;
     Field.Type = Structure.Size;
     Field.LengthOf = 1;
diff --git a/llvm/test/tools/llvm-ml/struct_alignment.test b/llvm/test/tools/llvm-ml/struct_alignment.test
new file mode 100644
index 0000000..cfe8038
--- /dev/null
+++ b/llvm/test/tools/llvm-ml/struct_alignment.test
@@ -0,0 +1,44 @@
+; RUN: llvm-ml -filetype=asm %s | FileCheck %s
+
+.data
+
+FOO STRUCT 8
+  f FWORD -1
+FOO ENDS
+
+t1 FOO <>
+; CHECK-LABEL: t1:
+; CHECK-NEXT: .long 4294967295
+; CHECK-NEXT: .short 65535
+; CHECK-NOT: .zero
+
+BAZ STRUCT
+  b BYTE 3 DUP (-1)
+  f FWORD -1
+BAZ ENDS
+
+FOOBAR STRUCT 8
+  f1 BAZ <>
+  f2 BAZ <>
+  h BYTE -1
+FOOBAR ENDS
+
+t2 FOOBAR <>
+; CHECK-LABEL: t2:
+; CHECK-NEXT: .byte -1
+; CHECK-NEXT: .byte -1
+; CHECK-NEXT: .byte -1
+; CHECK-NEXT: .long 4294967295
+; CHECK-NEXT: .short 65535
+; CHECK-NEXT: .zero 3
+; CHECK-NEXT: .byte -1
+; CHECK-NEXT: .byte -1
+; CHECK-NEXT: .byte -1
+; CHECK-NEXT: .long 4294967295
+; CHECK-NEXT: .short 65535
+; CHECK-NEXT: .byte -1
+; CHECK-NEXT: .zero 2
+
+.code
+
+END
-- 
2.7.4