[Clang] Correct handling of negative and out-of-bounds indices (#71877)

GCC returns 0 for a negative index on an array in a structure. It also
returns 0 for an array index that goes beyond the extent of the array.
In addition. a pointer to a struct field returns that field's size, not
the size of it plus the rest of the struct, unless it's the first field
in the struct.

  struct s {
    int count;
    char dummy;
    int array[] __attribute((counted_by(count)));
  };

  struct s *p = malloc(...);

  p->count = 10;

A __bdos on the elements of p return:

  __bdos(p, 0) == 30
  __bdos(p->array, 0) == 10
  __bdos(&p->array[0], 0) == 10
  __bdos(&p->array[-1], 0) == 0
  __bdos(&p->array[42], 0) == 0

Also perform some refactoring, putting the "counted_by" calculations in
their own function.

Author: bwendling

clang/lib/CodeGen/CGBuiltin.cpp

@@ -851,78 +851,94 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,
   //        max(sizeof(struct s),
   //            offsetof(struct s, array) + p->count * sizeof(*p->array))
   //
+  ASTContext &Ctx = getContext();
   const Expr *Base = E->IgnoreParenImpCasts();
   const Expr *Idx = nullptr;
 
   if (const auto *UO = dyn_cast<UnaryOperator>(Base);
       UO && UO->getOpcode() == UO_AddrOf) {
-    if (const auto *ASE =
-            dyn_cast<ArraySubscriptExpr>(UO->getSubExpr()->IgnoreParens())) {
-      Base = ASE->getBase();
+    Expr *SubExpr = UO->getSubExpr()->IgnoreParenImpCasts();
+    if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(SubExpr)) {
+      Base = ASE->getBase()->IgnoreParenImpCasts();
       Idx = ASE->getIdx()->IgnoreParenImpCasts();
 
-      if (const auto *IL = dyn_cast<IntegerLiteral>(Idx);
-          IL && !IL->getValue().getSExtValue())
-        Idx = nullptr;
+      if (const auto *IL = dyn_cast<IntegerLiteral>(Idx)) {
+        int64_t Val = IL->getValue().getSExtValue();
+        if (Val < 0)
+          // __bdos returns 0 for negative indexes into an array in a struct.
+          return getDefaultBuiltinObjectSizeResult(Type, ResType);
+
+        if (Val == 0)
+          // The index is 0, so we don't need to take it into account.
+          Idx = nullptr;
+      }
+    } else {
+      // Potential pointer to another element in the struct.
+      Base = SubExpr;
     }
   }
 
+  // Get the flexible array member Decl.
+  const ValueDecl *FAMDecl = nullptr;
+  if (const auto *ME = dyn_cast<MemberExpr>(Base)) {
+    // Check if \p Base is referencing the FAM itself.
+    if (const ValueDecl *MD = ME->getMemberDecl()) {
+      const LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
+          getLangOpts().getStrictFlexArraysLevel();
+      if (!Decl::isFlexibleArrayMemberLike(
+              Ctx, MD, MD->getType(), StrictFlexArraysLevel,
+              /*IgnoreTemplateOrMacroSubstitution=*/true))
+        return nullptr;
+
+      FAMDecl = MD;
+    }
+  } else if (const auto *DRE = dyn_cast<DeclRefExpr>(Base)) {
+    // Check if we're pointing to the whole struct.
+    QualType Ty = DRE->getDecl()->getType();
+    if (Ty->isPointerType())
+      Ty = Ty->getPointeeType();
+
+    if (const auto *RD = Ty->getAsRecordDecl())
+      // Don't use the outer lexical record because the FAM might be in a
+      // different RecordDecl.
+      FAMDecl = FindFlexibleArrayMemberField(Ctx, RD);
+  }
+
+  if (!FAMDecl || !FAMDecl->hasAttr<CountedByAttr>())
+    // No flexible array member found or it doesn't have the "counted_by"
+    // attribute.
+    return nullptr;
+
   const ValueDecl *CountedByFD = FindCountedByField(Base);
   if (!CountedByFD)
     // Can't find the field referenced by the "counted_by" attribute.
     return nullptr;
 
   // Build a load of the counted_by field.
   bool IsSigned = CountedByFD->getType()->isSignedIntegerType();
-  const RecordDecl *OuterRD =
-      CountedByFD->getDeclContext()->getOuterLexicalRecordContext();
-  ASTContext &Ctx = getContext();
-
-  // Load the counted_by field.
   const Expr *CountedByExpr = BuildCountedByFieldExpr(Base, CountedByFD);
   Value *CountedByInst = EmitAnyExprToTemp(CountedByExpr).getScalarVal();
   llvm::Type *CountedByTy = CountedByInst->getType();
 
+  // Build a load of the index and subtract it from the count.
+  Value *IdxInst = nullptr;
   if (Idx) {
-    // There's an index into the array. Remove it from the count.
     bool IdxSigned = Idx->getType()->isSignedIntegerType();
-    Value *IdxInst = EmitAnyExprToTemp(Idx).getScalarVal();
+    IdxInst = EmitAnyExprToTemp(Idx).getScalarVal();
     IdxInst = IdxSigned ? Builder.CreateSExtOrTrunc(IdxInst, CountedByTy)
                         : Builder.CreateZExtOrTrunc(IdxInst, CountedByTy);
 
-    // If the index is negative, don't subtract it from the counted_by
-    // value. The pointer is pointing to something before the FAM.
-    IdxInst = Builder.CreateNeg(IdxInst, "", !IdxSigned, IdxSigned);
+    // We go ahead with the calculation here. If the index turns out to be
+    // negative, we'll catch it at the end.
     CountedByInst =
-        Builder.CreateAdd(CountedByInst, IdxInst, "", !IsSigned, IsSigned);
+        Builder.CreateSub(CountedByInst, IdxInst, "", !IsSigned, IsSigned);
   }
 
-  // Get the size of the flexible array member's base type.
-  const ValueDecl *FAMDecl = nullptr;
-  if (const auto *ME = dyn_cast<MemberExpr>(Base)) {
-    const LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
-        getLangOpts().getStrictFlexArraysLevel();
-    if (const ValueDecl *MD = ME->getMemberDecl()) {
-      if (!Decl::isFlexibleArrayMemberLike(
-              Ctx, MD, MD->getType(), StrictFlexArraysLevel,
-              /*IgnoreTemplateOrMacroSubstitution=*/true))
-        return nullptr;
-      // Base is referencing the FAM itself.
-      FAMDecl = MD;
-    }
-  }
-
-  if (!FAMDecl)
-    FAMDecl = FindFlexibleArrayMemberField(Ctx, OuterRD);
-
-  assert(FAMDecl && "Can't find the flexible array member field");
-
+  // Calculate how large the flexible array member is in bytes.
   const ArrayType *ArrayTy = Ctx.getAsArrayType(FAMDecl->getType());
   CharUnits Size = Ctx.getTypeSizeInChars(ArrayTy->getElementType());
   llvm::Constant *ElemSize =
       llvm::ConstantInt::get(CountedByTy, Size.getQuantity(), IsSigned);
-
-  // Calculate how large the flexible array member is in bytes.
   Value *FAMSize =
       Builder.CreateMul(CountedByInst, ElemSize, "", !IsSigned, IsSigned);
   FAMSize = IsSigned ? Builder.CreateSExtOrTrunc(FAMSize, ResType)
@@ -931,47 +947,38 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,
 
   if (const auto *DRE = dyn_cast<DeclRefExpr>(Base)) {
     // The whole struct is specificed in the __bdos.
+    const RecordDecl *OuterRD =
+        CountedByFD->getDeclContext()->getOuterLexicalRecordContext();
     const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(OuterRD);
 
     // Get the offset of the FAM.
     CharUnits Offset = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FAMDecl));
     llvm::Constant *FAMOffset =
         ConstantInt::get(ResType, Offset.getQuantity(), IsSigned);
