Only perform type-base tag-preservation analysis if we know the real …

…type

Marking a memcpy() to/from a long* as not tag-preserving could result in
tag stripping for code that using type casts and is correct under strict
aliasing rules since the first store to a memory location determines the
type. Example from CTSRD-CHERI#506:
```
void *malloc(__SIZE_TYPE__);
void *memcpy(void *, const void *, __SIZE_TYPE__);

void foo(long **p, long **q) {
    *p = malloc(32);
    *q = malloc(32);
    (*p)[0] = 1;
    (*p)[1] = 2;
    *(void (**)(long **, long **))(*p + 2) = &foo;
    memcpy(*q, *p, 32);
}
```

Despite the memcpy being a long* (and therefore intuitevly not tag
preserving), we can't add the attribute since we don't actually know the
type of the underlying object (malloc creates an allocated with no declared
type). From C99:

The effective type of an object for an access to its stored value is the
declared type of the object, if any (footnote 75: Allocated objects have
no declared type).

If a value is stored into an object having no declared type through an
lvalue having a type that is not a character type, then the type of the
lvalue becomes the effective type of the object for that access and for
subsequent accesses that do not modify the stored value.

If a value is copied into an object having no declared type using memcpy
or memmove, or is copied as an array of character type, then the effective
type of the modified object for that access and for subsequent accesses
that do not modify the value is the effective type of the object from
which the value is copied, if it has one.

For all other accesses to an object having no declared type, the effective
type of the object is simply the type of the lvalue used for the access.

clang/lib/CodeGen/CGClass.cpp

@@ -980,11 +980,14 @@ namespace {
       LValue SrcLV = CGF.MakeNaturalAlignAddrLValue(SrcPtr, RecordTy);
       LValue Src = CGF.EmitLValueForFieldInitialization(SrcLV, FirstField);
 
+      // We can pass EffectiveTypeKnown=true since this a C++ field copy.
       emitMemcpyIR(
           Dest.isBitField() ? Dest.getBitFieldAddress() : Dest.getAddress(CGF),
           Src.isBitField() ? Src.getBitFieldAddress() : Src.getAddress(CGF),
           MemcpySize,
-          CGF.getTypes().copyShouldPreserveTagsForPointee(RecordTy));
+          CGF.getTypes().copyShouldPreserveTagsForPointee(
+              RecordTy, /*EffectiveTypeKnown=*/true,
+              CGF.Builder.getInt64(MemcpySize.getQuantity())));
       reset();
     }
 

clang/lib/CodeGen/CGDecl.cpp

@@ -1265,14 +1265,16 @@ static void emitStoresForConstant(CodeGenModule &CGM, const VarDecl &D,
     }
   }
 
-  // Copy from a global.
+  // Copy from a global (and therefore the effective type of the variable is
+  // known).
   auto *I = Builder.CreateMemCpy(
       Loc,
       createUnnamedGlobalForMemcpyFrom(CGM, D, Builder, constant,
                                        Loc.getAlignment()),
       SizeVal,
       IsAutoInit ? llvm::PreserveCheriTags::Unnecessary
-                 : CGM.getTypes().copyShouldPreserveTagsForPointee(D.getType()),
+                 : CGM.getTypes().copyShouldPreserveTagsForPointee(
+                       D.getType(), /*EffectiveTypeKnown=*/true, SizeVal),
       isVolatile);
   if (IsAutoInit)
     I->addAnnotationMetadata("auto-init");

clang/lib/CodeGen/CGExprAgg.cpp

@@ -749,7 +749,7 @@ void AggExprEmitter::VisitCastExpr(CastExpr *E) {
         CGF.getContext().getTypeSizeInChars(E->getType()).getQuantity());
     Builder.CreateMemCpy(
         DestAddress, SourceAddress, SizeVal,
-        CGF.getTypes().copyShouldPreserveTagsForPointee(E->getType()));
+        CGF.getTypes().copyShouldPreserveTags(E, E->getSubExpr(), SizeVal));
     break;
   }
 
@@ -2170,10 +2170,14 @@ void CodeGenFunction::EmitAggregateCopy(LValue Dest, LValue Src, QualType Ty,
       }
     }
   }
