Fix -fsanitize=array-bounds with comma operator

[vitalybuka]

Summary

Depends on D77373.

[vitalybuka]

remove debug dump

[vitalybuka]

clang/test/CodeGen/bounds-checking.c

116

C version works even without patch

[vitalybuka]

try arc diff

[vitalybuka]

ping

[rsmith]

clang/lib/CodeGen/CGExpr.cpp

885–890

If we're going to further extend what Clang considers to be a flexible array access, we should do so consistently across our warning machinery and our sanitizers. Perhaps we could start by unifying this function with IsTailPaddedMemberArray in SemaChecking?

[vitalybuka]

clang/lib/CodeGen/CGExpr.cpp

885–890

There is one place in external code which is blocking me from enabling this at Google.

How much work it's going to be? To me these functions looks very different.

[rsmith]

clang/lib/CodeGen/CGExpr.cpp

885–890

If you don't want to do the refactoring, please at least update Sema::CheckArrayAccess to skip over commas when looking for a member access in BaseExpr. Testcase:

struct X { int a; int b[1]; } *p;
int n = (0, p->b)[3];

... currently warns and trips the array-bounds sanitizer; with this change it would still warn but not trip the sanitizer, which seems bad. (Though I suppose the opposite case is worse.)

clang/lib/CodeGen/CGExpr.cpp

if (Done) {
     Builder.CreateBr(Done);
     EmitBlock(Done);
   }
 }
 
 /// Determine whether this expression refers to a flexible array member in a
 /// struct. We disable array bounds checks for such members.
 static bool isFlexibleArrayMemberExpr(const Expr *E) {
+  // E->dump();
+
   // For compatibility with existing code, we treat arrays of length 0 or
   // 1 as flexible array members.
   // FIXME: This is inconsistent with the warning code in SemaChecking. Unify
   // the two mechanisms.
   const ArrayType *AT = E->getType()->castAsArrayTypeUnsafe();
   if (const auto *CAT = dyn_cast<ConstantArrayType>(AT)) {
     // FIXME: Sema doesn't treat [1] as a flexible array member if the bound
     // was produced by macro expansion.
     if (CAT->getSize().ugt(1))
       return false;
   } else if (!isa<IncompleteArrayType>(AT))
     return false;
 
+  E->dump();
   E = E->IgnoreParens();
 
+  while (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
+    if (!BO->isCommaOp())
+      break;
+    E = BO->getRHS();
+    E = E->IgnoreParens();
+  }
+
   // A flexible array member must be the last member in the class.
   if (const auto *ME = dyn_cast<MemberExpr>(E)) {
     // FIXME: If the base type of the member expr is not FD->getParent(),
     // this should not be treated as a flexible array member access.
     if (const auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
       // FIXME: Sema doesn't treat a T[1] union member as a flexible array
       // member, only a T[0] or T[] member gets that treatment.
       if (FD->getParent()->isUnion())

clang/test/CodeGen/bounds-checking.c

 
 // CHECK-LABEL: define {{.*}} @SFlex
 int SFlex(struct SFlex *s, int i) {
   // a and b are treated as flexible array members.
   // CHECK-NOT: @llvm.trap
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}

clang/test/CodeGen/bounds-checking.cpp

 
 // CHECK-LABEL: define {{.*}} @_Z5SFlex
 int SFlex(struct SFlex *s, int i) {
   // a and b are treated as flexible array members.
   // CHECK-NOT: @llvm.trap
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @_Z10SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @_Z7S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return ((s->t, (1, s->a)))[i];
+  // CHECK: }
+}