-
-    // max(sizeof(struct s),
-    //     offsetof(struct s, array) + p->count * sizeof(*p->array))
     Value *OffsetAndFAMSize =
         Builder.CreateAdd(FAMOffset, Res, "", !IsSigned, IsSigned);
 
     // Get the full size of the struct.
     llvm::Constant *SizeofStruct =
         ConstantInt::get(ResType, Layout.getSize().getQuantity(), IsSigned);
 
+    // max(sizeof(struct s),
+    //     offsetof(struct s, array) + p->count * sizeof(*p->array))
     Res = IsSigned
               ? Builder.CreateBinaryIntrinsic(llvm::Intrinsic::smax,
                                               OffsetAndFAMSize, SizeofStruct)
               : Builder.CreateBinaryIntrinsic(llvm::Intrinsic::umax,
                                               OffsetAndFAMSize, SizeofStruct);
-  } else if (const auto *ME = dyn_cast<MemberExpr>(Base)) {
-    // Pointing to a place before the FAM. Add the difference to the FAM's
-    // size.
-    if (const ValueDecl *MD = ME->getMemberDecl(); MD != FAMDecl) {
-      CharUnits Offset = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(MD));
-      CharUnits FAMOffset =
-          Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FAMDecl));
-
-      Res = Builder.CreateAdd(
-          Res, ConstantInt::get(ResType, FAMOffset.getQuantity() -
-                                             Offset.getQuantity()));
-    }
   }
 
