-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[Clang][IR] add TBAA metadata on pointer, union and array types. #75177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4285,7 +4285,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); | ||
|
@@ -4803,8 +4807,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
FieldTBAAInfo = TBAAAccessInfo::getMayAliasInfo(); | ||
} else { | ||
// If no base type been assigned for the base access, then try to generate | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,154 @@ 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; | ||
} | ||
|
||
// Appends unique tag for compatible pointee types. | ||
void CodeGenTBAA::appendPointeeName(llvm::raw_ostream &OS, const Type *Ty) { | ||
// Although type compatibilty in C standard requires cv-qualification | ||
// match and exact type match, here more relaxed rules are applied. | ||
// | ||
// For built-in types consider them 'compatible' if their respective | ||
// TBAA metadata tag is same(e.g. that makes 'int' and 'unsigned' | ||
// compatible). | ||
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"); | ||
OS << cast<llvm::MDString>(Op)->getString().str(); | ||
} | ||
|
||
// Non-builtin types are considered compatible if their tag matches. | ||
OS << Ty->getUnqualifiedDesugaredType() | ||
->getCanonicalTypeInternal() | ||
.getAsString(); | ||
} | ||
|
||
/// 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. | ||
llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type *Ty) { | ||
uint64_t Size = Context.getTypeSizeInChars(Ty).getQuantity(); | ||
|
||
|
@@ -184,13 +325,40 @@ 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); | ||
// | ||
// 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. | ||
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()->getUnqualifiedDesugaredType(); | ||
// Any array-like type is considered a pointer-to qualification. | ||
if (Ty && Ty->isArrayType()) { | ||
Ty = Ty->getAsArrayTypeUnsafe()->getElementType().getTypePtr(); | ||
} | ||
} while (!Ty->getPointeeType().isNull()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I want to look for member pointers. In [conv.qual] of C++ standard mentioned:
Where Pi is i-th inderection in qualification decomposition. I want to consider all those types of indirection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
std::string PtrName; | ||
llvm::raw_string_ostream OS{PtrName}; | ||
OS << "p" << PtrDepth << " "; | ||
appendPointeeName(OS, 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 +409,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 +521,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 +546,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 +586,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(); | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||
// 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 | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you mention this DR in llvm-project/clang/test/CXX/drs/dr1xx.cpp Line 286 in 114e6d7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I see it is already mentioned: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry I missed that you're not writing a new test. |
||||
|
||||
|
@@ -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; | ||||
} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.