[llvm] 326a5a2 - Fix a bug in OptimizedStructLayout when filling gaps before

[John McCall via llvm-commits]

Author: John McCall
Date: 2021-07-21T15:47:18-04:00
New Revision: 326a5a2658d81db46a78b184fe42e522ef170f32

URL: https://github.com/llvm/llvm-project/commit/326a5a2658d81db46a78b184fe42e522ef170f32
DIFF: https://github.com/llvm/llvm-project/commit/326a5a2658d81db46a78b184fe42e522ef170f32.diff

LOG: Fix a bug in OptimizedStructLayout when filling gaps before
fixed fields with highly-aligned flexible fields.

The code was not considering the possibility that aligning
the current offset to the alignment of a queue might push
us past the end of the gap.  Subtracting the offsets to
figure out the maximum field size for the gap then overflowed,
making us think that we had nearly unbounded space to fill.

Fixes PR 51131.

Added: 
    

Modified: 
    llvm/lib/Support/OptimizedStructLayout.cpp
    llvm/unittests/Support/OptimizedStructLayoutTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Support/OptimizedStructLayout.cpp b/llvm/lib/Support/OptimizedStructLayout.cpp
index 9bbd767c5ce9b..19a93ed6776d2 100644
--- a/llvm/lib/Support/OptimizedStructLayout.cpp
+++ b/llvm/lib/Support/OptimizedStructLayout.cpp
@@ -350,6 +350,7 @@ llvm::performOptimizedStructLayout(MutableArrayRef<Field> Fields) {
                                    Optional<uint64_t> EndOffset) -> bool {
     assert(Queue->Head);
     assert(StartOffset == alignTo(LastEnd, Queue->Alignment));
+    assert(!EndOffset || StartOffset < *EndOffset);
 
     // Figure out the maximum size that a field can be, and ignore this
     // queue if there's nothing in it that small.
@@ -372,6 +373,7 @@ llvm::performOptimizedStructLayout(MutableArrayRef<Field> Fields) {
   // Helper function to find the "best" flexible-offset field according
   // to the criteria described above.
   auto tryAddBestField = [&](Optional<uint64_t> BeforeOffset) -> bool {
+    assert(!BeforeOffset || LastEnd < *BeforeOffset);
     auto QueueB = FlexibleFieldsByAlignment.begin();
     auto QueueE = FlexibleFieldsByAlignment.end();
 
@@ -403,9 +405,12 @@ llvm::performOptimizedStructLayout(MutableArrayRef<Field> Fields) {
         return false;
 
       // Otherwise, scan backwards to find the most-aligned queue that
-      // still has minimal leading padding after LastEnd.
+      // still has minimal leading padding after LastEnd.  If that
+      // minimal padding is already at or past the end point, we're done.
       --FirstQueueToSearch;
       Offset = alignTo(LastEnd, FirstQueueToSearch->Alignment);
+      if (BeforeOffset && Offset >= *BeforeOffset)
+        return false;
       while (FirstQueueToSearch != QueueB &&
              Offset == alignTo(LastEnd, FirstQueueToSearch[-1].Alignment))
         --FirstQueueToSearch;
@@ -415,6 +420,7 @@ llvm::performOptimizedStructLayout(MutableArrayRef<Field> Fields) {
   // Phase 1: fill the gaps between fixed-offset fields with the best
   // flexible-offset field that fits.
   for (auto I = Fields.begin(); I != FirstFlexible; ++I) {
+    assert(LastEnd <= I->Offset);
     while (LastEnd != I->Offset) {
       if (!tryAddBestField(I->Offset))
         break;

diff  --git a/llvm/unittests/Support/OptimizedStructLayoutTest.cpp b/llvm/unittests/Support/OptimizedStructLayoutTest.cpp
index 26d5e27b14b01..e8cd5f4285e52 100644
--- a/llvm/unittests/Support/OptimizedStructLayoutTest.cpp
+++ b/llvm/unittests/Support/OptimizedStructLayoutTest.cpp
@@ -129,4 +129,21 @@ TEST(OptimizedStructLayoutTest, GardenPath) {
     .flexible(2, 2, 42)
     .flexible(2, 2, 48)
     .verify(50, 4);
-}
\ No newline at end of file
+}
+
+// PR 51131
+TEST(OptimizedStructLayoutTest, HighAlignment) {
+  // Handle the case where a flexible field has such a high alignment
+  // requirement that aligning LastEnd to it gives an offset past the
+  // end of the gap before the next fixed-alignment field.
+  LayoutTest()
+    .fixed(8, 8, 0)
+    .fixed(8, 8, 8)
+    .fixed(64, 64, 64)
+    .flexible(1, 1, 16)
+    .flexible(1, 1, 17)
+    .flexible(4, 128, 128)
+    .flexible(1, 1, 18)
+    .flexible(1, 1, 19)
+    .verify(132, 128);
+}