Conservatively set no_preserve_tags for MemberExprs

arichardson · arichardson · commit b35a6cb2475b · 2022-10-06T21:56:25.000Z
If we have an expression such as __builtin_memcpy(buf, &amp;s.not_a_cap, len);
and we can see the declaration for s and the entire VarDecl for s does not
contain tags and len does not extend beyond the end of s, then we can
set no_preserve_tags.
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
@@ -982,7 +982,7 @@ namespace {
 
       // We can pass EffectiveTypeKnown=true since this a C++ field copy.
       auto PreserveTags = CGF.getTypes().copyShouldPreserveTagsForPointee(
-          RecordTy, /*EffectiveTypeKnown=*/true, MemcpySize);
+          RecordTy, /*EffectiveTypeKnown=*/true, MemcpySize, FirstByteOffset);
       emitMemcpyIR(
           Dest.isBitField() ? Dest.getBitFieldAddress() : Dest.getAddress(CGF),
           Src.isBitField() ? Src.getBitFieldAddress() : Src.getAddress(CGF),
diff --git a/clang/lib/CodeGen/CodeGenTypes.cpp b/clang/lib/CodeGen/CodeGenTypes.cpp
@@ -959,11 +959,13 @@ static bool isLessThanCapSize(const ASTContext &Context,
 }
 
 static bool copiesAtMostTypeSize(const QualType Ty, const ASTContext &Context,
-                                 Optional<CharUnits> Size) {
+                                 Optional<CharUnits> Size,
+                                 uint64_t BitOffset = 0) {
   if (!Size)
     return false;
-  auto TypeSize = Context.getTypeSizeInCharsIfKnown(Ty);
-  return TypeSize && Size <= TypeSize;
+  auto TypeSize = Context.getTypeSize(Ty);
+  assert(BitOffset <= TypeSize);
+  return (uint64_t)Context.toBits(*Size) <= TypeSize - BitOffset;
 }
 
 llvm::PreserveCheriTags
@@ -1004,58 +1006,127 @@ CodeGenTypes::copyShouldPreserveTags(const Expr *DestPtr, const Expr *SrcPtr,
   return DstPreserve;
 }
 
-static const VarDecl *findUnderlyingVarDecl(const Expr *E) {
-  // Note: this is pretty similar to E->getReferencedDeclOfCallee(); and should
-  // possibly be moved to Expr.cpp
+struct UnderlyingVarDeclInfo {
+  const ValueDecl *MemberDecl = nullptr;
+  QualType CopiedType;
+  uint64_t CopiedTypeOffset = 0;
+  bool ConservativeApproxmation = false;
+  bool CanAffectOtherTypes = false;
+};
+
+static const ValueDecl *
+findUnderlyingVarDeclForCopy(const ASTContext &Context, const Expr *E,
+                             UnderlyingVarDeclInfo &Info,
+                             Optional<CharUnits> Size) {
   const Expr *UnderlyingExpr = E->IgnoreParenImpCasts();
   if (auto *UO = dyn_cast<UnaryOperator>(UnderlyingExpr)) {
+    // NB: We only look through AddrOf here to ensure that we don't classify
+    // expressions such as `*(&ptr)` as having a known underlying type (it could
+    // still be anything as that expression is equivalent to just `ptr`).
     if (UO->getOpcode() == UO_AddrOf) {
-      return findUnderlyingVarDecl(UO->getSubExpr());
+      return findUnderlyingVarDeclForCopy(Context, UO->getSubExpr(), Info,
+                                          Size);
     }
-  } else if (auto DRE = dyn_cast<DeclRefExpr>(UnderlyingExpr)) {
-    return dyn_cast<const VarDecl>(DRE->getDecl());
+  } else if (auto *DRE = dyn_cast<DeclRefExpr>(UnderlyingExpr)) {
+    return DRE->getDecl();
+  } else if (auto *ME = dyn_cast<MemberExpr>(UnderlyingExpr)) {
+    Info.MemberDecl = ME->getMemberDecl();
+    UnderlyingVarDeclInfo IgnoredInfo; // Don't update `Info` when recursing.
+    // If the copy size is up to sizeof(member), we can precisely determine the
+    // type and do not have to be conservative.
+    if (copiesAtMostTypeSize(Info.MemberDecl->getType(), Context, Size)) {
+      Info.CopiedType = Info.MemberDecl->getType();
+      return findUnderlyingVarDeclForCopy(Context, ME->getBase(), IgnoredInfo,
+                                          Size);
+    }
+    // For address-of member expressions where the size is not known to be just
+    // that member (or a subset thereof), we must conservatively assume that
+    // the copy extends across all fields of the struct (and possibly
+    // following instances of that struct for arrays and/or containing structs).
+    Info.CanAffectOtherTypes = true;
+    Info.ConservativeApproxmation = true;
+    // There is one optimization we can make though: If copy size - field offset
+    // is up to the size of the containing struct/class we can use that struct
+    // as a precise approximation.
+    // XXX: we could only look at the following fields, but that would make
+    // the analysis rather complicated and is unlikely to have large benefits.
+    if (auto *FD = dyn_cast<FieldDecl>(Info.MemberDecl)) {
+      auto ParentType = QualType(FD->getParent()->getTypeForDecl(), 0);
+      auto FieldOffset = Context.getFieldOffset(FD);
+      if (copiesAtMostTypeSize(ParentType, Context, Size, FieldOffset)) {
+        Info.CopiedType = ParentType;
+        Info.CopiedTypeOffset = FieldOffset;
+        Info.CanAffectOtherTypes = false;
+      }
+    }
+    return findUnderlyingVarDeclForCopy(Context, ME->getBase(), IgnoredInfo,
+                                        Size);
   }
-  // TODO: We could improve analysis for MemberExpr, but only if the copy size
-  //  is <= the size of the member, since memcpy() accross multiple fields is
-  //  a something that exists (despite not being compatible with sub-object
-  //  bounds). For now we just look at the declaration of the entire struct
-  return nullptr;
+  Info.CanAffectOtherTypes = true;
+  return {};
 }
 
 llvm::PreserveCheriTags
 CodeGenTypes::copyShouldPreserveTags(const Expr *E, Optional<CharUnits> Size) {
   assert(E->getType()->isAnyPointerType());
+  assert(!isLessThanCapSize(Context, Size) &&
+         "Should not call this function for small copies!");
   // Ignore the implicit cast to void* for the memcpy call.
   // Note: IgnoreParenImpCasts() might strip function/array-to-pointer decay
   // so we can't always call getPointeeType().
-  QualType Ty = E->IgnoreParenImpCasts()->getType();
-  if (Ty->isAnyPointerType())
-    Ty = Ty->getPointeeType();
+  QualType ExprTy = E->IgnoreParenImpCasts()->getType();
+  QualType PointeeTy =
+      ExprTy->isAnyPointerType() ? ExprTy->getPointeeType() : ExprTy;
+  QualType CheckTy = PointeeTy;
   bool EffectiveTypeKnown = false;
-  const VarDecl *UnderlyingVar = findUnderlyingVarDecl(E);
-  if (UnderlyingVar) {
-    QualType VarTy = UnderlyingVar->getType();
-    assert(!VarTy->isIncompleteType() && "Unexpected incomplete type");
-    if (VarTy->isReferenceType()) {
+  UnderlyingVarDeclInfo UnderlyingInfo;
+  const ValueDecl *UnderlyingDecl =
+      findUnderlyingVarDeclForCopy(Context, E, UnderlyingInfo, Size);
+  if (UnderlyingDecl) {
+    auto UnderlyingTy = UnderlyingDecl->getType();
+    assert(!UnderlyingTy->isIncompleteType() && "Unexpected incomplete type");
+    if (UnderlyingTy->isReferenceType()) {
       // If the variable declaration is a C++ reference we can assume that the
       // effective type of the object matches the type of the reference since
       // forming the reference would have been invalid otherwise.
-      Ty = VarTy->getPointeeType();
+      CheckTy = UnderlyingTy->getPointeeType();
       EffectiveTypeKnown = true;
-    } else if (!VarTy->isAnyPointerType()) {
-      // If we found a non-pointer declaration that we are copying to/from, use
+    } else if (!UnderlyingTy->isAnyPointerType()) {
+      // If we found a non-pointer declaration (e.g. address-of a variable), use
       // the type of the declaration for the analysis since that defines the
       // effective type. For pointers we can't assume anything since they could
       // be "allocated objects" without a declared type.
-      Ty = VarTy;
-      EffectiveTypeKnown = true;
+      CheckTy = UnderlyingTy;
+      EffectiveTypeKnown = !UnderlyingInfo.CanAffectOtherTypes;
     }
+    // If we were able to precisely/conservatively determine the copied type
+    // use that instead of the VarDecl type for the tag-preservation checks.
+    if (!UnderlyingInfo.CopiedType.isNull())
+      CheckTy = UnderlyingInfo.CopiedType;
+  }
+  llvm::PreserveCheriTags Result = copyShouldPreserveTagsForPointee(
+      CheckTy, EffectiveTypeKnown, Size, UnderlyingInfo.CopiedTypeOffset);
+  if (UnderlyingInfo.ConservativeApproxmation &&
+      Result == llvm::PreserveCheriTags::Required) {
+    // If we had a MemberExpr and were looking at the entire containing struct
+    // rather than just the single field, we only add the must_preserve_tags
+    // attribute if the pointee type is actually a capability type. This avoids
+    // lots of unnecessary warnings and still lets backend/middle-end decide
+    // how to handle the copy.
+    // TODO: this can be removed once we no longer add must_preserve_cheri_tags.
+    assert(UnderlyingInfo.MemberDecl != nullptr);
+    if (copyShouldPreserveTagsForPointee(UnderlyingInfo.MemberDecl->getType(),
+                                         /*EffectiveTypeKnown=*/true, Size,
+                                         0) !=
+        llvm::PreserveCheriTags::Required)
+      Result = llvm::PreserveCheriTags::Unknown;
   }
-  return copyShouldPreserveTagsForPointee(Ty, EffectiveTypeKnown, Size);
+  return Result;
 }
 
 llvm::PreserveCheriTags CodeGenTypes::copyShouldPreserveTagsForPointee(
-    QualType Pointee, bool EffectiveTypeKnown, Optional<CharUnits> Size) {
+    QualType Pointee, bool EffectiveTypeKnown, Optional<CharUnits> Size,
+    uint64_t CopyOffsetInBits) {
   // Don't add the no_preserve_tags/must_preserve_tags attribute for non-CHERI
   // targets to avoid changing tests and to avoid compile-time impact.
   if (!Context.getTargetInfo().SupportsCapabilities())
@@ -1102,7 +1173,7 @@ llvm::PreserveCheriTags CodeGenTypes::copyShouldPreserveTagsForPointee(
     // another structure and the copy could affect adjacent capability data.s
     // If the copy size is <= sizeof(T), we can still mark copies as
     // non-tag-preserving since it cannot affect subclass/adjacent data.
-    if (!copiesAtMostTypeSize(Pointee, Context, Size))
+    if (!copiesAtMostTypeSize(Pointee, Context, Size, CopyOffsetInBits))
       return llvm::PreserveCheriTags::Unknown;
     // structures without fields could be used as an opaque type -> assume it
     // might contain capabilities
@@ -1120,7 +1191,7 @@ llvm::PreserveCheriTags CodeGenTypes::copyShouldPreserveTagsForPointee(
     // having to call the library function.
     return llvm::PreserveCheriTags::Unnecessary;
   } else if (Context.cannotContainCapabilities(Pointee) &&
-             copiesAtMostTypeSize(Pointee, Context, Size)) {
+             copiesAtMostTypeSize(Pointee, Context, Size, CopyOffsetInBits)) {
     // If the type cannot contain capabilities and we are copying at most
     // sizeof(type), then we can use a non-tag-preserving copy.
     return llvm::PreserveCheriTags::Unnecessary;
diff --git a/clang/lib/CodeGen/CodeGenTypes.h b/clang/lib/CodeGen/CodeGenTypes.h
@@ -324,12 +324,15 @@ class CodeGenTypes {
   /// pointee type rather than the type of the buffer pointer.
   llvm::PreserveCheriTags
   copyShouldPreserveTagsForPointee(QualType CopyTy, bool EffectiveTypeKnown,
-                                   Optional<CharUnits> Size);
+                                   Optional<CharUnits> Size,
+                                   uint64_t CopyOffsetInBits);
   llvm::PreserveCheriTags
   copyShouldPreserveTagsForPointee(QualType CopyTy, bool EffectiveTypeKnown,
-                                   const llvm::Value *Size) {
+                                   const llvm::Value *Size,
+                                   uint64_t CopyOffsetInBits = 0) {
     return copyShouldPreserveTagsForPointee(CopyTy, EffectiveTypeKnown,
-                                            copySizeInCharUnits(Size));
+                                            copySizeInCharUnits(Size),
+                                            CopyOffsetInBits);
   }
 
   bool isRecordLayoutComplete(const Type *Ty) const;
diff --git a/clang/test/CodeGenCXX/cheri/no-tag-copy-member-expr.cpp b/clang/test/CodeGenCXX/cheri/no-tag-copy-member-expr.cpp
@@ -43,26 +43,25 @@ void test_member_expr_byval(void *buf, struct TestWithCap t, struct TestNoCap t2
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
   // CHECK-SAME: , i64 32, i1 false) [[MUST_PRESERVE_CHARPTR:#[0-9]+]]{{$}}
   // No attribute for the following four cases:
-  __builtin_memcpy(buf, &t.not_a_cap, 32);
+  __builtin_memcpy(buf, &t.not_a_cap, 64);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-SAME: , i64 32, i1 false){{$}}
-  __builtin_memcpy(buf, t.array, 32);
+  // CHECK-SAME: , i64 64, i1 false){{$}}
+  __builtin_memcpy(buf, t.array, 48);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-SAME: , i64 32, i1 false){{$}}
+  // CHECK-SAME: , i64 48, i1 false){{$}}
   __builtin_memcpy(buf, &t.n, 32);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
   // CHECK-SAME: , i64 32, i1 false){{$}}
-  __builtin_memcpy(buf, (&t)->array, 32);
+  __builtin_memcpy(buf, (&t)->array, 48);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-SAME: , i64 32, i1 false){{$}}
+  // CHECK-SAME: , i64 48, i1 false){{$}}
   __builtin_memcpy(buf, &(&t)->not_a_cap, 32);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
   // CHECK-SAME: , i64 32, i1 false){{$}}
 
   // However, for the struct without capabilities all of these should be safe:
   // However, we don't know here that there is no subclass that could have
-  // capabilities following the current object.
-  // TODO: If we know the size of the copy <= sizeof(T), we should set the attribute.
+  // capabilities following the current object, so we have to look at the copy size.
   __builtin_memcpy(buf, &t2, sizeof(t2));
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
   // CHECK-SAME: , i64 80, i1 false) [[NO_PRESERVE_TAGS:#[0-9]+]]{{$}}
@@ -72,24 +71,24 @@ void test_member_expr_byval(void *buf, struct TestWithCap t, struct TestNoCap t2
   // CHECK-SAME: , i64 96, i1 false){{$}}
   __builtin_memcpy(buf, &t2.not_a_cap, 40);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  __builtin_memcpy(buf, t2.array, 32);
+  // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
+  // CHECK-SAME: , i64 32, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  /// Check that we also set the attribute if we are copying beyond
+  /// the end of the array-decay expressions.
   __builtin_memcpy(buf, t2.array, 40);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
   __builtin_memcpy(buf, &t2.n, 40);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
   __builtin_memcpy(buf, (&t2)->array, 40);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
   __builtin_memcpy(buf, &(&t2)->not_a_cap, 40);
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
 
   // Direct assignment should always the no_preserve_tags attribute (the C2x
   // 6.5 memcpy+"Allocated objects have no declared type." case does not apply):
@@ -177,15 +176,15 @@ void test_member_expr_ref(void *buf, struct TestWithCap &t, struct TestNoCap &t2
   __builtin_memcpy(buf, &t.not_a_cap, 32);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
   // CHECK-CXX-SAME: , i64 32, i1 false){{$}}
-  __builtin_memcpy(buf, t.array, 32);
+  __builtin_memcpy(buf, t.array, 40);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-CXX-SAME: , i64 32, i1 false){{$}}
+  // CHECK-CXX-SAME: , i64 40, i1 false){{$}}
   __builtin_memcpy(buf, &t.n, 32);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
   // CHECK-CXX-SAME: , i64 32, i1 false){{$}}
-  __builtin_memcpy(buf, (&t)->array, 32);
+  __builtin_memcpy(buf, (&t)->array, 40);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-CXX-SAME: , i64 32, i1 false){{$}}
+  // CHECK-CXX-SAME: , i64 40, i1 false){{$}}
   __builtin_memcpy(buf, &(&t)->not_a_cap, 32);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
   // CHECK-CXX-SAME: , i64 32, i1 false){{$}}
@@ -196,24 +195,19 @@ void test_member_expr_ref(void *buf, struct TestWithCap &t, struct TestNoCap &t2
   // CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
   __builtin_memcpy(buf, &t2.not_a_cap, 40);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-CXX-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
   __builtin_memcpy(buf, t2.array, 40);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-CXX-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
   __builtin_memcpy(buf, &t2.n, 40);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-CXX-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
   __builtin_memcpy(buf, (&t2)->array, 40);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-CXX-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
   __builtin_memcpy(buf, &(&t2)->not_a_cap, 40);
   // CHECK-CXX: call void @llvm.memcpy.p200i8.p200i8.i64(
-  // CHECK-CXX-SAME: , i64 40, i1 false){{$}}
-  // TODO-CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
+  // CHECK-CXX-SAME: , i64 40, i1 false) [[NO_PRESERVE_TAGS]]{{$}}
 
   // Direct assignment should always the no_preserve_tags attribute (the C2x
   // 6.5 memcpy+"Allocated objects have no declared type." case does not apply):
@@ -225,4 +219,4 @@ void test_member_expr_ref(void *buf, struct TestWithCap &t, struct TestNoCap &t2
 
 // CHECK: attributes [[MUST_PRESERVE_STRUCT_WITHCAP]] = { must_preserve_cheri_tags "frontend-memtransfer-type"="'struct TestWithCap'" }
 // CHECK: attributes [[MUST_PRESERVE_CHARPTR]] = { must_preserve_cheri_tags "frontend-memtransfer-type"="'char * __capability'" }
-// CHECK-C: attributes [[NO_PRESERVE_TAGS]] = { no_preserve_cheri_tags }
+// CHECK: attributes [[NO_PRESERVE_TAGS]] = { no_preserve_cheri_tags }