-
-  auto Inst = Builder.CreateMemCpy(
-      DestPtr, SrcPtr, SizeVal, getTypes().copyShouldPreserveTagsForPointee(Ty),
-      isVolatile);
+  // Note: this is used for expressions such as x = y, and not memcpy() calls,
+  // so according to C2x 6.5 "the effective type of the object is simply
+  // the type of the lvalue used for the access."
+  auto Inst =
+      Builder.CreateMemCpy(DestPtr, SrcPtr, SizeVal,
+                           getTypes().copyShouldPreserveTagsForPointee(
+                               Ty, /*EffectiveTypeKnown=*/true, SizeVal),
+                           isVolatile);
 
   // Determine the metadata to describe the position of any padding in this
   // memcpy, as well as the TBAA tags for the members of the struct, in case

clang/lib/CodeGen/CodeGenTypes.cpp

@@ -952,37 +952,39 @@ bool CodeGenTypes::isZeroInitializable(QualType T) {
   return true;
 }
 
-llvm::PreserveCheriTags
-CodeGenTypes::copyShouldPreserveTags(const Expr *Dest, const Expr *Src,
-                                     const llvm::Value *Size) {
+static bool isLessThanCapSize(const ASTContext &Context,
+                              const llvm::Value *Size) {
   if (auto ConstSize = dyn_cast_or_null<llvm::ConstantInt>(Size)) {
     // If the copy size is smaller than capability size we do not need to
     // preserve tag bits.
     auto CapSize = Context.toCharUnitsFromBits(
         Context.getTargetInfo().getCHERICapabilityWidth());
     if (ConstSize->getValue().slt(CapSize.getQuantity()))
-      return llvm::PreserveCheriTags::Unnecessary;
+      return true;
   }
-  auto GetPointee = [](const Expr *E) {
-    assert(E->getType()->isPointerType());
-    // 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 getPointee().
-    QualType Ty = E->IgnoreParenImpCasts()->getType();
-    if (Ty->isAnyPointerType())
-      return Ty->getPointeeType();
-    return Ty;
-  };
+  return false;
+}
 
-  auto DstPreserve = copyShouldPreserveTagsForPointee(GetPointee(Dest));
+llvm::PreserveCheriTags
+CodeGenTypes::copyShouldPreserveTags(const Expr *DestPtr, const Expr *SrcPtr,
+                                     const llvm::Value *Size) {
+  // 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())
+    return llvm::PreserveCheriTags::Unknown;
+  if (isLessThanCapSize(Context, Size)) {
+    // Copies smaller than capability size do not need to preserve tag bits.
+    return llvm::PreserveCheriTags::Unnecessary;
+  }
+  auto DstPreserve = copyShouldPreserveTags(DestPtr);
   if (DstPreserve == llvm::PreserveCheriTags::Unnecessary) {
     // If the destination does not need to preserve tags, we know that we don't
     // need to retain tags even if the source is a capability type.
     return llvm::PreserveCheriTags::Unnecessary;
   }
   assert(DstPreserve == llvm::PreserveCheriTags::Required ||
          DstPreserve == llvm::PreserveCheriTags::Unknown);
-  auto SrcPreserve = copyShouldPreserveTagsForPointee(GetPointee(Src));
+  auto SrcPreserve = copyShouldPreserveTags(SrcPtr);
   if (SrcPreserve == llvm::PreserveCheriTags::Unnecessary) {
     // If the copy source never contains capabilities, we don't need to retain
     // tags even if the destination is contains capabilities.
@@ -1001,14 +1003,42 @@ CodeGenTypes::copyShouldPreserveTags(const Expr *Dest, const Expr *Src,
   return DstPreserve;
 }
 
-llvm::PreserveCheriTags
-CodeGenTypes::copyShouldPreserveTags(QualType DestType) {
-  assert(DestType->isAnyPointerType() && "Copy dest must be a pointer type");
-  return copyShouldPreserveTagsForPointee(DestType->getPointeeType());
+llvm::PreserveCheriTags CodeGenTypes::copyShouldPreserveTags(const Expr *E) {
+  assert(E->getType()->isAnyPointerType());
+  // 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();
+  // TODO: Find the underlying VarDecl to improve diagnostics
+  const VarDecl *UnderlyingVar = nullptr;
+  // TODO: this assert may be overly aggressive
+  assert((!UnderlyingVar || UnderlyingVar->getType() == Ty) &&
+         "Passed wrong VarDecl?");
+  // If we have an underlying VarDecl, we can assume that the dynamic type of
+  // the object is known and can perform more detailed analysis.
+  return copyShouldPreserveTagsForPointee(Ty, UnderlyingVar != nullptr);
+}
+
+llvm::PreserveCheriTags CodeGenTypes::copyShouldPreserveTagsForPointee(
+    QualType CopyTy, bool EffectiveTypeKnown, const llvm::Value *SizeVal) {
+  // 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())
+    return llvm::PreserveCheriTags::Unknown;
+  if (isLessThanCapSize(Context, SizeVal)) {
+    // Copies smaller than capability size do not need to preserve tag bits.
+    return llvm::PreserveCheriTags::Unnecessary;
+  }
+  return copyShouldPreserveTagsForPointee(CopyTy, EffectiveTypeKnown);
 }
 
 llvm::PreserveCheriTags
-CodeGenTypes::copyShouldPreserveTagsForPointee(QualType Pointee) {
+CodeGenTypes::copyShouldPreserveTagsForPointee(QualType Pointee,
+                                               bool EffectiveTypeKnown) {
+  assert(Context.getTargetInfo().SupportsCapabilities() &&
+         "Should only be called for CHERI targets");
   assert(!Pointee.isNull() && "Should only be called for valid types");
   if (!getTarget().SupportsCapabilities()) {
     // If we are not compiling for CHERI, return Unknown to avoid adding the
@@ -1020,14 +1050,36 @@ CodeGenTypes::copyShouldPreserveTagsForPointee(QualType Pointee) {
     // If this is a capability type or a structure/union containing
     // capabilities, we obviously need to retain tag bits when copying.
     return llvm::PreserveCheriTags::Required;
+  } else if (!EffectiveTypeKnown) {
+    // If we don't know the underlying type of the copy (i.e. we just have a
+    // pointer without any additional context), we cannot assume that the actual
+    // object at that location matches the type of the pointer, so we have to
+    // conservatively return Unknown if containsCapabilities() was false.
+    // This is needed because C's strict aliasing rules are not based on the
+    // type of the pointer but rather based on the type of what was last stored.
+    // See C2x 6.5:
+    // "If a value is copied into an object having no declared type using memcpy
+    // or memmove, or is copied as an array of character type, then the
+    // effective type of the modified object for that access and for subsequent
+    // accesses that do not modify the value is the effective type of the object
+    // from which the value is copied, if it has one. For all other accesses to
+    // an object having no declared type, the effective type of the object is
+    // simply the type of the lvalue used for the access."
+    // And importantly: "Allocated objects have no declared type.", so unless
+    // we know what the underlying VarDecl is, we cannot use the type of the
+    // expression to determine whether it could hold tags.
+    return llvm::PreserveCheriTags::Unknown;
   } else if (Pointee->isIncompleteType()) {
     // We don't know if incomplete types contain capabilities, so be
     // conservative and assume that they might.
     return llvm::PreserveCheriTags::Unknown;
   } else if (auto *RD = Pointee->getAsRecordDecl()) {
     // TODO: should maybe add ASTContext::neverContainsCapabilities()
     if (RD->hasFlexibleArrayMember()) {
-      // This record may store capabilities in the trailing array.
+      // If this record has a trailing flexible array, we conservatively assume
+      // that the flexible array could contain capabilities.
+      // TODO: We could find the flexible array member and use that to
+      //  determine whether the type may contain tags.
       return llvm::PreserveCheriTags::Unknown;
     } else if (RD->field_empty()) {
       // This structure could be used as an opaque type -> assume it might
@@ -1046,15 +1098,16 @@ CodeGenTypes::copyShouldPreserveTagsForPointee(QualType Pointee) {
     // having to call the library function.
     return llvm::PreserveCheriTags::Unnecessary;
   } else if (Pointee->isArrayType()) {
-    return copyShouldPreserveTagsForPointee(
-        QualType(Pointee->getArrayElementTypeNoTypeQual(), 0));
+    return copyShouldPreserveTagsForPointee(Context.getBaseElementType(Pointee),
+                                            EffectiveTypeKnown);
   }
 
   if (Pointee->isScalarType()) {
     // We can return PreserveCheriTags::Unneccessary for scalar types that are
     // not void *, char *, or std::byte * (since those could point to anything).
     assert(!Pointee->isCHERICapabilityType(Context));
-    if (!Pointee->isStdByteType() && !Pointee->isCharType())
+    if (!Pointee->isStdByteType() && !Pointee->isCharType() &&
+        !Pointee->isVoidType())
       return llvm::PreserveCheriTags::Unnecessary;
   }
 

clang/lib/CodeGen/CodeGenTypes.h

@@ -308,13 +308,18 @@ class CodeGenTypes {
   /// pointer to DestType may need to preserve CHERI tags (i.e. needs to call
   /// the copy function at run time if the alignment is not greater than the
   /// alignment of the destination buffer.
-  llvm::PreserveCheriTags copyShouldPreserveTags(QualType DestType);
-  llvm::PreserveCheriTags copyShouldPreserveTags(const Expr *Dest,
-                                                 const Expr *Src,
+  /// This function attempts to determine the effective type of the source and
+  /// destination values (C2x 6.5p6) by checking for the underlying storage
+  /// (e.g. a referenced VarDecl) and performing a more conservative analysis
+  /// if this is not the case.
+  llvm::PreserveCheriTags copyShouldPreserveTags(const Expr *DestPtr,
+                                                 const Expr *SrcPtr,
                                                  const llvm::Value *SizeVal);
-  /// Same as the copyShouldPreserveTags(), but expects DestType to be the
+  /// Same as the copyShouldPreserveTags(), but expects CopyTy to be the
   /// pointee type rather than the type of the buffer pointer.
-  llvm::PreserveCheriTags copyShouldPreserveTagsForPointee(QualType DestType);
+  llvm::PreserveCheriTags
+  copyShouldPreserveTagsForPointee(QualType CopyTy, bool EffectiveTypeKnown,
+                                   const llvm::Value *SizeVal);
 
   bool isRecordLayoutComplete(const Type *Ty) const;
   bool noRecordsBeingLaidOut() const {
@@ -324,6 +329,10 @@ class CodeGenTypes {
     return RecordsBeingLaidOut.count(Ty);
   }
 
+private:
+  llvm::PreserveCheriTags copyShouldPreserveTags(const Expr *E);
+  llvm::PreserveCheriTags
+  copyShouldPreserveTagsForPointee(QualType DestType, bool EffectiveTypeKnown);
 };
 
 }  // end namespace CodeGen

clang/test/CodeGen/cheri/no-tag-copy-attribute-nocap-struct.c

@@ -3,9 +3,8 @@
 /// not to contain capabilities. Previously we assumed that all copies >= sizeof(capability)
 /// can contain capabilities and we therefore fell back to calling memcpy if the
 /// alignment was less than >= alignof(capability).
-/// TODO: include some end-to-end testing to ensure that the frontend and backend agree on
-///  eliding the memcpy() call that we were previously force to make?
-// RUN: %riscv64_cheri_purecap_cc1 %s -emit-llvm -o - -O0 | FileCheck %s
+// RUN: %riscv64_cheri_purecap_cc1 %s -emit-llvm -o - -O0 | FileCheck %s --check-prefixes=CHECK
+// RUN: %riscv64_cheri_purecap_cc1 %s -relaxed-aliasing -emit-llvm -o - -O0 | FileCheck %s --check-prefixes=CHECK
 
 #if __has_feature(capabilities)
 #define CAP_SIZE sizeof(void *__capability)
@@ -28,6 +27,9 @@ struct vdso_timehands {
 
 void test_binuptime_assign(struct bintime *bt, struct vdso_timehands *th) {
   *bt = th->th_offset;
+  // We should always be able add the no_preserve_tags attribute for this assignment
+  // since this falls under C2x 6.5 "For all other accesses to an object having no declared type,
+  // the effective type of the object is simply the type of the lvalue used for the access."
   // CHECK-LABEL: void @test_binuptime_assign(
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(i8 addrspace(200)* align 8 {{%[0-9a-zA-Z.]+}}, i8 addrspace(200)* align 8 {{%[0-9a-zA-Z.]+}},
   // CHECK-SAME: i64 16, i1 false) [[NO_PRESERVE_TAGS_ATTR:#[0-9]+]]{{$}}
@@ -53,15 +55,17 @@ union large_union_align_half_cap_size {
 };
 
 void test_bigger_union_copy(union large_union_align_half_cap_size *a, union large_union_align_half_cap_size *b) {
-  // No tags in this union -> can inline the memcpy()
+  // No tags in this union -> can inline the memcpy().
+  // Note: we can do this even with -fno-strict-aliasing since this is a direct assignment.
+  // XXX: Do we need an option to treats underaligned char[] members as potentially tag-bearing?
   *a = *b;
   // CHECK-LABEL: void @test_bigger_union_copy(
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(i8 addrspace(200)* align 8 {{%[0-9a-zA-Z.]+}}, i8 addrspace(200)* align 8 {{%[0-9a-zA-Z.]+}},
   // CHECK-SAME: i64 64, i1 false) [[NO_PRESERVE_TAGS_ATTR]]{{$}}
 }
 
 void test_align_copy_voidptr(void *a, void *b) {
-  // void* could contain caps but we don't add the attribute and rely on the backend to decide
+  // void* could contain caps so we don't add the attribute and rely on the backend to decide
   memcpy(a, b, CAP_SIZE);
   // CHECK-LABEL: void @test_align_copy_voidptr(
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(i8 addrspace(200)* align 1 {{%[0-9a-zA-Z.]+}}, i8 addrspace(200)* align 1 {{%[0-9a-zA-Z.]+}},
@@ -70,19 +74,20 @@ void test_align_copy_voidptr(void *a, void *b) {
 
 void test_align_copy_charptr(char *a, char *b) {
   // char* could contain caps since it's (unfortunately) basically the same as void*,
-  // but again we don't add the attribute and rely on the backend to decide
+  // so again we don't add the attribute and rely on the backend to decide
   memcpy(a, b, CAP_SIZE);
   // CHECK-LABEL: void @test_align_copy_charptr(
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(i8 addrspace(200)* align 1 {{%[0-9a-zA-Z.]+}}, i8 addrspace(200)* align 1 {{%[0-9a-zA-Z.]+}},
   // CHECK-SAME: i64 16, i1 false){{$}}
 }
 
 void test_align_copy_longptr(long *a, long *b) {
-  // Note: here we don't infer the no-preserve tags (yet)
+  // We don't know the effective type of the underlying objects, so we can't add
+  // no_preserve_cheri_tags despite both pointeee types being non-tag-carrying.
   memcpy(a, b, CAP_SIZE);
   // CHECK-LABEL: void @test_align_copy_longptr(
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(i8 addrspace(200)* align 8 {{%[0-9a-zA-Z.]+}}, i8 addrspace(200)* align 8 {{%[0-9a-zA-Z.]+}},
-  // CHECK-SAME: i64 16, i1 false) [[NO_PRESERVE_TAGS_ATTR]]
+  // CHECK-SAME: i64 16, i1 false){{$}}
 }
 
 #if __has_feature(capabilities)
@@ -115,10 +120,11 @@ void test_align_copy_fwd_declared_2(void *a, struct fwddecl *b) {
 }
 
 void test_align_copy_fwd_declared_dst_notag(long *a, struct fwddecl *b) {
-  // We don't know if src contains capabilities, but the destination can't contain tags
+  // We don't know if src contains capabilities, and we also can't assume this
+  // for the destination since we don't know the effective type of the underlying object.
   // CHECK-LABEL: void @test_align_copy_fwd_declared_dst_notag(
   // CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(i8 addrspace(200)* align 8 {{%[0-9a-zA-Z.]+}}, i8 addrspace(200)* align 1 {{%[0-9a-zA-Z.]+}},
-  // CHECK-SAME: i64 16, i1 false) [[NO_PRESERVE_TAGS_ATTR]]
+  // CHECK-SAME: i64 16, i1 false){{$}}
   memcpy(a, b, CAP_SIZE);
   // Note: if you look at the assembly output for  this call, it
   // still uses memcpy despite the attribute. b is only aligned to one byte and