Skip to content

Commit

Permalink
Don't perform type-base tag-preservation analysis with -fno-strict-al…
Browse files Browse the repository at this point in the history
…iasing

Marking a memcpy() to/from a long* as not tag-preserving could result in
tag stripping for code that assumes relaxed aliasing.

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);
}
```

Adding the no_preserve_tags attribute to memcpy() is safe under
-fstrict-aliasing (since the store above is invalid), but with
-fno-strict-aliasing this aligned memcpy must preserve tags.
  • Loading branch information
arichardson committed Mar 10, 2021
1 parent 567a13a commit 0fb29f1
Show file tree
Hide file tree
Showing 10 changed files with 106 additions and 42 deletions.
21 changes: 13 additions & 8 deletions clang/lib/CodeGen/CGBuiltin.cpp
Expand Up @@ -2749,7 +2749,8 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
E->getArg(1)->getExprLoc(), FD, 1);
auto CI = Builder.CreateMemCpy(
Dest, Src, SizeVal,
getTypes().copyShouldPreserveTags(E->getArg(0), E->getArg(1), SizeVal),
getTypes().copyShouldPreserveTags(E->getArg(0), E->getArg(1), SizeVal,
CGM.getCodeGenOpts().RelaxedAliasing),
false);
checkCapabilityCopy(*this, "memcpy", E->getArg(1), CI);
if (BuiltinID == Builtin::BImempcpy ||
Expand All @@ -2769,10 +2770,11 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
E->getArg(0)->getExprLoc(), FD, 0);
EmitNonNullArgCheck(RValue::get(Src.getPointer()), E->getArg(1)->getType(),
E->getArg(1)->getExprLoc(), FD, 1);
Builder.CreateMemCpyInline(
Dest, Src, Size,
getTypes().copyShouldPreserveTags(E->getArg(0), E->getArg(1),
Builder.getInt64(Size)));
Builder.CreateMemCpyInline(Dest, Src, Size,
getTypes().copyShouldPreserveTags(
E->getArg(0), E->getArg(1),
Builder.getInt64(Size),
CGM.getCodeGenOpts().RelaxedAliasing));
return RValue::get(nullptr);
}

Expand All @@ -2799,7 +2801,8 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
Value *SizeVal = llvm::ConstantInt::get(Builder.getContext(), Size);
auto CI = Builder.CreateMemCpy(
Dest, Src, SizeVal,
getTypes().copyShouldPreserveTags(E->getArg(0), E->getArg(1), SizeVal),
getTypes().copyShouldPreserveTags(E->getArg(0), E->getArg(1), SizeVal,
CGM.getCodeGenOpts().RelaxedAliasing),
false);
checkCapabilityCopy(*this, "memcpy", E->getArg(1), CI);
return RValue::get(Dest.getPointer(), Dest.getAlignment().getQuantity());
Expand Down Expand Up @@ -2834,7 +2837,8 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
Value *SizeVal = llvm::ConstantInt::get(Builder.getContext(), Size);
auto CI = Builder.CreateMemMove(
Dest, Src, SizeVal,
getTypes().copyShouldPreserveTags(E->getArg(0), E->getArg(1), SizeVal),
getTypes().copyShouldPreserveTags(E->getArg(0), E->getArg(1), SizeVal,
CGM.getCodeGenOpts().RelaxedAliasing),
false);
checkCapabilityCopy(*this, "memmove", E->getArg(1), CI);
return RValue::get(Dest.getPointer(), Dest.getAlignment().getQuantity());
Expand All @@ -2851,7 +2855,8 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
E->getArg(1)->getExprLoc(), FD, 1);
auto CI = Builder.CreateMemMove(
Dest, Src, SizeVal,
getTypes().copyShouldPreserveTags(E->getArg(0), E->getArg(1), SizeVal),
getTypes().copyShouldPreserveTags(E->getArg(0), E->getArg(1), SizeVal,
CGM.getCodeGenOpts().RelaxedAliasing),
false);
checkCapabilityCopy(*this, "memmove", E->getArg(1), CI);
return RValue::get(Dest.getPointer(), Dest.getAlignment().getQuantity());
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/CodeGen/CGClass.cpp
Expand Up @@ -987,7 +987,8 @@ namespace {
Dest.isBitField() ? Dest.getBitFieldAddress() : Dest.getAddress(CGF),
Src.isBitField() ? Src.getBitFieldAddress() : Src.getAddress(CGF),
MemcpySize,
CGF.getTypes().copyShouldPreserveTagsForPointee(RecordTy));
CGF.getTypes().copyShouldPreserveTagsForPointee(
RecordTy, /*IgnoreTypes=*/false));
reset();
}

Expand Down
13 changes: 7 additions & 6 deletions clang/lib/CodeGen/CGDecl.cpp
Expand Up @@ -1239,12 +1239,13 @@ static void emitStoresForConstant(CodeGenModule &CGM, const VarDecl &D,
}

// Copy from a global.
Builder.CreateMemCpy(
Loc,
createUnnamedGlobalForMemcpyFrom(CGM, D, Builder, constant,
Loc.getAlignment()),
SizeVal, CGM.getTypes().copyShouldPreserveTagsForPointee(D.getType()),
isVolatile);
Builder.CreateMemCpy(Loc,
createUnnamedGlobalForMemcpyFrom(
CGM, D, Builder, constant, Loc.getAlignment()),
SizeVal,
CGM.getTypes().copyShouldPreserveTagsForPointee(
D.getType(), /*IgnoreTypes=*/false),
isVolatile);
}

static void emitStoresForZeroInit(CodeGenModule &CGM, const VarDecl &D,
Expand Down
9 changes: 5 additions & 4 deletions clang/lib/CodeGen/CGExprAgg.cpp
Expand Up @@ -743,9 +743,9 @@ void AggExprEmitter::VisitCastExpr(CastExpr *E) {
llvm::Value *SizeVal = llvm::ConstantInt::get(
CGF.SizeTy,
CGF.getContext().getTypeSizeInChars(E->getType()).getQuantity());
Builder.CreateMemCpy(
DestAddress, SourceAddress, SizeVal,
CGF.getTypes().copyShouldPreserveTagsForPointee(E->getType()));
Builder.CreateMemCpy(DestAddress, SourceAddress, SizeVal,
CGF.getTypes().copyShouldPreserveTagsForPointee(
E->getType(), /*IgnoreTypes=*/false));
break;
}

Expand Down Expand Up @@ -2042,7 +2042,8 @@ void CodeGenFunction::EmitAggregateCopy(LValue Dest, LValue Src, QualType Ty,
}

auto Inst = Builder.CreateMemCpy(
DestPtr, SrcPtr, SizeVal, getTypes().copyShouldPreserveTagsForPointee(Ty),
DestPtr, SrcPtr, SizeVal,
getTypes().copyShouldPreserveTagsForPointee(Ty, /*IgnoreTypes=*/false),
isVolatile);

// Determine the metadata to describe the position of any padding in this
Expand Down
1 change: 1 addition & 0 deletions clang/lib/CodeGen/CGExprScalar.cpp
Expand Up @@ -4216,6 +4216,7 @@ Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) {
? cast<BinaryOperator>(op.E)->getRHS()
: nullptr;
if (RHSExpr && RHSExpr->getType()->isIntCapType()) {
// FIXME: avoid warning if result is casted to int
const bool IsAddrMode = CGF.CGM.getLangOpts().cheriUIntCapUsesAddr();
// Subtraction of __intcap_t is ambiguous: could be pointer
// increment/decrement or pointer difference
Expand Down
32 changes: 23 additions & 9 deletions clang/lib/CodeGen/CodeGenTypes.cpp
Expand Up @@ -970,7 +970,8 @@ bool CodeGenTypes::isZeroInitializable(QualType T) {

llvm::PreserveCheriTags
CodeGenTypes::copyShouldPreserveTags(const Expr *Dest, const Expr *Src,
const llvm::Value *Size) {
const llvm::Value *Size,
bool IgnoreTypes) {
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.
Expand All @@ -990,15 +991,17 @@ CodeGenTypes::copyShouldPreserveTags(const Expr *Dest, const Expr *Src,
return Ty;
};

auto DstPreserve = copyShouldPreserveTagsForPointee(GetPointee(Dest));
auto DstPreserve =
copyShouldPreserveTagsForPointee(GetPointee(Dest), IgnoreTypes);
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(GetUnderlyingType(Src), IgnoreTypes);
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.
Expand All @@ -1017,27 +1020,37 @@ CodeGenTypes::copyShouldPreserveTags(const Expr *Dest, const Expr *Src,
return DstPreserve;
}

llvm::PreserveCheriTags
CodeGenTypes::copyShouldPreserveTags(QualType DestType) {
llvm::PreserveCheriTags CodeGenTypes::copyShouldPreserveTags(QualType DestType,
bool IgnoreTypes) {
assert(DestType->isAnyPointerType() && "Copy dest must be a pointer type");
return copyShouldPreserveTagsForPointee(DestType->getPointeeType());
return copyShouldPreserveTagsForPointee(DestType->getPointeeType(),
IgnoreTypes);
}

llvm::PreserveCheriTags
CodeGenTypes::copyShouldPreserveTagsForPointee(QualType Pointee) {
CodeGenTypes::copyShouldPreserveTagsForPointee(QualType Pointee,
bool IgnoreTypes) {
assert(!Pointee.isNull() && "Should only be called for valid types");
if (Context.containsCapabilities(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 (IgnoreTypes) {
// If we are ignoring the type of the copy (e.g. -fno-strict-aliasing), we
// cannot assume anything else so return Unknown if containsCapabilities()
// was false.
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
Expand All @@ -1064,7 +1077,8 @@ CodeGenTypes::copyShouldPreserveTagsForPointee(QualType Pointee) {
// 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;
}

Expand Down
11 changes: 8 additions & 3 deletions clang/lib/CodeGen/CodeGenTypes.h
Expand Up @@ -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);
/// IgnoreTypes can be set to not treat copies of a e.g. non-capability type
/// as potentially carrying tags (e.g. memcpy() with -fno-strict-aliasing).
llvm::PreserveCheriTags copyShouldPreserveTags(QualType DestType,
bool IgnoreTypes);
llvm::PreserveCheriTags copyShouldPreserveTags(const Expr *Dest,
const Expr *Src,
const llvm::Value *SizeVal);
const llvm::Value *SizeVal,
bool IgnoreTypes);
/// Same as the copyShouldPreserveTags(), but expects DestType to be the
/// pointee type rather than the type of the buffer pointer.
llvm::PreserveCheriTags copyShouldPreserveTagsForPointee(QualType DestType);
llvm::PreserveCheriTags copyShouldPreserveTagsForPointee(QualType DestType,
bool IgnoreTypes);

bool isRecordLayoutComplete(const Type *Ty) const;
bool noRecordsBeingLaidOut() const {
Expand Down
21 changes: 14 additions & 7 deletions clang/test/CodeGen/cheri/no-tag-copy-attribute-nocap-struct.c
Expand Up @@ -5,7 +5,8 @@
/// 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,CHECK-STRICT-ALIASING
// RUN: %riscv64_cheri_purecap_cc1 %s -relaxed-aliasing -emit-llvm -o - -O0 | FileCheck %s --check-prefixes=CHECK,CHECK-RELAXED-ALIASING

#if __has_feature(capabilities)
#define CAP_SIZE sizeof(void *__capability)
Expand All @@ -28,6 +29,7 @@ struct vdso_timehands {

void test_binuptime_assign(struct bintime *bt, struct vdso_timehands *th) {
*bt = th->th_offset;
// We should add the no_preserve_tags attribute for this assignment even with -fno-strict-aliasing:
// CHECK-LABEL: define 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]+]]{{$}}
Expand All @@ -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: define 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: define 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.]+}},
Expand All @@ -78,11 +82,12 @@ void test_align_copy_charptr(char *a, char *b) {
}

void test_align_copy_longptr(long *a, long *b) {
// Note: here we don't infer the no-preserve tags (yet)
// Note: We cannot infer the no-preserve tags attribute when building with -fno-strict-aliasing
memcpy(a, b, CAP_SIZE);
// CHECK-LABEL: define 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-STRICT-ALIASING-SAME: i64 16, i1 false) [[NO_PRESERVE_TAGS_ATTR]]{{$}}
// CHECK-RELAXED-ALIASING-SAME: i64 16, i1 false){{$}}
}

#if __has_feature(capabilities)
Expand Down Expand Up @@ -115,10 +120,12 @@ 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, but the destination can't contain tags.
// Note: we can't make this assumption for -fno-strict-aliasing
// CHECK-LABEL: define 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-STRICT-ALIASING-SAME: i64 16, i1 false) [[NO_PRESERVE_TAGS_ATTR]]{{$}}
// CHECK-RELAXED-ALIASING-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
Expand Down
12 changes: 8 additions & 4 deletions clang/test/CodeGen/cheri/no-tag-copy-attribute-with-caps.c
@@ -1,5 +1,7 @@
// REQUIRES: riscv-registered-target
// RUN: %riscv64_cheri_purecap_cc1 %s -o - -emit-llvm | FileCheck %s
// RUN: %riscv64_cheri_purecap_cc1 %s -o - -emit-llvm | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-ALIASING
// RUN: %riscv64_cheri_purecap_cc1 %s -o - -emit-llvm -relaxed-aliasing
// RUN: %riscv64_cheri_purecap_cc1 %s -o - -emit-llvm -relaxed-aliasing | FileCheck %s --check-prefixes=CHECK,CHECK-RELAXED-ALIASING
// Diagnostics are only emitted when generating assembly (with optimizations)
// RUN: %riscv64_cheri_purecap_cc1 -debug-info-kind=standalone %s -o /dev/null -O1 -S -verify

Expand All @@ -18,13 +20,15 @@ void test_addrof_char(struct OneCap *cap, char c, __uint128_t u) {
// CHECK: call void @llvm.memmove.p200i8.p200i8.i64(i8 addrspace(200)* align 1 {{%[a-z0-9]+}}.addr, i8 addrspace(200)* align 16 {{%[a-z0-9]+}}
// CHECK-SAME: , i64 1, i1 false) [[NO_PRESERVE_ATTR]]{{$}}

// uint128_t cannot not hold tags -> no need to preserve them.
// uint128_t cannot not hold tags -> no need to preserve them (unless we use -fno-strict-aliasing).
__builtin_memmove(cap, &u, sizeof(u));
// CHECK: call void @llvm.memmove.p200i8.p200i8.i64(i8 addrspace(200)* align 16 {{%[a-z0-9]+}}, i8 addrspace(200)* align 16 {{%[a-z0-9]+}}
// CHECK-SAME: , i64 16, i1 false) [[NO_PRESERVE_ATTR]]{{$}}
// CHECK-STRICT-ALIASING-SAME: , i64 16, i1 false) [[NO_PRESERVE_ATTR]]{{$}}
// CHECK-RELAXED-ALIASING-SAME: , i64 16, i1 false){{$}}
__builtin_memmove(&u, cap, sizeof(u));
// CHECK: call void @llvm.memmove.p200i8.p200i8.i64(i8 addrspace(200)* align 16 {{%[a-z0-9]+}}, i8 addrspace(200)* align 16 {{%[a-z0-9]+}}
// CHECK-SAME: , i64 16, i1 false) [[NO_PRESERVE_ATTR]]{{$}}
// CHECK-STRICT-ALIASING-SAME: , i64 16, i1 false) [[NO_PRESERVE_ATTR]]{{$}}
// CHECK-RELAXED-ALIASING-SAME: , i64 16, i1 false){{$}}
}

void test_small_copy(struct OneCap *cap1, struct OneCap *cap2) {
Expand Down
25 changes: 25 additions & 0 deletions clang/test/CodeGen/cheri/no-tag-copy-strict-aliasing.c
@@ -0,0 +1,25 @@
// RUN: %riscv64_cheri_purecap_cc1 %s -emit-llvm -o - -O0 | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-ALIASING
// RUN: %riscv64_cheri_purecap_cc1 %s -relaxed-aliasing -emit-llvm -o - -O0 | FileCheck %s --check-prefixes=CHECK,CHECK-RELAXED-ALIASING

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;
// Note: Storing a function pointer to a long* breaks the strict-aliasing rules
*(void (**)(long **, long **))(*p + 2) = &foo;
// As this is invalid under -fstrict-aliasing, we can add the no_preserve_tags metadata here despite knowing that
// it conains tags. However, with -fno-strict-aliasing, we must ensure that the memcpy() retains tags if possible:
// XXX: Maybe we should add a -fsanitize=cheri flag to diagnose this problem?
memcpy(*q, *p, 32);
// CHECK: @foo(i64 addrspace(200)* addrspace(200)* [[P:%.*]], i64 addrspace(200)* addrspace(200)* [[Q:%.*]]) addrspace(200)
// CHECK: call void @llvm.memcpy.p200i8.p200i8.i64(i8 addrspace(200)* align 8 {{%[0-9]+}}, i8 addrspace(200)* align 8 {{%[0-9]+}}
// CHECK-STRICT-ALIASING-SAME: , i64 32, i1 false) [[NO_TAGS_ATTR:#.*]]{{$}}
// CHECK-RELAXED-ALIASING-SAME: , i64 32, i1 false){{$}}
// CHECK-NEXT: ret void
}

// CHECK-STRICT-ALIASING: [[NO_TAGS_ATTR]] = { no_preserve_cheri_tags }

0 comments on commit 0fb29f1

Please sign in to comment.