-  // A negative 'FAMSize' means that the index was greater than the count,
-  // or an improperly set count field. Return -1 (for types 0 and 1) or 0
-  // (for types 2 and 3).
-  return Builder.CreateSelect(Builder.CreateIsNeg(FAMSize),
-                              getDefaultBuiltinObjectSizeResult(Type, ResType),
-                              Res);
+  // A negative \p IdxInst or \p CountedByInst means that the index lands
+  // outside of the flexible array member. If that's the case, we want to
+  // return 0.
+  Value *Cmp = Builder.CreateIsNotNeg(CountedByInst);
+  if (IdxInst)
+    Cmp = Builder.CreateAnd(Builder.CreateIsNotNeg(IdxInst), Cmp);
+
+  return Builder.CreateSelect(Cmp, Res, ConstantInt::get(ResType, 0, IsSigned));
 }
 
 /// Returns a Value corresponding to the size of the given expression.
@@ -1006,19 +1013,19 @@ CodeGenFunction::emitBuiltinObjectSize(const Expr *E, unsigned Type,
     }
   }
 
-  // LLVM can't handle Type=3 appropriately, and __builtin_object_size shouldn't
-  // evaluate E for side-effects. In either case, we shouldn't lower to
-  // @llvm.objectsize.
-  if (Type == 3 || (!EmittedE && E->HasSideEffects(getContext())))
-    return getDefaultBuiltinObjectSizeResult(Type, ResType);
-
   if (IsDynamic) {
     // Emit special code for a flexible array member with the "counted_by"
     // attribute.
     if (Value *V = emitFlexibleArrayMemberSize(E, Type, ResType))
       return V;
   }
 
+  // LLVM can't handle Type=3 appropriately, and __builtin_object_size shouldn't
+  // evaluate E for side-effects. In either case, we shouldn't lower to
+  // @llvm.objectsize.
+  if (Type == 3 || (!EmittedE && E->HasSideEffects(getContext())))
+    return getDefaultBuiltinObjectSizeResult(Type, ResType);
+
   Value *Ptr = EmittedE ? EmittedE : EmitScalarExpr(E);
   assert(Ptr->getType()->isPointerTy() &&
          "Non-pointer passed to __builtin_object_size?");