Fix a bug in classification of structs with field holes.

There is a bug in classifying the register passing structs where field
layout leaves holes in the struct layout.

Author: Lubomir Litchev

src/vm/methodtable.cpp

@@ -2509,121 +2509,7 @@ bool MethodTable::ClassifyEightBytes(SystemVStructRegisterPassingHelperPtr helpe
         _ASSERTE(helperPtr->currentUniqueOffsetField < SYSTEMV_MAX_NUM_FIELDS_IN_REGISTER_PASSED_STRUCT);
     } // end per-field for loop
 
-    if (!helperPtr->inEmbeddedStruct)
-    {
-        _ASSERTE(nestingLevel == 0);
-
-        // We're at the top level of the recursion, and we're done looking at the fields.
-        // Now sort the fields by offset and set the output data.
-
-        int sortedFieldOrder[SYSTEMV_MAX_NUM_FIELDS_IN_REGISTER_PASSED_STRUCT];
-        for (unsigned i = 0; i < SYSTEMV_MAX_NUM_FIELDS_IN_REGISTER_PASSED_STRUCT; i++)
-        {
-            sortedFieldOrder[i] = -1;
-        }
-
-        for (unsigned i = 0; i < helperPtr->currentUniqueOffsetField; i++)
-        {
-            _ASSERTE(helperPtr->fieldOffsets[i] < SYSTEMV_MAX_NUM_FIELDS_IN_REGISTER_PASSED_STRUCT);
-            _ASSERTE(sortedFieldOrder[helperPtr->fieldOffsets[i]] == -1); // we haven't seen this field offset yet.
-            sortedFieldOrder[helperPtr->fieldOffsets[i]] = i;
-        }
-
-        // Set the layoutSizes (includes holes from alignment of the fields.)
-        int lastField = -1;
-        for (unsigned i = 0; i < SYSTEMV_MAX_NUM_FIELDS_IN_REGISTER_PASSED_STRUCT; i++)
-        {
-            int ordinal = sortedFieldOrder[i];
-            if (ordinal == -1)
-            {
-                continue;
-            }
-
-            if (lastField == -1)
-            {
-                lastField = ordinal;
-                continue;
-            }
-
-            helperPtr->fieldLayoutSizes[lastField] = helperPtr->fieldOffsets[ordinal] - helperPtr->fieldOffsets[lastField];
-
-            lastField = ordinal;
-        }
-        // Now the last field
-        _ASSERTE(lastField != -1); // if lastField==-1, then the struct has no fields!
-        helperPtr->fieldLayoutSizes[lastField] = helperPtr->structSize - helperPtr->fieldOffsets[lastField];
-
-        // Calculate the eightbytes and their types.
-        unsigned int accumulatedSizeForEightByte = 0;
-        unsigned int lastEightByteOffset = 0;
-        unsigned int currentEightByte = 0;
-
-        for (unsigned i = 0; i < SYSTEMV_MAX_NUM_FIELDS_IN_REGISTER_PASSED_STRUCT; i++)
-        {
-            int ordinal = sortedFieldOrder[i];
-            if (ordinal == -1)
-            {
-                continue;
-            }
-
-            if ((accumulatedSizeForEightByte + helperPtr->fieldLayoutSizes[ordinal]) > SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES)
-            {
-                // Save data for this eightbyte.
-                helperPtr->eightByteSizes[currentEightByte] = accumulatedSizeForEightByte;
-                helperPtr->eightByteOffsets[currentEightByte] = lastEightByteOffset;
-
-                // Set up for next eightbyte.
-                currentEightByte++;
-                _ASSERTE(currentEightByte < CLR_SYSTEMV_MAX_EIGHTBYTES_COUNT_TO_PASS_IN_REGISTERS);
-
-                lastEightByteOffset = helperPtr->fieldOffsets[ordinal];
-                accumulatedSizeForEightByte = 0;
-            }
-
-            accumulatedSizeForEightByte += helperPtr->fieldLayoutSizes[ordinal];
-
-            _ASSERTE(helperPtr->fieldClassifications[ordinal] != SystemVClassificationTypeMemory);
-
-            if (helperPtr->eightByteClassifications[currentEightByte] == helperPtr->fieldClassifications[ordinal])
-            {
-                // Do nothing. The eight-byte is already classified.
-            }
-            else if (helperPtr->eightByteClassifications[currentEightByte] == SystemVClassificationTypeNoClass)
-            {
-                helperPtr->eightByteClassifications[currentEightByte] = helperPtr->fieldClassifications[ordinal];
-            }
-            else if ((helperPtr->eightByteClassifications[currentEightByte] == SystemVClassificationTypeInteger) ||
-                     (helperPtr->fieldClassifications[ordinal] == SystemVClassificationTypeInteger))
-            {
-                _ASSERTE(helperPtr->fieldClassifications[ordinal] != SystemVClassificationTypeIntegerReference);
-                helperPtr->eightByteClassifications[currentEightByte] = SystemVClassificationTypeInteger;
-            }
-            else if ((helperPtr->eightByteClassifications[currentEightByte] == SystemVClassificationTypeIntegerReference) ||
-                     (helperPtr->fieldClassifications[ordinal] == SystemVClassificationTypeIntegerReference))
-            {
-                helperPtr->eightByteClassifications[currentEightByte] = SystemVClassificationTypeIntegerReference;
-            }
-            else
-            {
-                helperPtr->eightByteClassifications[currentEightByte] = SystemVClassificationTypeSSE;
-            }
-        }
-
-        helperPtr->eightByteCount = currentEightByte + 1; 
-        helperPtr->eightByteSizes[currentEightByte] = accumulatedSizeForEightByte;
-        helperPtr->eightByteOffsets[currentEightByte] = lastEightByteOffset;
-        _ASSERTE(helperPtr->eightByteCount <= CLR_SYSTEMV_MAX_EIGHTBYTES_COUNT_TO_PASS_IN_REGISTERS);
-
-#ifdef _DEBUG
-        LOG((LF_JIT, LL_EVERYTHING, "     ----\n"));
-        LOG((LF_JIT, LL_EVERYTHING, "     **** Number EightBytes: %d\n", helperPtr->eightByteCount));
-        for (unsigned i = 0; i < helperPtr->eightByteCount; i++)
-        {
-            LOG((LF_JIT, LL_EVERYTHING, "     **** eightByte %d -- classType: %s, eightByteOffset: %d, eightByteSize: %d\n",
-                i, GetSystemVClassificationTypeName(helperPtr->eightByteClassifications[i]), helperPtr->eightByteOffsets[i], helperPtr->eightByteSizes[i]));
-        }
-#endif // _DEBUG
-    }
+    AssignClassifiedEightByteTypes(helperPtr, nestingLevel);
 
     return true;
 }
