[Clang][IR] add TBAA metadata on pointer, union and array types. #75177
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-backend-risc-v Author: Bushev Dmitry (dybv-sc) ChangesOptions to disable new behaviour: -Xclang -no-union-tbaa To enable union struct path tbaa there was need to update the way llvm handles struct path tbaa metadata. Multiple fields with same offset are now allowed. To properly resolve access type, struct path tbaa visitor considers each field with given offset. This patch works both for new and old struct path TBAA. Also this patch enables tbaa aware array and pointer accesses by default. Patch is 217.06 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75177.diff 24 Files Affected:
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index 0acb5ae134ea2..70f871267fd68 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -216,6 +216,9 @@ ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, SRCK_Defa
CODEGENOPT(RelaxAll , 1, 0) ///< Relax all machine code instructions.
CODEGENOPT(RelaxedAliasing , 1, 0) ///< Set when -fno-strict-aliasing is enabled.
CODEGENOPT(StructPathTBAA , 1, 0) ///< Whether or not to use struct-path TBAA.
+CODEGENOPT(UnionTBAA , 1, 0) ///< Whether or not to use struct-path TBAA on unions.
+CODEGENOPT(PointerTBAA , 1, 0) ///< Whether or not to generate TBAA on pointers.
+CODEGENOPT(ArrayTBAA , 1, 0) ///< Whether or not to generate TBAA on arrays.
CODEGENOPT(NewStructPathTBAA , 1, 0) ///< Whether or not to use enhanced struct-path TBAA.
CODEGENOPT(SaveTempLabels , 1, 0) ///< Save temporary labels.
CODEGENOPT(SanitizeAddressUseAfterScope , 1, 0) ///< Enable use-after-scope detection
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 25c76cf2ad2c8..688424f0b12bc 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -6822,6 +6822,15 @@ def relaxed_aliasing : Flag<["-"], "relaxed-aliasing">,
def no_struct_path_tbaa : Flag<["-"], "no-struct-path-tbaa">,
HelpText<"Turn off struct-path aware Type Based Alias Analysis">,
MarshallingInfoNegativeFlag<CodeGenOpts<"StructPathTBAA">>;
+def no_union_tbaa : Flag<["-"], "no-union-tbaa">,
+ HelpText<"Turn off struct-path aware Type Based Alias Analysis for unions">,
+ MarshallingInfoNegativeFlag<CodeGenOpts<"UnionTBAA">>;
+def no_pointer_tbaa : Flag<["-"], "no-pointer-tbaa">,
+ HelpText<"Turn off Type Based Alias Analysis for pointer types">,
+ MarshallingInfoNegativeFlag<CodeGenOpts<"PointerTBAA">>;
+def no_array_tbaa : Flag<["-"], "no-array-tbaa">,
+ HelpText<"Turn off Type Based Alias Analysis for array types">,
+ MarshallingInfoNegativeFlag<CodeGenOpts<"ArrayTBAA">>;
def new_struct_path_tbaa : Flag<["-"], "new-struct-path-tbaa">,
HelpText<"Enable enhanced struct-path aware Type Based Alias Analysis">;
def mdebug_pass : Separate<["-"], "mdebug-pass">,
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 69cf7f76be9a7..5012bf9c5cbff 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -4080,7 +4080,11 @@ LValue CodeGenFunction::EmitArraySubscriptExpr(const ArraySubscriptExpr *E,
E->getType(), !getLangOpts().isSignedOverflowDefined(), SignedIndices,
E->getExprLoc(), &arrayType, E->getBase());
EltBaseInfo = ArrayLV.getBaseInfo();
- EltTBAAInfo = CGM.getTBAAInfoForSubobject(ArrayLV, E->getType());
+ // If array is member of some aggregate, keep struct path TBAA information
+ // about it.
+ EltTBAAInfo = isa<MemberExpr>(Array) && CGM.getCodeGenOpts().ArrayTBAA
+ ? ArrayLV.getTBAAInfo()
+ : CGM.getTBAAInfoForSubobject(ArrayLV, E->getType());
} else {
// The base must be a pointer; emit it with an estimate of its alignment.
Addr = EmitPointerWithAlignment(E->getBase(), &EltBaseInfo, &EltTBAAInfo);
@@ -4598,8 +4602,7 @@ LValue CodeGenFunction::EmitLValueForField(LValue base,
if (base.getTBAAInfo().isMayAlias() ||
rec->hasAttr<MayAliasAttr>() || FieldType->isVectorType()) {
FieldTBAAInfo = TBAAAccessInfo::getMayAliasInfo();
- } else if (rec->isUnion()) {
- // TODO: Support TBAA for unions.
+ } else if (rec->isUnion() && !CGM.getCodeGenOpts().UnionTBAA) {
FieldTBAAInfo = TBAAAccessInfo::getMayAliasInfo();
} else {
// If no base type been assigned for the base access, then try to generate
diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp b/clang/lib/CodeGen/CodeGenTBAA.cpp
index dc288bc3f6157..5381d308204d1 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -94,7 +94,7 @@ static bool TypeHasMayAlias(QualType QTy) {
}
/// Check if the given type is a valid base type to be used in access tags.
-static bool isValidBaseType(QualType QTy) {
+static bool isValidBaseType(QualType QTy, const CodeGenOptions &CodeGenOpts) {
if (QTy->isReferenceType())
return false;
if (const RecordType *TTy = QTy->getAs<RecordType>()) {
@@ -105,13 +105,28 @@ static bool isValidBaseType(QualType QTy) {
if (RD->hasFlexibleArrayMember())
return false;
// RD can be struct, union, class, interface or enum.
- // For now, we only handle struct and class.
- if (RD->isStruct() || RD->isClass())
+ if (RD->isStruct() || RD->isClass() ||
+ (RD->isUnion() && CodeGenOpts.UnionTBAA))
return true;
}
return false;
}
+std::string CodeGenTBAA::getPointeeName(const Type *Ty) {
+ if (isa<BuiltinType>(Ty)) {
+ llvm::MDNode *ScalarMD = getTypeInfoHelper(Ty);
+ auto &Op = ScalarMD->getOperand(CodeGenOpts.NewStructPathTBAA ? 2 : 0);
+ assert(isa<llvm::MDString>(Op) && "Expected MDString operand");
+ return cast<llvm::MDString>(Op)->getString().str();
+ }
+
+ if (Ty->isIncompleteType())
+ return "<incomplete type>";
+
+ // Pointers to different types never alias
+ return Ty->getCanonicalTypeInternal().getAsString();
+}
+
llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type *Ty) {
uint64_t Size = Context.getTypeSizeInChars(Ty).getQuantity();
@@ -184,13 +199,24 @@ llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type *Ty) {
return getChar();
// Handle pointers and references.
- // TODO: Implement C++'s type "similarity" and consider dis-"similar"
- // pointers distinct.
- if (Ty->isPointerType() || Ty->isReferenceType())
- return createScalarTypeNode("any pointer", getChar(), Size);
+ // Pointer types never alias if their pointee type is distinct.
+ if ((Ty->isPointerType() || Ty->isReferenceType())) {
+ llvm::MDNode *AnyPtr = createScalarTypeNode("any pointer", getChar(), Size);
+ if (!CodeGenOpts.PointerTBAA)
+ return AnyPtr;
+ unsigned PtrDepth = 0;
+ do {
+ PtrDepth++;
+ Ty = Ty->getPointeeType().getTypePtr();
+ } while (!Ty->getPointeeType().isNull());
+ std::string PtrName;
+ llvm::raw_string_ostream OS{PtrName};
+ OS << "p" << PtrDepth << " " << getPointeeName(Ty);
+ return createScalarTypeNode(PtrName, AnyPtr, Size);
+ }
// Accesses to arrays are accesses to objects of their element types.
- if (CodeGenOpts.NewStructPathTBAA && Ty->isArrayType())
+ if (CodeGenOpts.ArrayTBAA && Ty->isArrayType())
return getTypeInfo(cast<ArrayType>(Ty)->getElementType());
// Enum types are distinct types. In C++ they have "underlying types",
@@ -241,7 +267,7 @@ llvm::MDNode *CodeGenTBAA::getTypeInfo(QualType QTy) {
// subsequent accesses to direct and indirect members of that aggregate will
// be considered may-alias too.
// TODO: Combine getTypeInfo() and getBaseTypeInfo() into a single function.
- if (isValidBaseType(QTy))
+ if (isValidBaseType(QTy, CodeGenOpts))
return getBaseTypeInfo(QTy);
const Type *Ty = Context.getCanonicalType(QTy).getTypePtr();
@@ -353,7 +379,7 @@ llvm::MDNode *CodeGenTBAA::getBaseTypeInfoHelper(const Type *Ty) {
const CXXRecordDecl *BaseRD = BaseQTy->getAsCXXRecordDecl();
if (BaseRD->isEmpty())
continue;
- llvm::MDNode *TypeNode = isValidBaseType(BaseQTy)
+ llvm::MDNode *TypeNode = isValidBaseType(BaseQTy, CodeGenOpts)
? getBaseTypeInfo(BaseQTy)
: getTypeInfo(BaseQTy);
if (!TypeNode)
@@ -378,8 +404,9 @@ llvm::MDNode *CodeGenTBAA::getBaseTypeInfoHelper(const Type *Ty) {
if (Field->isZeroSize(Context) || Field->isUnnamedBitfield())
continue;
QualType FieldQTy = Field->getType();
- llvm::MDNode *TypeNode = isValidBaseType(FieldQTy) ?
- getBaseTypeInfo(FieldQTy) : getTypeInfo(FieldQTy);
+ llvm::MDNode *TypeNode = isValidBaseType(FieldQTy, CodeGenOpts)
+ ? getBaseTypeInfo(FieldQTy)
+ : getTypeInfo(FieldQTy);
if (!TypeNode)
return nullptr;
@@ -417,7 +444,7 @@ llvm::MDNode *CodeGenTBAA::getBaseTypeInfoHelper(const Type *Ty) {
}
llvm::MDNode *CodeGenTBAA::getBaseTypeInfo(QualType QTy) {
- if (!isValidBaseType(QTy))
+ if (!isValidBaseType(QTy, CodeGenOpts))
return nullptr;
const Type *Ty = Context.getCanonicalType(QTy).getTypePtr();
diff --git a/clang/lib/CodeGen/CodeGenTBAA.h b/clang/lib/CodeGen/CodeGenTBAA.h
index a65963596fe9d..02fda5a5d9270 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.h
+++ b/clang/lib/CodeGen/CodeGenTBAA.h
@@ -158,6 +158,8 @@ class CodeGenTBAA {
llvm::MDNode *createScalarTypeNode(StringRef Name, llvm::MDNode *Parent,
uint64_t Size);
+ std::string getPointeeName(const Type *Ty);
+
/// getTypeInfoHelper - An internal helper function to generate metadata used
/// to describe accesses to objects of the given type.
llvm::MDNode *getTypeInfoHelper(const Type *Ty);
diff --git a/clang/test/CXX/drs/dr158.cpp b/clang/test/CXX/drs/dr158.cpp
index a0a8bd05baee3..cba85fa8989b4 100644
--- a/clang/test/CXX/drs/dr158.cpp
+++ b/clang/test/CXX/drs/dr158.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++98 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++11 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++14 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++1z %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++98 %s -O3 -pedantic-errors -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++11 %s -O3 -pedantic-errors -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++14 %s -O3 -pedantic-errors -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++1z %s -O3 -pedantic-errors -emit-llvm -o - | FileCheck %s
// dr158: yes
@@ -18,9 +18,9 @@ struct A {};
// CHECK-LABEL: define {{.*}} @_Z1g
const int *(A::*const *g(const int *(A::* const **p)[3], int *(A::***q)[3]))[3] {
- // CHECK: load ptr, {{.*}}, !tbaa ![[MEMPTR_TBAA:[^,]*]]
+ // CHECK: load ptr, {{.*}}, !tbaa ![[MEMPTR_TBAA_CONST:[^,]*]]
const int *(A::*const *x)[3] = *p;
- // CHECK: store ptr null, {{.*}}, !tbaa ![[MEMPTR_TBAA]]
+ // CHECK: store ptr null, {{.*}}, !tbaa ![[MEMPTR_TBAA:[^,]*]]
*q = 0;
return x;
}
diff --git a/clang/test/CodeGen/attr-arm-sve-vector-bits-bitcast.c b/clang/test/CodeGen/attr-arm-sve-vector-bits-bitcast.c
index 22e2e0c2ff102..c9545c3346108 100644
--- a/clang/test/CodeGen/attr-arm-sve-vector-bits-bitcast.c
+++ b/clang/test/CodeGen/attr-arm-sve-vector-bits-bitcast.c
@@ -85,21 +85,21 @@ void write_int64(struct struct_int64 *s, svint64_t x) {
// CHECK-128-LABEL: @read_float64(
// CHECK-128-NEXT: entry:
// CHECK-128-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_FLOAT64:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-128-NEXT: [[TMP0:%.*]] = load <2 x double>, ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-128-NEXT: [[TMP0:%.*]] = load <2 x double>, ptr [[Y]], align 16, !tbaa [[TBAA6:![0-9]+]]
// CHECK-128-NEXT: [[CAST_SCALABLE:%.*]] = tail call <vscale x 2 x double> @llvm.vector.insert.nxv2f64.v2f64(<vscale x 2 x double> undef, <2 x double> [[TMP0]], i64 0)
// CHECK-128-NEXT: ret <vscale x 2 x double> [[CAST_SCALABLE]]
//
// CHECK-256-LABEL: @read_float64(
// CHECK-256-NEXT: entry:
// CHECK-256-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_FLOAT64:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-256-NEXT: [[TMP0:%.*]] = load <4 x double>, ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-256-NEXT: [[TMP0:%.*]] = load <4 x double>, ptr [[Y]], align 16, !tbaa [[TBAA6:![0-9]+]]
// CHECK-256-NEXT: [[CAST_SCALABLE:%.*]] = tail call <vscale x 2 x double> @llvm.vector.insert.nxv2f64.v4f64(<vscale x 2 x double> undef, <4 x double> [[TMP0]], i64 0)
// CHECK-256-NEXT: ret <vscale x 2 x double> [[CAST_SCALABLE]]
//
// CHECK-512-LABEL: @read_float64(
// CHECK-512-NEXT: entry:
// CHECK-512-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_FLOAT64:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-512-NEXT: [[TMP0:%.*]] = load <8 x double>, ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-512-NEXT: [[TMP0:%.*]] = load <8 x double>, ptr [[Y]], align 16, !tbaa [[TBAA6:![0-9]+]]
// CHECK-512-NEXT: [[CAST_SCALABLE:%.*]] = tail call <vscale x 2 x double> @llvm.vector.insert.nxv2f64.v8f64(<vscale x 2 x double> undef, <8 x double> [[TMP0]], i64 0)
// CHECK-512-NEXT: ret <vscale x 2 x double> [[CAST_SCALABLE]]
//
@@ -111,21 +111,21 @@ svfloat64_t read_float64(struct struct_float64 *s) {
// CHECK-128-NEXT: entry:
// CHECK-128-NEXT: [[CAST_FIXED:%.*]] = tail call <2 x double> @llvm.vector.extract.v2f64.nxv2f64(<vscale x 2 x double> [[X:%.*]], i64 0)
// CHECK-128-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_FLOAT64:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-128-NEXT: store <2 x double> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-128-NEXT: store <2 x double> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA6]]
// CHECK-128-NEXT: ret void
//
// CHECK-256-LABEL: @write_float64(
// CHECK-256-NEXT: entry:
// CHECK-256-NEXT: [[CAST_FIXED:%.*]] = tail call <4 x double> @llvm.vector.extract.v4f64.nxv2f64(<vscale x 2 x double> [[X:%.*]], i64 0)
// CHECK-256-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_FLOAT64:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-256-NEXT: store <4 x double> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-256-NEXT: store <4 x double> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA6]]
// CHECK-256-NEXT: ret void
//
// CHECK-512-LABEL: @write_float64(
// CHECK-512-NEXT: entry:
// CHECK-512-NEXT: [[CAST_FIXED:%.*]] = tail call <8 x double> @llvm.vector.extract.v8f64.nxv2f64(<vscale x 2 x double> [[X:%.*]], i64 0)
// CHECK-512-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_FLOAT64:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-512-NEXT: store <8 x double> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-512-NEXT: store <8 x double> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA6]]
// CHECK-512-NEXT: ret void
//
void write_float64(struct struct_float64 *s, svfloat64_t x) {
@@ -139,21 +139,21 @@ void write_float64(struct struct_float64 *s, svfloat64_t x) {
// CHECK-128-LABEL: @read_bfloat16(
// CHECK-128-NEXT: entry:
// CHECK-128-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_BFLOAT16:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-128-NEXT: [[TMP0:%.*]] = load <8 x bfloat>, ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-128-NEXT: [[TMP0:%.*]] = load <8 x bfloat>, ptr [[Y]], align 16, !tbaa [[TBAA8:![0-9]+]]
// CHECK-128-NEXT: [[CAST_SCALABLE:%.*]] = tail call <vscale x 8 x bfloat> @llvm.vector.insert.nxv8bf16.v8bf16(<vscale x 8 x bfloat> undef, <8 x bfloat> [[TMP0]], i64 0)
// CHECK-128-NEXT: ret <vscale x 8 x bfloat> [[CAST_SCALABLE]]
//
// CHECK-256-LABEL: @read_bfloat16(
// CHECK-256-NEXT: entry:
// CHECK-256-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_BFLOAT16:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-256-NEXT: [[TMP0:%.*]] = load <16 x bfloat>, ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-256-NEXT: [[TMP0:%.*]] = load <16 x bfloat>, ptr [[Y]], align 16, !tbaa [[TBAA8:![0-9]+]]
// CHECK-256-NEXT: [[CAST_SCALABLE:%.*]] = tail call <vscale x 8 x bfloat> @llvm.vector.insert.nxv8bf16.v16bf16(<vscale x 8 x bfloat> undef, <16 x bfloat> [[TMP0]], i64 0)
// CHECK-256-NEXT: ret <vscale x 8 x bfloat> [[CAST_SCALABLE]]
//
// CHECK-512-LABEL: @read_bfloat16(
// CHECK-512-NEXT: entry:
// CHECK-512-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_BFLOAT16:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-512-NEXT: [[TMP0:%.*]] = load <32 x bfloat>, ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-512-NEXT: [[TMP0:%.*]] = load <32 x bfloat>, ptr [[Y]], align 16, !tbaa [[TBAA8:![0-9]+]]
// CHECK-512-NEXT: [[CAST_SCALABLE:%.*]] = tail call <vscale x 8 x bfloat> @llvm.vector.insert.nxv8bf16.v32bf16(<vscale x 8 x bfloat> undef, <32 x bfloat> [[TMP0]], i64 0)
// CHECK-512-NEXT: ret <vscale x 8 x bfloat> [[CAST_SCALABLE]]
//
@@ -165,21 +165,21 @@ svbfloat16_t read_bfloat16(struct struct_bfloat16 *s) {
// CHECK-128-NEXT: entry:
// CHECK-128-NEXT: [[CAST_FIXED:%.*]] = tail call <8 x bfloat> @llvm.vector.extract.v8bf16.nxv8bf16(<vscale x 8 x bfloat> [[X:%.*]], i64 0)
// CHECK-128-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_BFLOAT16:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-128-NEXT: store <8 x bfloat> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-128-NEXT: store <8 x bfloat> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA8]]
// CHECK-128-NEXT: ret void
//
// CHECK-256-LABEL: @write_bfloat16(
// CHECK-256-NEXT: entry:
// CHECK-256-NEXT: [[CAST_FIXED:%.*]] = tail call <16 x bfloat> @llvm.vector.extract.v16bf16.nxv8bf16(<vscale x 8 x bfloat> [[X:%.*]], i64 0)
// CHECK-256-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_BFLOAT16:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-256-NEXT: store <16 x bfloat> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-256-NEXT: store <16 x bfloat> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA8]]
// CHECK-256-NEXT: ret void
//
// CHECK-512-LABEL: @write_bfloat16(
// CHECK-512-NEXT: entry:
// CHECK-512-NEXT: [[CAST_FIXED:%.*]] = tail call <32 x bfloat> @llvm.vector.extract.v32bf16.nxv8bf16(<vscale x 8 x bfloat> [[X:%.*]], i64 0)
// CHECK-512-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_BFLOAT16:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-512-NEXT: store <32 x bfloat> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA2]]
+// CHECK-512-NEXT: store <32 x bfloat> [[CAST_FIXED]], ptr [[Y]], align 16, !tbaa [[TBAA8]]
// CHECK-512-NEXT: ret void
//
void write_bfloat16(struct struct_bfloat16 *s, svbfloat16_t x) {
@@ -193,7 +193,7 @@ void write_bfloat16(struct struct_bfloat16 *s, svbfloat16_t x) {
// CHECK-128-LABEL: @read_bool(
// CHECK-128-NEXT: entry:
// CHECK-128-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_BOOL:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-128-NEXT: [[TMP0:%.*]] = load <2 x i8>, ptr [[Y]], align 2, !tbaa [[TBAA2]]
+// CHECK-128-NEXT: [[TMP0:%.*]] = load <2 x i8>, ptr [[Y]], align 2, !tbaa [[TBAA10:![0-9]+]]
// CHECK-128-NEXT: [[CAST_SCALABLE:%.*]] = tail call <vscale x 2 x i8> @llvm.vector.insert.nxv2i8.v2i8(<vscale x 2 x i8> undef, <2 x i8> [[TMP0]], i64 0)
// CHECK-128-NEXT: [[TMP1:%.*]] = bitcast <vscale x 2 x i8> [[CAST_SCALABLE]] to <vscale x 16 x i1>
// CHECK-128-NEXT: ret <vscale x 16 x i1> [[TMP1]]
@@ -201,7 +201,7 @@ void write_bfloat16(struct struct_bfloat16 *s, svbfloat16_t x) {
// CHECK-256-LABEL: @read_bool(
// CHECK-256-NEXT: entry:
// CHECK-256-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_BOOL:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-256-NEXT: [[TMP0:%.*]] = load <4 x i8>, ptr [[Y]], align 2, !tbaa [[TBAA2]]
+// CHECK-256-NEXT: [[TMP0:%.*]] = load <4 x i8>, ptr [[Y]], align 2, !tbaa [[TBAA10:![0-9]+]]
// CHECK-256-NEXT: [[CAST_SCALABLE:%.*]] = tail call <vscale x 2 x i8> @llvm.vector.insert.nxv2i8.v4i8(<vscale x 2 x i8> undef, <4 x i8> [[TMP0]], i64 0)
// CHECK-256-NEXT: [[TMP1:%.*]] = bitcast <vscale x 2 x i8> [[CAST_SCALABLE]] to <vscale x 16 x i1>
// CHECK-256-NEXT: ret <vscale x 16 x i1> [[TMP1]]
@@ -209,7 +209,7 @@ void write_bfloat16(struct struct_bfloat16 *s, svbfloat16_t x) {
// CHECK-512-LABEL: @read_bool(
// CHECK-512-NEXT: entry:
// CHECK-512-NEXT: [[Y:%.*]] = getelementptr inbounds [[STRUCT_STRUCT_BOOL:%.*]], ptr [[S:%.*]], i64 0, i32 1
-// CHECK-512-NEXT: [[TMP0:%.*]] = load <8 x i8>, ptr [[Y]], align 2, !tbaa [[TBAA2]]
+//...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff 35c19fdde2583e74d940f6cd47b97a5c28bfe368 4c6d8097ce87ff87d096466487e57101f5c2b642 -- clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/CodeGenTBAA.cpp clang/lib/CodeGen/CodeGenTBAA.h clang/test/CXX/drs/dr158.cpp clang/test/CodeGen/attr-arm-sve-vector-bits-bitcast.c clang/test/CodeGen/attr-counted-by.c clang/test/CodeGen/attr-riscv-rvv-vector-bits-bitcast.c clang/test/CodeGen/sanitize-metadata-nosanitize.c clang/test/CodeGen/tbaa-pointers.c clang/test/CodeGen/tbaa-reference.cpp clang/test/CodeGen/tbaa-struct.cpp clang/test/CodeGen/union-tbaa1.c clang/test/CodeGenCXX/attr-likelihood-iteration-stmt.cpp clang/test/OpenMP/bug57757.cpp clang/test/OpenMP/nvptx_target_parallel_reduction_codegen_tbaa_PR46146.cpp clang/unittests/CodeGen/TBAAMetadataTest.cpp llvm/include/llvm/IR/Verifier.h llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp llvm/lib/IR/Verifier.cppView the diff from clang-format here.diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp b/clang/lib/CodeGen/CodeGenTBAA.cpp
index 9048491330..87859cdced 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -129,8 +129,8 @@ void CodeGenTBAA::appendPointeeName(llvm::raw_ostream &OS, const Type *Ty) {
// Non-builtin types are considered compatible if their tag matches.
OS << Ty->getUnqualifiedDesugaredType()
- ->getCanonicalTypeInternal()
- .getAsString();
+ ->getCanonicalTypeInternal()
+ .getAsString();
}
/// Return an LLVM TBAA metadata node appropriate for an access through
|
dybv-sc
force-pushed
the
clang-tbaa-impove
branch
from
8711499 to
3aae796
Compare
| @@ -1,7 +1,7 @@ | |||
| // RUN: %clang_cc1 -triple x86_64-linux -std=c++98 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s | |||
There was a problem hiding this comment.
Can you explain why -disable-llvm-passes is there, and why is can be removed now?
There was a problem hiding this comment.
Yes,
The matter is that if no llvm passes are run, IR is unoptimized and contains extra load and store instruction that operate with stack (storing and loading params there). Those extra loads should not be considered by the test, because it's goal is to check tbaa for specific load and store coming from line 22 and 24. Before I introduced more precise metadata on pointer types, all of that load and stores had same metadata, so test passed even if wrong load was considered. Now, this test should pick only load coming from line 22. So, to make things easier, I just enabled optimization passes and final IR contains only one load and one store instruction.
There was a problem hiding this comment.
This is okay as long as you're sure the optimization will be performed and you make sure your test only checks the pattern you want out of the optimizer and doesn't cover other details that could potentially be changed by unrelated LLVM patches. Otherwise, we generally aim to make IR-generation tests "unit" tests that specifically test the output of Clang's IR-generation. For output that's only included in optimized modes, like TBAA metadata, that generally requires enabling optimization (with -O<n>) and then disabling all the LLVM optimizations (with -disable-llvm-passes).
There was a problem hiding this comment.
I am sure that level 3 optimization should cut out all stack manipulation here leaving only one necessary load(const int *x = *p) and one necessary store(*q = 0). This test checks only presence of that load/store and metadata information, so I don't think it could be disrupted by unrelated patches.
There was a problem hiding this comment.
In general, this patch needs to be clearer about what rules it's actually enforcing. You're adding new command-line options, but users have to guess what they mean!
If you're going to be working on TBAA, would you mind adding a section to Clang's manual (UsersManual.rst) about type-based alias analysis? We should start by documenting our current behavior, then document the behavior of all these new options. To make this a less onerous request, let me suggest some starter text:
C and C++ forbid programmers from accessing objects using l-values that don't match the type of the object. By default, Clang takes advantage of these rules to decide that certain pointers cannot point to the same object; this is called *strict aliasing* or *type-based alias analysis* (TBAA). This can be completely disabled using the option ``-fno-strict-aliasing``. ``-fno-strict-aliasing`` is the default for ``clang-cl``.
When strict aliasing is enabled, Clang uses the type-based aliasing rules from the appropriate standard for the current language mode. In the C standard, the aliasing rules are laid out in section 6.5 (Expressions). In the C++ standard, the aliasing rules are laid out in [basic.lval]. For the most part, the C and C++ rules coincide and can be summarized as follows:
- An object can be accessed through an l-value of character type (e.g. ``char``).
- An object of integer type can be accessed through an l-value of different signedness; e.g. a ``signed short`` object can be accessed through an ``unsigned short`` l-value.
- Otherwise, objects can only be accessed through l-values of the type of the object.
For the exact rules, please consult the standards. Clang generally reserves the flexibility to take advantage of the exact rules for the current language mode, except as noted here:
- While C gives all character types the power to arbitrarily alias, C++ reserves this to ``char`` and ``unsigned char``. Clang relaxes this rule in C++ to match the C rule.
There are several ways to load from or store to an object as if it had a different type without violating the strict aliasing rule. The most explicit and portable is to ``memcpy`` between the object and an object of the desired type; for aliasing purposes, ``memcpy`` behaves as if it used loads and stores of character type. Clang also supports ``__attribute__((may_alias))``, which can be placed on a type declaration (such as a ``struct`` or ``typedef``) to give that type the equivalent aliasing power of a character type.
Clang uses an implementation model in which "sufficiently obvious" aliasing should override type-based assumptions. Strict aliasing means that Clang will assume that `int*` and `float*` parameters to a function do not alias, and it may reorder loads and stores to those parameters accordingly. However, if a `float*` parameter to a function is cast to `int*`, Clang will understand that the result of the cast still aliases the original parameter, and it should not reorder loads and stores to those pointers. This is only a best-effort attempt to avoid miscompiles, and programmers should generally still aim to write code which does not violate the strict aliasing rules, as discussed above.
An access to a member of an aggregate type (such as a ``struct``) is considered to also be an access to the aggregate. This means that there must also be an object of the aggregate type at that location, and it means that accesses into different aggregates cannot alias. This rule can be weakened to only consider the final accessed type using ``-fno-struct-path-tbaa``.
<document your new options here>
| @@ -216,6 +216,9 @@ ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, SRCK_Defa | |||
| CODEGENOPT(RelaxAll , 1, 0) ///< Relax all machine code instructions. | |||
| CODEGENOPT(RelaxedAliasing , 1, 0) ///< Set when -fno-strict-aliasing is enabled. | |||
| CODEGENOPT(StructPathTBAA , 1, 0) ///< Whether or not to use struct-path TBAA. | |||
| CODEGENOPT(UnionTBAA , 1, 0) ///< Whether or not to use struct-path TBAA on unions. | |||
| CODEGENOPT(PointerTBAA , 1, 0) ///< Whether or not to generate TBAA on pointers. | |||
| CODEGENOPT(ArrayTBAA , 1, 0) ///< Whether or not to generate TBAA on arrays. | |||
There was a problem hiding this comment.
Please match the style of surrounding lines, in which the commas are aligned.
| // about it. | ||
| EltTBAAInfo = isa<MemberExpr>(Array) && CGM.getCodeGenOpts().ArrayTBAA | ||
| ? ArrayLV.getTBAAInfo() | ||
| : CGM.getTBAAInfoForSubobject(ArrayLV, E->getType()); |
There was a problem hiding this comment.
Hmm. Okay, so if I understand correctly, the basic idea here is that TBAA for array types is just TBAA for the underlying element types, so if we have TBAA for an array l-value, whether it's struct-path TBAA or not, subscripting into the array can just preserve that TBAA onto the element. And then that gets complicated by the fact that we apparently actually use char as the TBAA for array types unless we're doing struct-path TBAA, so it's quite important that we actually override that or else we basically lose TBAA completely for these subscripts.
At the very least, this needs to be reflected in the comment; the overall situation is very non-obvious locally. But I would actually prefer that we just unconditionally change the TBAA we use for array types, because it seems unjustifiable. And as far as I can see, that should be an NFC refactor because it's not actually possible to do accesses directly of array type: arrays just decay into pointers in every context that would otherwise cause an access, and that decay ends up changing the TBAA we'd use anyway.
That is, I think you should consider just doing the array part of this patch unconditionally. The union and pointer changes are real increases in precision / risk, though, and should continue to be guarded with flags.
| @@ -4598,8 +4602,7 @@ LValue CodeGenFunction::EmitLValueForField(LValue base, | |||
| if (base.getTBAAInfo().isMayAlias() || | |||
| rec->hasAttr<MayAliasAttr>() || FieldType->isVectorType()) { | |||
| FieldTBAAInfo = TBAAAccessInfo::getMayAliasInfo(); | |||
| } else if (rec->isUnion()) { | |||
| // TODO: Support TBAA for unions. | |||
| } else if (rec->isUnion() && !CGM.getCodeGenOpts().UnionTBAA) { | |||
There was a problem hiding this comment.
You've checked that this is the right rule for unions? We can just record the union as a containing aggregate that happens to have all the different union members at offset 0?
There was a problem hiding this comment.
The goal of that patch is to re-utilize existing struct path TBAA metadata and amend it functionality to work for union types. Here I allow for that metadata to be generated for union members accesses, so yes there will be tbaa struct records with all members with offset 0. Later in code I adapted current struct-path-tbaa tree walkers to handle such situations where multiple fields have same offset.
There was a problem hiding this comment.
Well, I understand your intent and was asking whether you've put any effort in analyzing whether this rule actually matches the standard's aliasing rules. For what it's worth, I think it does, at least as currently written; IIRC, the committee has considered various weaker rules that permit union members to alias. But it's also true that this is going to make us enforce a stricter rule than we have been.
This patch is both implementing the stricter rules and enabling them by default, right? I think we should probably stage those separately, i.e. land this as an opt-in feature and then try turning it on by default in a follow-up commit. That way, if we see miscompiles from this, we can (temporarily) revert the small commit changing the default while we're analyzing that and not have to revert the whole implementation.
clang/lib/CodeGen/CodeGenTBAA.cpp
Outdated
| // pointers distinct. | ||
| if (Ty->isPointerType() || Ty->isReferenceType()) | ||
| return createScalarTypeNode("any pointer", getChar(), Size); | ||
| // Pointer types never alias if their pointee type is distinct. |
There was a problem hiding this comment.
This comment both is incorrect and doesn't match the code. What you seem to be implementing here is the C++ similar-type rule, in which what matters is whether the pointer type sub-structure matches while ignoring qualifiers.
Unfortunately, this rule seems to treat void* and char* as different types. That is wrong in C because those types are compatible, and we probably ought to use the C rule even in C++.
Also, in general, I would suggest that you write getPointeeName much more conservatively rather than assuming that you can just render an arbitrary type and it's going to be okay to treat different renderings as distinct for TBAA purposes. For example, this is going to treat pointers to different vector types as non-aliasing, which worries me a lot because vector programmers are often pretty fast-and-loose. You're also going to be stricter about ObjC than I'm comfortable with. I would strongly suggest just doing this rendering for specific kinds of types, like records, and otherwise having some kind of fallback to the any-pointer metadata.
There was a problem hiding this comment.
I agree that this may be a little bit too strict, but, as far as I could understand, according to C11 standard void* and char* are not the same (correct me if I am wrong):
From section 6.5 point 7:
an object shall have its stored value accessed only by an lvalue expression that has one of
the following types:88)
— a type compatible with the effective type of the object,
— a qualified version of a type compatible with the effective type of the object,
— a type that is the signed or unsigned type corresponding to the effective type of the
object,
— a type that is the signed or unsigned type corresponding to a qualified version of the
effective type of the object,
— an aggregate or union type that includes one of the aforementioned types among its
members (including, recursively, a member of a subaggregate or contained union), or
— a character type.
From all of above, the only way void* and char* alias if they are compatible, but they are not:
From section 6.2.7:
Two types have compatible type if their types are the same. Additional rules for
determining whether two types are compatible are described in 6.7.2 for type specifiers,
in 6.7.3 for type qualifiers, and in 6.7.6 for declarators.
And from section 6.7.6.1 for pointer declarators:
For two pointer types to be compatible, both shall be identically qualified and both shall
be pointers to compatible types.
void and char types are not compatible, because they are not the same. That means that void* and char* are not compatible and thus shall not alias.
So, from what I see in standard, pointer types to incompatible types should not alias. May be I got something wrong, so let's discuss it.
Also, now after I revisited standard, I see that getPointeeName is really not so accurate - it should consider type compatibility better(not only by exact match).
| do { | ||
| PtrDepth++; | ||
| Ty = Ty->getPointeeType().getTypePtr(); | ||
| } while (!Ty->getPointeeType().isNull()); |
There was a problem hiding this comment.
getPointeeType() will look through a lot of types that you probably don't want to look through, including member pointers. You should write this to specifically look for pointers. (References can't occur in nested positions.)
There was a problem hiding this comment.
Actually I want to look for member pointers. In [conv.qual] of C++ standard mentioned:
each Pi is “pointer to” (9.3.4.2), “pointer to member of
class Ci of type” (9.3.4.4), “array of Ni”, or “array of unknown bound of” (9.3.4.5
Where Pi is i-th inderection in qualification decomposition. I want to consider all those types of indirection.
I think, all I do if I get rid of getPointeeType() is just move most of it's checks outside.
There was a problem hiding this comment.
You're right, you do need to look through member pointers here, although this means you're also treating member-pointer structure as if it were just pointer structure. I guess that's okay for now.
clang/lib/CodeGen/CodeGenTBAA.cpp
Outdated
| } | ||
|
|
||
| if (Ty->isIncompleteType()) | ||
| return "<incomplete type>"; |
There was a problem hiding this comment.
Why are incomplete types treated differently here?
There was a problem hiding this comment.
Removed. I was uncertain in a moment what to do in that case, but after revisiting C/C++ standard I learned that there is no difference between complete and incomplete types when considering their similarity/compatibility.
Ah, you're right, I had misremembered this. 6.2.5p31 requires them to have the same representation as each other, and there's a footnote about this being meant to imply "interchangeability as arguments to functions, return values from functions, and members of unions", but it doesn't go so far as to make them formally compatible. And even if we wanted to read that as implying a sort of semi-compatibility that ought to affect aliasing, the same paragraph goes on to say that all pointers to structs must have the same representation, and presumably we wouldn't want that to affect aliasing. Alright, I withdraw that comment; it's not formally required. We may need to be more conservative about the aliasing of |
dybv-sc
force-pushed
the
clang-tbaa-impove
branch
from
3aae796 to
264d67d
Compare
|
@rjmccall , I updated part of commit that handles pointers. Added comment with C/C++ standard references to explain my decisions. Could you please review it again? |
dybv-sc
force-pushed
the
clang-tbaa-impove
branch
from
264d67d to
6680952
Compare
|
(As usual, please make any LLVM changes separately from Clang changes, especially if they affect IR design.) |
dybv-sc
force-pushed
the
clang-tbaa-impove
branch
from
6680952 to
9c095cc
Compare
I splitted commit into 2 parts for llvm and clang. Should I make separate PRs for them as well? |
|
I made separate PR for llvm's part: #76356 |
| @@ -4598,8 +4602,7 @@ LValue CodeGenFunction::EmitLValueForField(LValue base, | |||
| if (base.getTBAAInfo().isMayAlias() || | |||
| rec->hasAttr<MayAliasAttr>() || FieldType->isVectorType()) { | |||
| FieldTBAAInfo = TBAAAccessInfo::getMayAliasInfo(); | |||
| } else if (rec->isUnion()) { | |||
| // TODO: Support TBAA for unions. | |||
| } else if (rec->isUnion() && !CGM.getCodeGenOpts().UnionTBAA) { | |||
There was a problem hiding this comment.
Well, I understand your intent and was asking whether you've put any effort in analyzing whether this rule actually matches the standard's aliasing rules. For what it's worth, I think it does, at least as currently written; IIRC, the committee has considered various weaker rules that permit union members to alias. But it's also true that this is going to make us enforce a stricter rule than we have been.
This patch is both implementing the stricter rules and enabling them by default, right? I think we should probably stage those separately, i.e. land this as an opt-in feature and then try turning it on by default in a follow-up commit. That way, if we see miscompiles from this, we can (temporarily) revert the small commit changing the default while we're analyzing that and not have to revert the whole implementation.
clang/lib/CodeGen/CodeGenTBAA.cpp
Outdated
| return createScalarTypeNode("any pointer", getChar(), Size); | ||
| // | ||
| // In C11 for two pointer type to alias it is required for them to be | ||
| // compatible [section 6.5 p7]. |
There was a problem hiding this comment.
This comment is pretty good, but it'd be better if we had a good comment on the function itself, and then this could be specifically in the context of that. Please add something like this as a doc comment on the entire function:
/// Return an LLVM TBAA metadata node appropriate for an access through
/// an l-value of the given type. Type-based alias analysis takes advantage
/// of the following rules from the language standards:
///
/// C 6.5p7:
/// An object shall have its stored value accessed only by an lvalue
/// expression that has one of the following types:
/// - a type compatible with the effective type of the object,
/// - a qualified version of a type compatible with the effective
/// type of the object,
/// - a type that is the signed or unsigned type corresponding
/// to the effective type of the object,
/// - a type that is the signed or unsigned type corresponding
/// to a qualified version of the effective type of the object,
/// - an aggregate or union type that includes one of the
/// aforementioned types among its members (including,
/// recursively, a member of a subaggregate or contained union), or
/// - a character type.
///
/// C++ [basic.lval]p11:
/// If a program attempts to access the stored value of an object
/// through a glvalue whose type is not similar to one of the following
/// types the behavior is undefined:
/// - the dynamic type of the object,
/// - a type that is the signed or unsigned type corresponding
/// to the dynamic type of the object, or
/// - a char, unsigned char, or std::byte type.
///
/// The C and C++ rules about effective/dynamic type are broadly similar
/// and permit memory to be reused with a different type. C does not have
/// an explicit operation to change the effective type of memory; any store
/// can do it. While C++ arguably does have such an operation (the standard
/// global `operator new(void*, size_t)`), in practice it is important to
/// be just as permissive as C. We therefore treat all stores as being able to
/// change the effective type of memory, regardless of language mode. That is,
/// loads have both a precondition and a postcondition on the effective
/// type of the memory, but stores only have a postcondition. This imposes
/// an inherent limitation that TBAA can only be used to reorder loads
/// before stores. This is quite restrictive, but we don't have much of a
/// choice. In practice, hoisting loads is the most important optimization
/// for alias analysis to enable anyway.
///
/// Therefore, given a load (and its precondition) and an earlier store
/// (and its postcondition), the question posed to TBAA is whether there
/// exists a type that is consistent with both accesses. If there isn't,
/// it's fine to hoist the load because either the memory is non-overlapping
/// or the precondition on the load is wrong (which would be UB).
///
/// LLVM TBAA says that two accesses with TBAA metadata nodes may alias if:
/// - the metadata nodes are the same,
/// - one of the metadata nodes is a base of the other (this can be
/// recursive, but it has to be the original node that's a base,
/// not just that the nodes have a common base), or
/// - one of the metadata nodes is a `tbaa.struct` node (the access
/// necessarily being a `memcpy`) with a subobject node that would
/// be allowed to alias with the other.
///
/// Our job here is to produce metadata nodes that will never say that
/// an alias is not allowed when there exists a type that would be consistent
/// with the types of the accesses from which the nodes were produced.
///
/// The last clause in both language rules permits character types to
/// alias objects of any type. We handle this by converting all character
/// types (as well as `std::byte` and types with the `mayalias` attribute)
/// to a single metadata node (the `char` node), then making sure that
/// that node is a base of every other metadata node we generate.
/// We can always just conservatively use this node if we aren't otherwise
/// sure how to implement the language rules for a type.
///
/// Read literally, the C rule for aggregates permits an aggregate l-value
/// (e.g. of type `struct { int x; }`) to be used to access an object that
/// is not part of an aggregate object of that type (e.g. a local variable
/// of type `int`). That case is perhaps sensical, but it would also permit
/// e.g. an l-value of type `struct { int x; float f; }` to be used to
/// access an object of type `float`, which is nonsense. We interpret this
/// clause as just intending to permit objects to be accessed through an
/// l-value that properly references a containing object.
///
/// C++ does not have an explicit rule for aggregates because in C++
/// a non-member access to an aggregate l-value is always a call to a
/// constructor or assignment operator, which then accesses all the
/// subobjects. In general, however, our interpretation of member
/// accesses is that they are also an access to the containing object
/// and therefore require such an object to exist at that address;
/// this permits us to just use the C rule for the accesses done by
/// trivial copy/move constructors/operators.
///
/// Both C and C++ permit some qualification differences. In C, however,
/// qualification can only differ at the outermost level, whereas C++
/// allows qualification to differ in nested positions through the
/// similar-types rule. This means that e.g. an l-value of type
/// `const float *` is not permitted to access an object of type
/// `float *` in C, but it is in C++. We use the C++ rule
/// unconditionally; the C rule is needlessly strict and frequently
/// violated in practice by code that we don't want to say is wrong.
/// We implement this by just discarding type qualifiers within pointer-like
/// types when deriving TBAA nodes; basically, we produce the TBAA node
/// for the type that is unqualified at all the recursive positions
/// considered by the C++ similar type rule. The implementation
/// doesn't actually construct this recursively-qualified type as a
/// `QualType`; it just ignores qualifiers when recursing into types.
///
/// The similar-type rule only really applies to the standard CVR
/// qualifiers, which never affect representations. Qualifiers such as
/// address spaces that may involve a representation difference would
/// be totally appropriate to distinguish for TBAA purposes. However,
/// the current implementation just discards all qualifiers.
///
/// We handle the signed/unsigned clause by just making unsigned types
/// use the the metadata node for the signed variant of the type. In the
/// language rules, this only applies at the outermost level, and e.g. an
/// l-value of type `signed int *` is not permitted to alias an object of
/// type `unsigned int *`. We choose not to distinguish those types when
/// pointer-type TBAA is enabled, however.
///
/// After discarding qualifiers and signedness differences as above,
/// the language rules come down to whether the types are compatible
/// (in C) or identical (in C++). Even in C, most types are compatible
/// only with themselves. The exceptions will be considered in the cases
/// below.
and then this comment can just be something like this:
// When PointerTBAA is disabled, all pointers and references use the same
// "any pointer" TBAA node. Otherwise, we generate a type-specific TBAA
// node and use the "any pointer" node as its base for compatibility between
// TUs with different settings. To implement the C++ similar-type rules
// (which we also adopt in C), we need to ignore qualifiers on the
// pointee type, and that has to be done recursively if the pointee type
// is itself a pointer-like type.
//
// Currently we ignore the differences between pointer-like types and just
// and use this tag for the type: `p<pointer depth> <inner type tag>`.
// This means we give e.g. `char **` and `char A::**` the same TBAA tag.
There was a problem hiding this comment.
Great idea! Thanks for help with it. Added those comments.
clang/lib/CodeGen/CodeGenTBAA.cpp
Outdated
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| // Give unique tag for compatible types. | ||
| std::string CodeGenTBAA::getPointeeName(const Type *Ty) { |
There was a problem hiding this comment.
Since this is always being used to build a larger string, could you make this append to a raw_ostream?
I guess Ty is guaranteed to be a canonical type here, because that's apparently a precondition of getTypeInfoHelper. Could you assert that?
There was a problem hiding this comment.
Changed function to append to raw_ostream.
Ty here is coming from getPointeeType() so it may be not canonical, right?
| do { | ||
| PtrDepth++; | ||
| Ty = Ty->getPointeeType().getTypePtr(); | ||
| } while (!Ty->getPointeeType().isNull()); |
There was a problem hiding this comment.
You're right, you do need to look through member pointers here, although this means you're also treating member-pointer structure as if it were just pointer structure. I guess that's okay for now.
|
Thanks for working on this! I think it would be good to split this up into multiple distinct parts for the different improvements. I put up a patch to support distinct metadata for distinct pointers a while ago, which I just moved to GH: #76261. The discussion on Phabricator has some interesting points and one of the concerns was that it is very difficult already to detect type violations in source code, which makes adopting more powerful TBAA features quite difficult. There are some tooling improvements we can make here, including a sanitizer for types, which would be good to get rolling again, see https://discourse.llvm.org/t/reviving-typesanitizer-a-sanitizer-to-catch-type-based-aliasing-violations/66092 TBAA union support and potential issues have been discussed in detail a number of years ago on the old mailing list (llvm-dev), did you have a look at those threads? |
dybv-sc
force-pushed
the
clang-tbaa-impove
branch
from
9c095cc to
58a082e
Compare
| // RUN: %clang_cc1 -triple x86_64-linux -std=c++98 %s -O3 -pedantic-errors -emit-llvm -o - | FileCheck %s | ||
| // RUN: %clang_cc1 -triple x86_64-linux -std=c++11 %s -O3 -pedantic-errors -emit-llvm -o - | FileCheck %s | ||
| // RUN: %clang_cc1 -triple x86_64-linux -std=c++14 %s -O3 -pedantic-errors -emit-llvm -o - | FileCheck %s | ||
| // RUN: %clang_cc1 -triple x86_64-linux -std=c++1z %s -O3 -pedantic-errors -emit-llvm -o - | FileCheck %s | ||
|
|
||
| // dr158: yes |
There was a problem hiding this comment.
Can you mention this DR in dr1xx.cpp, like it has been done recently for other tests that are placed in their own files?
llvm-project/clang/test/CXX/drs/dr1xx.cpp
Line 286 in 114e6d7
There was a problem hiding this comment.
As I see it is already mentioned:
https://github.com/llvm/llvm-project/blob/114e6d7ba02f090117f2cb1ffeb9027cf80f335b/clang/test/CXX/drs/dr1xx.cpp#L792C1-L792C25
There was a problem hiding this comment.
I'm sorry I missed that you're not writing a new test.
Hi, Thanks for sharing those materials. Actually I didn't look into any discussions considering standard violations, so I would be glad if you share those threads. Those standard violations worry me too, so as @rjmccall suggested, I put union TBAA under the option that is disabled by default. I am also considering to put pointer TBAA under option too, if it raises enough concerns too. |
dybv-sc
force-pushed
the
clang-tbaa-impove
branch
from
58a082e to
bb0d400
Compare
Support for multiple fields to have same offset in TBAA struct-path metadata nodes. Primary goal is to support union-like structures to participate in TBAA struct-path resolution.
dybv-sc
force-pushed
the
clang-tbaa-impove
branch
from
bb0d400 to
5aacc23
Compare
Options to disable new behaviour:
-Xclang -no-pointer-tbaa
-Xclang -no-array-tbaa
Following option enables unions to participate in struct-path TBAA:
-Xclang -union-tbaa
dybv-sc
force-pushed
the
clang-tbaa-impove
branch
from
5aacc23 to
4c6d809
Compare
Let's please file a bug once this is merged so we remember to come back to that (one for each, I guess). |
Options to disable new behaviour:
-Xclang -no-union-tbaa
-Xclang -no-pointer-tbaa
-Xclang -no-array-tbaa
To enable union struct path tbaa there was need to update the way llvm handles struct path tbaa metadata. Multiple fields with same offset are now allowed. To properly resolve access type, struct path tbaa visitor considers each field with given offset. This patch works both for new and old struct path TBAA.
Also this patch enables tbaa aware array and pointer accesses by default.