From 94ceb615746d237f4df84f757adc73b9320c94ae Mon Sep 17 00:00:00 2001 From: Daniel Dunbar Date: Tue, 24 Feb 2009 01:43:46 +0000 Subject: [PATCH] Fix two @synchronized bugs found by inspection: the expression to sychronize on should only be evaluated once, and it is evaluated outside the cleanup scope. Also, lift SyncEnter and SyncExit up in nervous anticipation of x86-64 zero cost EH. llvm-svn: 65362 --- clang/lib/CodeGen/CGObjCMac.cpp | 72 ++++++++++++--------------- clang/test/CodeGenObjC/synchronized.m | 19 +++++-- 2 files changed, 48 insertions(+), 43 deletions(-) diff --git a/clang/lib/CodeGen/CGObjCMac.cpp b/clang/lib/CodeGen/CGObjCMac.cpp index 6ba51ae99f8b..021fe15483d3 100644 --- a/clang/lib/CodeGen/CGObjCMac.cpp +++ b/clang/lib/CodeGen/CGObjCMac.cpp @@ -105,6 +105,12 @@ public: /// ExceptionThrowFn - LLVM objc_exception_throw function. llvm::Function *ExceptionThrowFn; + /// SyncEnterFn - LLVM object_sync_enter function. + llvm::Function *SyncEnterFn; + + /// SyncExitFn - LLVM object_sync_exit function. + llvm::Function *SyncExitFn; + ObjCCommonTypesHelper(CodeGen::CodeGenModule &cgm); ~ObjCCommonTypesHelper(){} }; @@ -188,12 +194,6 @@ public: /// SetJmpFn - LLVM _setjmp function. llvm::Function *SetJmpFn; - /// SyncEnterFn - LLVM object_sync_enter function. - llvm::Function *SyncEnterFn; - - /// SyncExitFn - LLVM object_sync_exit function. - llvm::Function *SyncExitFn; - public: ObjCTypesHelper(CodeGen::CodeGenModule &cgm); ~ObjCTypesHelper() {} @@ -1892,6 +1892,18 @@ void CGObjCMac::EmitTryOrSynchronizedStmt(CodeGen::CodeGenFunction &CGF, llvm::BasicBlock *FinallyNoExit = CGF.createBasicBlock("finally.noexit"); llvm::BasicBlock *FinallyRethrow = CGF.createBasicBlock("finally.throw"); llvm::BasicBlock *FinallyEnd = CGF.createBasicBlock("finally.end"); + + // For @synchronized, call objc_sync_enter(sync.expr). The + // evaluation of the expression must occur before we enter the + // @synchronized. We can safely avoid a temp here because jumps into + // @synchronized are illegal & this will dominate uses. + llvm::Value *SyncArg = 0; + if (!isTry) { + SyncArg = + CGF.EmitScalarExpr(cast(S).getSynchExpr()); + SyncArg = CGF.Builder.CreateBitCast(SyncArg, ObjCTypes.ObjectPtrTy); + CGF.Builder.CreateCall(ObjCTypes.SyncEnterFn, SyncArg); + } // Push an EH context entry, used for handling rethrows and jumps // through finally. @@ -1908,14 +1920,6 @@ void CGObjCMac::EmitTryOrSynchronizedStmt(CodeGen::CodeGenFunction &CGF, "_call_try_exit"); CGF.Builder.CreateStore(llvm::ConstantInt::getTrue(), CallTryExitPtr); - if (!isTry) { - // For @synchronized, call objc_sync_enter(sync.expr) - llvm::Value *Arg = CGF.EmitScalarExpr( - cast(S).getSynchExpr()); - Arg = CGF.Builder.CreateBitCast(Arg, ObjCTypes.ObjectPtrTy); - CGF.Builder.CreateCall(ObjCTypes.SyncEnterFn, Arg); - } - // Enter a new try block and call setjmp. CGF.Builder.CreateCall(ObjCTypes.ExceptionTryEnterFn, ExceptionData); llvm::Value *JmpBufPtr = CGF.Builder.CreateStructGEP(ExceptionData, 0, @@ -2079,14 +2083,10 @@ void CGObjCMac::EmitTryOrSynchronizedStmt(CodeGen::CodeGenFunction &CGF, if (const ObjCAtFinallyStmt* FinallyStmt = cast(S).getFinallyStmt()) CGF.EmitStmt(FinallyStmt->getFinallyBody()); - } - else { - // For @synchronized objc_sync_exit(expr); As finally's sole statement. - // For @synchronized, call objc_sync_enter(sync.expr) - llvm::Value *Arg = CGF.EmitScalarExpr( - cast(S).getSynchExpr()); - Arg = CGF.Builder.CreateBitCast(Arg, ObjCTypes.ObjectPtrTy); - CGF.Builder.CreateCall(ObjCTypes.SyncExitFn, Arg); + } else { + // Emit objc_sync_exit(expr); as finally's sole statement for + // @synchronized. + CGF.Builder.CreateCall(ObjCTypes.SyncExitFn, SyncArg); } // Emit the switch block @@ -2740,9 +2740,18 @@ ObjCCommonTypesHelper::ObjCCommonTypesHelper(CodeGen::CodeGenModule &cgm) Params.push_back(IdType); FTy = Types.GetFunctionType(Types.getFunctionInfo(Ctx.VoidTy, Params), false); - ExceptionThrowFn = CGM.CreateRuntimeFunction(FTy, "objc_exception_throw"); + + // synchronized APIs + // void objc_sync_enter (id) + // void objc_sync_exit (id) + Params.clear(); + Params.push_back(IdType); + + FTy = Types.GetFunctionType(Types.getFunctionInfo(Ctx.VoidTy, Params), false); + SyncEnterFn = CGM.CreateRuntimeFunction(FTy, "objc_sync_enter"); + SyncExitFn = CGM.CreateRuntimeFunction(FTy, "objc_sync_exit"); } ObjCTypesHelper::ObjCTypesHelper(CodeGen::CodeGenModule &cgm) @@ -3049,23 +3058,6 @@ ObjCTypesHelper::ObjCTypesHelper(CodeGen::CodeGenModule &cgm) false), "objc_exception_match"); - // synchronized APIs - // void objc_sync_enter (id) - Params.clear(); - Params.push_back(ObjectPtrTy); - SyncEnterFn = - CGM.CreateRuntimeFunction(llvm::FunctionType::get(llvm::Type::VoidTy, - Params, - false), - "objc_sync_enter"); - // void objc_sync_exit (id) - SyncExitFn = - CGM.CreateRuntimeFunction(llvm::FunctionType::get(llvm::Type::VoidTy, - Params, - false), - "objc_sync_exit"); - - Params.clear(); Params.push_back(llvm::PointerType::getUnqual(llvm::Type::Int32Ty)); SetJmpFn = diff --git a/clang/test/CodeGenObjC/synchronized.m b/clang/test/CodeGenObjC/synchronized.m index c74a83edbe35..3e59449e0bbb 100644 --- a/clang/test/CodeGenObjC/synchronized.m +++ b/clang/test/CodeGenObjC/synchronized.m @@ -1,5 +1,6 @@ -// RUN: clang -emit-llvm -triple=i686-apple-darwin8 -o %t %s -// RUNX: clang -emit-llvm -o %t %s +// RUN: clang -emit-llvm -triple=i686-apple-darwin8 -o %t %s -O2 && +// RUN: grep 'ret i32' %t | count 1 && +// RUN: grep 'ret i32 1' %t | count 1 #include @@ -28,5 +29,17 @@ void foo(id a) { } } +int f0(id a) { + int x = 0; + @synchronized((x++, a)) { + } + return x; // ret i32 1 +} - +void f1(id a) { + // The trick here is that the return shouldn't go through clean up, + // but there isn't a simple way to check this property. + @synchronized(({ return; }), a) { + return; + } +}