llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen Author: Akira Hatanaka (ahatanak) <details> <summary>Changes</summary> This fixes two bugs in GenBinaryFunc::visitVolatileTrivial (CGNonTrivialStruct.cpp): - It was passing the field's type instead of the containing struct's type to MakeAddrLValue. - It was calling EmitLoadOfLValue and EmitStoreThroughLValue even when the lvalue was an aggregate or a _Complex type. The functions end up calling EmitLoadOfScalar/EmitStoreOfScalar, which are meant to be used only for scalar values. Both caused CodeGen to emit incorrect TBAA metadata, which made the IR verifier fail. rdar://187077389 --- Full diff: https://github.com/llvm/llvm-project/pull/226615.diff 2 Files Affected: - (modified) clang/lib/CodeGen/CGNonTrivialStruct.cpp (+33-13) - (added) clang/test/CodeGen/ptrauth-nontrivial-c-struct-volatile.c (+75) ``````````diff diff --git a/clang/lib/CodeGen/CGNonTrivialStruct.cpp b/clang/lib/CodeGen/CGNonTrivialStruct.cpp index 0a383c8f919d9..3af65c8c9eb6c 100644 --- a/clang/lib/CodeGen/CGNonTrivialStruct.cpp +++ b/clang/lib/CodeGen/CGNonTrivialStruct.cpp @@ -516,7 +516,7 @@ template <class Derived> struct GenFuncBase { CodeGenFunction *CGF = nullptr; }; -template <class Derived, bool IsMove> +template <class Derived, bool IsMove, bool IsCtor> struct GenBinaryFunc : CopyStructVisitor<Derived, IsMove>, GenFuncBase<Derived> { GenBinaryFunc(ASTContext &Ctx) : CopyStructVisitor<Derived, IsMove>(Ctx) {} @@ -563,13 +563,16 @@ struct GenBinaryFunc : CopyStructVisitor<Derived, IsMove>, CanQualType RT = this->CGF->getContext().getCanonicalTagType(FD->getParent()); llvm::Type *Ty = this->CGF->ConvertType(RT); + // FT is volatile-qualified, so propagate that onto the base lvalue's + // type. + QualType QT = QualType(RT).withVolatile(); Address DstAddr = this->getAddrWithOffset(Addrs[DstIdx], Offset); LValue DstBase = - this->CGF->MakeAddrLValue(DstAddr.withElementType(Ty), FT); + this->CGF->MakeAddrLValue(DstAddr.withElementType(Ty), QT); DstLV = this->CGF->EmitLValueForField(DstBase, FD); Address SrcAddr = this->getAddrWithOffset(Addrs[SrcIdx], Offset); LValue SrcBase = - this->CGF->MakeAddrLValue(SrcAddr.withElementType(Ty), FT); + this->CGF->MakeAddrLValue(SrcAddr.withElementType(Ty), QT); SrcLV = this->CGF->EmitLValueForField(SrcBase, FD); } else { llvm::Type *Ty = this->CGF->ConvertTypeForMem(FT); @@ -578,8 +581,25 @@ struct GenBinaryFunc : CopyStructVisitor<Derived, IsMove>, DstLV = this->CGF->MakeAddrLValue(DstAddr, FT); SrcLV = this->CGF->MakeAddrLValue(SrcAddr, FT); } - RValue SrcVal = this->CGF->EmitLoadOfLValue(SrcLV, SourceLocation()); - this->CGF->EmitStoreThroughLValue(SrcVal, DstLV); + // Load the value from the source. For the aggregate case, load directly + // into DstLV's own storage via an AggValueSlot. + AggValueSlot Slot = AggValueSlot::forLValue( + DstLV, AggValueSlot::IsDestructed, AggValueSlot::DoesNotNeedGCBarriers, + IsCtor ? AggValueSlot::IsNotAliased : AggValueSlot::IsAliased, + AggValueSlot::DoesNotOverlap); + RValue SrcVal = + this->CGF->EmitLoadOfAnyValue(SrcLV, Slot, SourceLocation()); + switch (CodeGenFunction::getEvaluationKind(FT)) { + case TEK_Aggregate: + break; // Already copied into DstLV via Slot above. + case TEK_Complex: + this->CGF->EmitStoreOfComplex(SrcVal.getComplexVal(), DstLV, + /*isInit=*/IsCtor); + break; + case TEK_Scalar: + this->CGF->EmitStoreThroughLValue(SrcVal, DstLV, /*isInit=*/IsCtor); + break; + } } void visitPtrAuth(QualType FT, const FieldDecl *FD, CharUnits CurStackOffset, std::array<Address, 2> Addrs) { @@ -692,9 +712,9 @@ struct GenDefaultInitialize } }; -struct GenCopyConstructor : GenBinaryFunc<GenCopyConstructor, false> { +struct GenCopyConstructor : GenBinaryFunc<GenCopyConstructor, false, true> { GenCopyConstructor(ASTContext &Ctx) - : GenBinaryFunc<GenCopyConstructor, false>(Ctx) {} + : GenBinaryFunc<GenCopyConstructor, false, true>(Ctx) {} void visitARCStrong(QualType QT, const FieldDecl *FD, CharUnits CurStructOffset, std::array<Address, 2> Addrs) { @@ -722,9 +742,9 @@ struct GenCopyConstructor : GenBinaryFunc<GenCopyConstructor, false> { } }; -struct GenMoveConstructor : GenBinaryFunc<GenMoveConstructor, true> { +struct GenMoveConstructor : GenBinaryFunc<GenMoveConstructor, true, true> { GenMoveConstructor(ASTContext &Ctx) - : GenBinaryFunc<GenMoveConstructor, true>(Ctx) {} + : GenBinaryFunc<GenMoveConstructor, true, true>(Ctx) {} void visitARCStrong(QualType QT, const FieldDecl *FD, CharUnits CurStructOffset, std::array<Address, 2> Addrs) { @@ -754,9 +774,9 @@ struct GenMoveConstructor : GenBinaryFunc<GenMoveConstructor, true> { } }; -struct GenCopyAssignment : GenBinaryFunc<GenCopyAssignment, false> { +struct GenCopyAssignment : GenBinaryFunc<GenCopyAssignment, false, false> { GenCopyAssignment(ASTContext &Ctx) - : GenBinaryFunc<GenCopyAssignment, false>(Ctx) {} + : GenBinaryFunc<GenCopyAssignment, false, false>(Ctx) {} void visitARCStrong(QualType QT, const FieldDecl *FD, CharUnits CurStructOffset, std::array<Address, 2> Addrs) { @@ -785,9 +805,9 @@ struct GenCopyAssignment : GenBinaryFunc<GenCopyAssignment, false> { } }; -struct GenMoveAssignment : GenBinaryFunc<GenMoveAssignment, true> { +struct GenMoveAssignment : GenBinaryFunc<GenMoveAssignment, true, false> { GenMoveAssignment(ASTContext &Ctx) - : GenBinaryFunc<GenMoveAssignment, true>(Ctx) {} + : GenBinaryFunc<GenMoveAssignment, true, false>(Ctx) {} void visitARCStrong(QualType QT, const FieldDecl *FD, CharUnits CurStructOffset, std::array<Address, 2> Addrs) { diff --git a/clang/test/CodeGen/ptrauth-nontrivial-c-struct-volatile.c b/clang/test/CodeGen/ptrauth-nontrivial-c-struct-volatile.c new file mode 100644 index 0000000000000..14d50fe369f68 --- /dev/null +++ b/clang/test/CodeGen/ptrauth-nontrivial-c-struct-volatile.c @@ -0,0 +1,75 @@ +// RUN: %clang_cc1 -triple arm64-apple-ios -fblocks -fptrauth-calls -fptrauth-returns -fptrauth-intrinsics -O1 -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s + +#define AQ __ptrauth(1, 1, 50) + +struct HasScalarField { + void (* AQ fp)(void); + int f3; +}; + +// CHECK-LABEL: define{{.*}} void @copyScalar( +// CHECK: call void @[[COPY_ASSIGNMENT_SCALAR:__copy_assignment[a-zA-Z0-9_]*]]( +void copyScalar(volatile struct HasScalarField *dst, + volatile struct HasScalarField *src) { + *dst = *src; +} + +// CHECK: define{{.*}} void @[[COPY_ASSIGNMENT_SCALAR]]( +// CHECK: %[[DST:.*]] = getelementptr inbounds nuw %struct.HasScalarField, ptr %{{.*}}, i32 0, i32 1 +// CHECK: %[[SRC:.*]] = getelementptr inbounds nuw %struct.HasScalarField, ptr %{{.*}}, i32 0, i32 1 +// CHECK: load volatile i32, ptr %[[SRC]], align 8, !tbaa ![[TBAA_SCALAR:[0-9]+]]{{$}} +// CHECK: store volatile i32 %{{.*}}, ptr %[[DST]], align 8, !tbaa ![[TBAA_SCALAR]]{{$}} +// CHECK: ret void + +struct Inner { + int a; + int b; +}; + +struct HasAggregateField { + void (* AQ fp)(void); + struct Inner f3; +}; + +// CHECK-LABEL: define{{.*}} void @copyAggregate( +// CHECK: call void @[[COPY_ASSIGNMENT_AGGREGATE:__copy_assignment[a-zA-Z0-9_]*]]( +void copyAggregate(volatile struct HasAggregateField *dst, + volatile struct HasAggregateField *src) { + *dst = *src; +} + +// CHECK: define{{.*}} void @[[COPY_ASSIGNMENT_AGGREGATE]]( +// CHECK: %[[DST:.*]] = getelementptr inbounds nuw %struct.HasAggregateField, ptr %{{.*}}, i32 0, i32 1 +// CHECK: %[[SRC:.*]] = getelementptr inbounds nuw %struct.HasAggregateField, ptr %{{.*}}, i32 0, i32 1 +// CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 8 %[[DST]], ptr align 8 %[[SRC]], i64 8, i1 true), !tbaa.struct ![[TBAA_STRUCT:[0-9]+]]{{$}} +// CHECK: ret void + +struct HasComplexField { + void (* AQ fp)(void); + _Complex double f3; +}; + +// CHECK-LABEL: define{{.*}} void @copyComplex( +// CHECK: call void @[[COPY_ASSIGNMENT_COMPLEX:__copy_assignment[a-zA-Z0-9_]*]]( +void copyComplex(volatile struct HasComplexField *dst, + volatile struct HasComplexField *src) { + *dst = *src; +} + +// _Complex fields aren't given any TBAA at all. +// CHECK: define{{.*}} void @[[COPY_ASSIGNMENT_COMPLEX]]( +// CHECK: %[[SRC_REAL:.*]] = load volatile double, ptr %{{.*}}, align 8{{$}} +// CHECK: %[[SRC_IMAG:.*]] = load volatile double, ptr %{{.*}}, align 8{{$}} +// CHECK: store volatile double %[[SRC_REAL]], ptr %{{.*}}, align 8{{$}} +// CHECK: store volatile double %[[SRC_IMAG]], ptr %{{.*}}, align 8{{$}} +// CHECK: ret void + +// CHECK: ![[TBAA_INT:[0-9]+]] = !{!"int", ![[TBAA_CHAR:[0-9]+]], i64 0} +// CHECK: ![[TBAA_CHAR]] = !{!"omnipotent char", ![[TBAA_DOMAIN:[0-9]+]], i64 0} +// CHECK: ![[TBAA_DOMAIN]] = !{!"Simple C/C++ TBAA"} +// CHECK: ![[TBAA_PTR:[0-9]+]] = !{!"any pointer", ![[TBAA_CHAR]], i64 0} +// CHECK: ![[TBAA_SCALAR]] = !{![[TBAA_SCALAR_BASE:[0-9]+]], ![[TBAA_INT]], i64 8} +// CHECK: ![[TBAA_SCALAR_BASE]] = !{!"HasScalarField", ![[TBAA_PTR]], i64 0, ![[TBAA_INT]], i64 8} + +// CHECK: ![[TBAA_STRUCT]] = !{i64 0, i64 4, ![[TBAA_STRUCT_TAG:[0-9]+]], i64 4, i64 4, ![[TBAA_STRUCT_TAG]]} +// CHECK: ![[TBAA_STRUCT_TAG]] = !{![[TBAA_INT]], ![[TBAA_INT]], i64 0} `````````` </details> https://github.com/llvm/llvm-project/pull/226615 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
