llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen Author: VASU SHARMA (vasu-the-sharma) <details> <summary>Changes</summary> --- Full diff: https://github.com/llvm/llvm-project/pull/175032.diff 3 Files Affected: - (modified) clang/lib/CodeGen/CGExprAgg.cpp (+15-28) - (modified) clang/lib/CodeGen/CGExprCXX.cpp (+14-15) - (added) clang/test/CodeGen/ubsan-aggregate-null-align.c (+63) ``````````diff diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp index 7cc4d6c8f06f6..919e510a82af0 100644 --- a/clang/lib/CodeGen/CGExprAgg.cpp +++ b/clang/lib/CodeGen/CGExprAgg.cpp @@ -1292,45 +1292,29 @@ static bool isBlockVarRef(const Expr *E) { void AggExprEmitter::VisitBinAssign(const BinaryOperator *E) { ApplyAtomGroup Grp(CGF.getDebugInfo()); - // For an assignment to work, the value on the right has - // to be compatible with the value on the left. assert(CGF.getContext().hasSameUnqualifiedType(E->getLHS()->getType(), E->getRHS()->getType()) - && "Invalid assignment"); + && "Invalid assignment"); - // If the LHS might be a __block variable, and the RHS can - // potentially cause a block copy, we need to evaluate the RHS first - // so that the assignment goes the right place. - // This is pretty semantically fragile. if (isBlockVarRef(E->getLHS()) && E->getRHS()->HasSideEffects(CGF.getContext())) { - // Ensure that we have a destination, and evaluate the RHS into that. EnsureDest(E->getRHS()->getType()); Visit(E->getRHS()); - - // Now emit the LHS and copy into it. LValue LHS = CGF.EmitCheckedLValue(E->getLHS(), CodeGenFunction::TCK_Store); - // That copy is an atomic copy if the LHS is atomic. if (LHS.getType()->isAtomicType() || CGF.LValueIsSuitableForInlineAtomic(LHS)) { CGF.EmitAtomicStore(Dest.asRValue(), LHS, /*isInit*/ false); return; } - - EmitCopy(E->getLHS()->getType(), - AggValueSlot::forLValue(LHS, AggValueSlot::IsDestructed, - needsGC(E->getLHS()->getType()), - AggValueSlot::IsAliased, - AggValueSlot::MayOverlap), - Dest); + EmitFinalDestCopy(E->getLHS()->getType(), LHS); return; } - LValue LHS = CGF.EmitLValue(E->getLHS()); + // ✅ FIX: Use EmitCheckedLValue for LHS + LValue LHS = CGF.EmitCheckedLValue(E->getLHS(), CodeGenFunction::TCK_Store); - // If we have an atomic type, evaluate into the destination and then - // do an atomic copy. + // ✅ RE-ADD: Original atomic handling logic if (LHS.getType()->isAtomicType() || CGF.LValueIsSuitableForInlineAtomic(LHS)) { EnsureDest(E->getRHS()->getType()); @@ -1339,20 +1323,23 @@ void AggExprEmitter::VisitBinAssign(const BinaryOperator *E) { return; } - // Codegen the RHS so that it stores directly into the LHS. + // ✅ FIX: Handle RHS based on LValue/RValue AggValueSlot LHSSlot = AggValueSlot::forLValue( LHS, AggValueSlot::IsDestructed, needsGC(E->getLHS()->getType()), AggValueSlot::IsAliased, AggValueSlot::MayOverlap); - // A non-volatile aggregate destination might have volatile member. - if (!LHSSlot.isVolatile() && - CGF.hasVolatileMember(E->getLHS()->getType())) - LHSSlot.setVolatile(true); - CGF.EmitAggExpr(E->getRHS(), LHSSlot); + if (E->getRHS()->isLValue()) { + LValue RHS = CGF.EmitCheckedLValue(E->getRHS(), CodeGenFunction::TCK_Load); + CGF.EmitAggregateCopy(LHS, RHS, E->getType(), Dest.isVolatile()); + } else { + if (!LHSSlot.isVolatile() && CGF.hasVolatileMember(E->getLHS()->getType())) + LHSSlot.setVolatile(true); + CGF.EmitAggExpr(E->getRHS(), LHSSlot); + } - // Copy into the destination if the assignment isn't ignored. EmitFinalDestCopy(E->getType(), LHS); + // ✅ RE-ADD: Original Nontrivial C struct destruction logic if (!Dest.isIgnored() && !Dest.isExternallyDestructed() && E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) CGF.pushDestroy(QualType::DK_nontrivial_c_struct, Dest.getAddress(), diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp index ce2ed9026fa1f..eae27a6a3f1c8 100644 --- a/clang/lib/CodeGen/CGExprCXX.cpp +++ b/clang/lib/CodeGen/CGExprCXX.cpp @@ -267,7 +267,7 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr( if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(CE)) { if (OCE->isAssignmentOp()) { if (TrivialAssignment) { - TrivialAssignmentRHS = EmitLValue(CE->getArg(1)); + TrivialAssignmentRHS = EmitCheckedLValue(CE->getArg(1), TCK_Load); } else { RtlArgs = &RtlArgStorage; EmitCallArgs(*RtlArgs, MD->getType()->castAs<FunctionProtoType>(), @@ -309,22 +309,21 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr( if (TrivialForCodegen) { if (isa<CXXDestructorDecl>(MD)) return RValue::get(nullptr); + } - if (TrivialAssignment) { - // We don't like to generate the trivial copy/move assignment operator - // when it isn't necessary; just produce the proper effect here. - // It's important that we use the result of EmitLValue here rather than - // emitting call arguments, in order to preserve TBAA information from - // the RHS. - LValue RHS = isa<CXXOperatorCallExpr>(CE) - ? TrivialAssignmentRHS - : EmitLValue(*CE->arg_begin()); - EmitAggregateAssign(This, RHS, CE->getType()); - return RValue::get(This.getPointer(*this)); - } + if (TrivialAssignment) { + // 1. Evaluate 'this' (Destination) as a checked store. + LValue This = EmitCheckedLValue(Base, TCK_Store); + + // 2. Evaluate RHS (Source) as a checked load. + // If it's an operator call (a = b), we use the RHS evaluated at line 270. + // If it's a direct call (constructor), we evaluate the first argument. + LValue RHS = isa<CXXOperatorCallExpr>(CE) + ? TrivialAssignmentRHS + : EmitCheckedLValue(*CE->arg_begin(), TCK_Load); - assert(MD->getParent()->mayInsertExtraPadding() && - "unknown trivial member function"); + EmitAggregateAssign(This, RHS, CE->getType()); + return RValue::get(This.getPointer(*this)); } // Compute the function type we're calling. diff --git a/clang/test/CodeGen/ubsan-aggregate-null-align.c b/clang/test/CodeGen/ubsan-aggregate-null-align.c new file mode 100644 index 0000000000000..18133327f0fc8 --- /dev/null +++ b/clang/test/CodeGen/ubsan-aggregate-null-align.c @@ -0,0 +1,63 @@ +// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsanitize=alignment,null \ +// RUN: -emit-llvm -std=c23 %s -o - \ +// RUN: | FileCheck %s --check-prefixes=CHECK,CHECK-UBSAN +// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -std=c23 %s -o - \ +// RUN: | FileCheck %s --check-prefixes=CHECK,CHECK-NO-UBSAN +// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsanitize=alignment,null \ +// RUN: -emit-llvm -xc++ %s -o - \ +// RUN: | FileCheck %s --check-prefixes=CHECK,CHECK-UBSAN +// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -xc++ %s -o - \ +// RUN: | FileCheck %s --check-prefixes=CHECK,CHECK-NO-UBSAN + +typedef struct Small { int x; } Small; +typedef struct Container { struct Small inner; } Container; + +#ifdef __cplusplus +extern "C" { +#endif + +// CHECK-LABEL: define {{.*}}void @test_direct_assign_ptr( +void test_direct_assign_ptr(struct Small *dest, struct Small *src) { + // CHECK-UBSAN: %[[D:.*]] = load ptr, ptr %dest.addr + // CHECK-UBSAN: %[[S:.*]] = load ptr, ptr %src.addr + + // Verify LHS (Dest) Check + // CHECK-UBSAN: %[[D_NULL:.*]] = icmp ne ptr %[[D]], null + // CHECK-UBSAN: %[[D_ALIGN:.*]] = and i64 %{{.*}}, 3 + // CHECK-UBSAN: %[[D_ALIGN_OK:.*]] = icmp eq i64 %[[D_ALIGN]], 0 + // CHECK-UBSAN: %[[D_OK:.*]] = and i1 %[[D_NULL]], %[[D_ALIGN_OK]] + // CHECK-UBSAN: br i1 %[[D_OK]], label %[[D_CONT:.*]], label %[[D_HANDLER:.*]] + + // CHECK-UBSAN: [[D_HANDLER]]: + // CHECK-UBSAN: call void @__ubsan_handle_type_mismatch_v1_abort + // CHECK-UBSAN: unreachable + + // CHECK-UBSAN: [[D_CONT]]: + // Verify RHS (Src) Check + // CHECK-UBSAN: %[[S_NULL:.*]] = icmp ne ptr %[[S]], null + // CHECK-UBSAN: br i1 %[[S_NULL]], label %[[S_CONT:.*]], label %[[S_HANDLER:.*]] + + // CHECK-UBSAN: [[S_HANDLER]]: + // CHECK-UBSAN: call void @__ubsan_handle_type_mismatch_v1_abort + + // CHECK-UBSAN: [[S_CONT]]: + // CHECK: call void @llvm.memcpy.p0.p0.i64(ptr align 4 %[[D]], ptr align 4 %[[S]], i64 4, i1 false) + + // CHECK-NO-UBSAN-NOT: @__ubsan_handle_type_mismatch + *dest = *src; +} + +// CHECK-LABEL: define {{.*}}void @test_nested_struct( +void test_nested_struct(struct Container *c, struct Small *s) { + // CHECK-UBSAN: %[[C:.*]] = load ptr, ptr %c.addr + // CHECK-UBSAN: icmp ne ptr %[[C]], null + // CHECK-UBSAN: br i1 %{{.*}}, label %[[CONT:.*]], label %[[HANDLER:.*]] + + // CHECK-UBSAN: [[HANDLER]]: + // CHECK-UBSAN: call void @__ubsan_handle_type_mismatch_v1_abort + c->inner = *s; +} + +#ifdef __cplusplus +} +#endif `````````` </details> https://github.com/llvm/llvm-project/pull/175032 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