@@ -3066,6 +2952,15 @@ bool MethodTable::ClassifyEightBytesForNativeStruct(SystemVStructRegisterPassing
 
     } // end per-field for loop
 
+    AssignClassifiedEightByteTypes(helperPtr, nestingLevel);
+
+    return true;
+#endif // DACCESS_COMPILE
+}
+
+// Assigns the classification types to the array with eightbyte types.
+void  MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHelperPtr helperPtr, unsigned int nestingLevel)
+{
     if (!helperPtr->inEmbeddedStruct)
     {
         _ASSERTE(nestingLevel == 0);
@@ -3108,7 +3003,14 @@ bool MethodTable::ClassifyEightBytesForNativeStruct(SystemVStructRegisterPassing
         }
         // Now the last field
         _ASSERTE(lastField != -1); // if lastField==-1, then the struct has no fields!
-        helperPtr->fieldLayoutSizes[lastField] = helperPtr->structSize - helperPtr->fieldOffsets[lastField];
+
+        // A field size cannot be bigger than the size of an eightbyte.
+        // There are cases where for the native layout of a struct the VM allocates 16 
+        // bytes space, but the struct has one eight-byte field.
+        // This field is of classification type INTEGER or INTEGERREFERENCE.
+        // Make sure the field layout size does not extend beyound an eightbyte.
+        _ASSERTE(helperPtr->fieldSizes[lastField] <= SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES);
+        helperPtr->fieldLayoutSizes[lastField] = min(helperPtr->structSize - helperPtr->fieldOffsets[lastField], SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES);
 
         // Calculate the eightbytes and their types.
         unsigned int accumulatedSizeForEightByte = 0;
@@ -3123,6 +3025,8 @@ bool MethodTable::ClassifyEightBytesForNativeStruct(SystemVStructRegisterPassing
                 continue;
             }
 
+            _ASSERTE(helperPtr->fieldLayoutSizes[ordinal] <= SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES);
+
             if ((accumulatedSizeForEightByte + helperPtr->fieldLayoutSizes[ordinal]) > SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES)
             {
                 // Save data for this eightbyte.
@@ -3134,10 +3038,12 @@ bool MethodTable::ClassifyEightBytesForNativeStruct(SystemVStructRegisterPassing
                 _ASSERTE(currentEightByte < CLR_SYSTEMV_MAX_EIGHTBYTES_COUNT_TO_PASS_IN_REGISTERS);
 
                 lastEightByteOffset = helperPtr->fieldOffsets[ordinal];
-                accumulatedSizeForEightByte = 0;
+                accumulatedSizeForEightByte = (accumulatedSizeForEightByte + helperPtr->fieldLayoutSizes[ordinal]) - SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES;
+            }
+            else
+            {
+                accumulatedSizeForEightByte += helperPtr->fieldLayoutSizes[ordinal];
             }
-
-            accumulatedSizeForEightByte += helperPtr->fieldLayoutSizes[ordinal];
 
             _ASSERTE(helperPtr->fieldClassifications[ordinal] != SystemVClassificationTypeMemory);
 
@@ -3181,9 +3087,6 @@ bool MethodTable::ClassifyEightBytesForNativeStruct(SystemVStructRegisterPassing
         }
 #endif // _DEBUG
     }
-
-    return true;
-#endif // DACCESS_COMPILE
 }
 
 #endif // defined(FEATURE_UNIX_AMD64_STRUCT_PASSING_ITF)

src/vm/methodtable.h

@@ -1054,7 +1054,6 @@ class MethodTable
     // Builds the internal data structures and classifies struct eightbytes for Amd System V calling convention.
     bool ClassifyEightBytes(SystemVStructRegisterPassingHelperPtr helperPtr, unsigned int nestingLevel, unsigned int startOffsetOfStruct);
     bool ClassifyEightBytesForNativeStruct(SystemVStructRegisterPassingHelperPtr helperPtr, unsigned int nestingLevel, unsigned int startOffsetOfStruct);
-    
 #endif // defined(FEATURE_UNIX_AMD64_STRUCT_PASSING_ITF)
 
     // Copy m_dwFlags from another method table
@@ -1092,6 +1091,10 @@ class MethodTable
 
 private:
 
+#if defined(FEATURE_UNIX_AMD64_STRUCT_PASSING_ITF)
+    void AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHelperPtr helperPtr, unsigned int nestingLevel);
+#endif // defined(FEATURE_UNIX_AMD64_STRUCT_PASSING_ITF)
+
     DWORD   GetClassIndexFromToken(mdTypeDef typeToken)
     {
         LIMITED_METHOD_CONTRACT;