Do not insert asan paddings after fields that have flexible arrays.

[kcc]

Summary

We should avoid a tail padding not only if the last field
has zero size but also if the last field is a struct with a flexible array.

If/when http://reviews.llvm.org/D5478 is committed,
this will also handle the case of structs with zero-sized arrays.

[majnemer]

Do you handle stuff like the following correctly?

struct S {
  int Array[1]; // This field is actually variable length.
};

I would recommend that you do not put padding after the last field if it has one or zero as the array bound.

[kcc]

Changed the patch so that it does not check the field size any more,
but only relies on hasFlexibleArrayMember().
Now the loop iterating over the fields actually knows which element is the last one.
This way we will avoid instrumenting flexible arrays and structs that contain those.

Combined with the David's patch http://reviews.llvm.org/D5478
this will automatically handle cases like
struct S {int Array[0]; }
and
struct S {int Array[1]; }
as well as the cases where S is the last field of another struct.

PTAL

[kcc]

ping?

[rsmith]

LGTM

lib/AST/RecordLayoutBuilder.cpp

1335

Remove this comment.

lib/AST/RecordLayoutBuilder.cpp

void RecordLayoutBuilder::Layout(const ObjCInterfaceDecl *D) {
   // struct itself.
   FinishLayout(D);
 }
 
 void RecordLayoutBuilder::LayoutFields(const RecordDecl *D) {
   // Layout each field, for now, just sequentially, respecting alignment.  In
   // the future, this will need to be tweakable by targets.
   bool InsertExtraPadding = D->mayInsertExtraPadding(/*EmitRemark=*/true);
-  for (const auto *Field : D->fields())
-    LayoutField(Field, InsertExtraPadding);
+  bool HasFlexibleArrayMember = D->hasFlexibleArrayMember();
+  // for (const auto *Field : D->fields())
+  for (auto I = D->field_begin(), End = D->field_end(); I != End; ++I) {
+    auto Next(I);
+    ++Next;
+    LayoutField(*I,
+                InsertExtraPadding && (Next != End || !HasFlexibleArrayMember));
+  }
 }
 
 // Rounds the specified size to have it a multiple of the char size.
 static uint64_t
 roundUpSizeToCharAlignment(uint64_t Size,
                            const ASTContext &Context) {
   uint64_t CharAlignment = Context.getTargetInfo().getCharAlign();
   return llvm::RoundUpToAlignment(Size, CharAlignment);
void RecordLayoutBuilder::LayoutField(const FieldDecl *D,
   // Place this field at the current location.
   FieldOffsets.push_back(Context.toBits(FieldOffset));
 
   if (!ExternalLayout)
     CheckFieldPadding(Context.toBits(FieldOffset), UnpaddedFieldOffset, 
                       Context.toBits(UnpackedFieldOffset),
                       Context.toBits(UnpackedFieldAlign), FieldPacked, D);
 
-  if (InsertExtraPadding && !FieldSize.isZero()) {
+  if (InsertExtraPadding) {
     CharUnits ASanAlignment = CharUnits::fromQuantity(8);
     CharUnits ExtraSizeForAsan = ASanAlignment;
     if (FieldSize % ASanAlignment)
       ExtraSizeForAsan +=
           ASanAlignment - CharUnits::fromQuantity(FieldSize % ASanAlignment);
     FieldSize += ExtraSizeForAsan;
   }
 

test/CodeGen/sanitize-address-field-padding.cpp

public:
   int make_it_non_standard_layout;
  private:
   char x[7];
   char y[9];
 };
 
 ClassWithVirtualBase class_with_virtual_base;
 
+class WithFlexibleArray1 {
+ public:
+  WithFlexibleArray1() {}
+  ~WithFlexibleArray1() {}
+  int make_it_non_standard_layout;
+ private:
+  char private1[33];
+  int flexible[];  // Don't insert padding after this field.
+};
+
+WithFlexibleArray1 with_flexible_array1;
+// CHECK: %class.WithFlexibleArray1 = type { i32, [12 x i8], [33 x i8], [15 x i8], [0 x i32] }
+
+class WithFlexibleArray2 {
+ public:
+  char x[21];
+  WithFlexibleArray1 flex1;  // Don't insert padding after this field.
+};
+
+WithFlexibleArray2 with_flexible_array2;
+// CHECK: %class.WithFlexibleArray2 = type { [21 x i8], [11 x i8], %class.WithFlexibleArray1 }
+
+class WithFlexibleArray3 {
+ public:
+  char x[13];
+  WithFlexibleArray2 flex2;  // Don't insert padding after this field.
+};
+
+WithFlexibleArray3 with_flexible_array3;
+
 
 class Negative1 {
  public:
   Negative1() {}
   int public1, public2;
 };
 Negative1 negative1;
 // CHECK: type { i32, i32 }