Inline assembly? Yuck. I thought that [llvm-]gcc just marked tons of locals as volatile?
-Chris On Jul 31, 2010, at 4:20 PM, John McCall wrote: > Author: rjmccall > Date: Sat Jul 31 18:20:56 2010 > New Revision: 109960 > > URL: http://llvm.org/viewvc/llvm-project?rev=109960&view=rev > Log: > Fix fragile-ABI ObjC exceptions in the presence of optimization with > the magic of inline assembly. Essentially we use read and write hazards > on the set of local variables to force flushing locals to memory > immediately before any protected calls and to inhibit optimizing locals > across the setjmp->catch edge. Fixes rdar://problem/8160285 > > > Modified: > cfe/trunk/lib/CodeGen/CGObjCMac.cpp > cfe/trunk/test/CodeGenObjC/exceptions.m > cfe/trunk/test/CodeGenObjC/synchronized.m > > Modified: cfe/trunk/lib/CodeGen/CGObjCMac.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjCMac.cpp?rev=109960&r1=109959&r2=109960&view=diff > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGObjCMac.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGObjCMac.cpp Sat Jul 31 18:20:56 2010 > @@ -25,7 +25,8 @@ > #include "clang/Basic/LangOptions.h" > #include "clang/Frontend/CodeGenOptions.h" > > -#include "llvm/Intrinsics.h" > +#include "llvm/InlineAsm.h" > +#include "llvm/IntrinsicInst.h" > #include "llvm/LLVMContext.h" > #include "llvm/Module.h" > #include "llvm/ADT/DenseSet.h" > @@ -2596,6 +2597,155 @@ > } > } > }; > + > + class FragileHazards { > + CodeGenFunction &CGF; > + llvm::SmallVector<llvm::Value*, 20> Locals; > + llvm::DenseSet<llvm::BasicBlock*> BlocksBeforeTry; > + > + llvm::InlineAsm *ReadHazard; > + llvm::InlineAsm *WriteHazard; > + > + llvm::FunctionType *GetAsmFnType(); > + > + void collectLocals(); > + void emitReadHazard(CGBuilderTy &Builder); > + > + public: > + FragileHazards(CodeGenFunction &CGF); > + void emitReadHazard(); > + void emitReadHazardsInNewBlocks(); > + void emitWriteHazard(); > + }; > +} > + > +/// Create the fragile-ABI read and write hazards based on the current > +/// state of the function, which is presumed to be immediately prior > +/// to a @try block. These hazards are used to maintain correct > +/// semantics in the face of optimization and the fragile ABI's > +/// cavalier use of setjmp/longjmp. > +FragileHazards::FragileHazards(CodeGenFunction &CGF) : CGF(CGF) { > + collectLocals(); > + > + if (Locals.empty()) return; > + > + // Collect all the blocks in the function. > + for (llvm::Function::iterator > + I = CGF.CurFn->begin(), E = CGF.CurFn->end(); I != E; ++I) > + BlocksBeforeTry.insert(&*I); > + > + llvm::FunctionType *AsmFnTy = GetAsmFnType(); > + > + // Create a read hazard for the allocas. This inhibits dead-store > + // optimizations and forces the values to memory. This hazard is > + // inserted before any 'throwing' calls in the protected scope to > + // reflect the possibility that the variables might be read from the > + // catch block if the call throws. > + { > + std::string Constraint; > + for (unsigned I = 0, E = Locals.size(); I != E; ++I) { > + if (I) Constraint += ','; > + Constraint += "*m"; > + } > + > + ReadHazard = llvm::InlineAsm::get(AsmFnTy, "", Constraint, true, false); > + } > + > + // Create a write hazard for the allocas. This inhibits folding > + // loads across the hazard. This hazard is inserted at the > + // beginning of the catch path to reflect the possibility that the > + // variables might have been written within the protected scope. > + { > + std::string Constraint; > + for (unsigned I = 0, E = Locals.size(); I != E; ++I) { > + if (I) Constraint += ','; > + Constraint += "=*m"; > + } > + > + WriteHazard = llvm::InlineAsm::get(AsmFnTy, "", Constraint, true, false); > + } > +} > + > +/// Emit a write hazard at the current location. > +void FragileHazards::emitWriteHazard() { > + if (Locals.empty()) return; > + > + CGF.Builder.CreateCall(WriteHazard, Locals.begin(), Locals.end()) > + ->setDoesNotThrow(); > +} > + > +void FragileHazards::emitReadHazard() { > + if (Locals.empty()) return; > + emitReadHazard(CGF.Builder); > +} > + > +void FragileHazards::emitReadHazard(CGBuilderTy &Builder) { > + assert(!Locals.empty()); > + Builder.CreateCall(ReadHazard, Locals.begin(), Locals.end()) > + ->setDoesNotThrow(); > +} > + > +/// Emit read hazards in all the protected blocks, i.e. all the blocks > +/// which have been inserted since the beginning of the try. > +void FragileHazards::emitReadHazardsInNewBlocks() { > + if (Locals.empty()) return; > + > + CGBuilderTy Builder(CGF.getLLVMContext()); > + > + // Iterate through all blocks, skipping those prior to the try. > + for (llvm::Function::iterator > + FI = CGF.CurFn->begin(), FE = CGF.CurFn->end(); FI != FE; ++FI) { > + llvm::BasicBlock &BB = *FI; > + if (BlocksBeforeTry.count(&BB)) continue; > + > + // Walk through all the calls in the block. > + for (llvm::BasicBlock::iterator > + BI = BB.begin(), BE = BB.end(); BI != BE; ++BI) { > + llvm::Instruction &I = *BI; > + > + // Ignore instructions that aren't non-intrinsic calls. > + // These are the only calls that can possibly call longjmp. > + if (!isa<llvm::CallInst>(I) && !isa<llvm::InvokeInst>(I)) continue; > + if (isa<llvm::IntrinsicInst>(I)) > + continue; > + > + // Ignore call sites marked nounwind. This may be questionable, > + // since 'nounwind' doesn't necessarily mean 'does not call longjmp'. > + llvm::CallSite CS(&I); > + if (CS.doesNotThrow()) continue; > + > + // Insert a read hazard before the call. > + Builder.SetInsertPoint(&BB, BI); > + emitReadHazard(Builder); > + } > + } > +} > + > +static void addIfPresent(llvm::DenseSet<llvm::Value*> &S, llvm::Value *V) { > + if (V) S.insert(V); > +} > + > +void FragileHazards::collectLocals() { > + // Compute a set of allocas to ignore. > + llvm::DenseSet<llvm::Value*> AllocasToIgnore; > + addIfPresent(AllocasToIgnore, CGF.ReturnValue); > + addIfPresent(AllocasToIgnore, CGF.NormalCleanupDest); > + addIfPresent(AllocasToIgnore, CGF.EHCleanupDest); > + > + // Collect all the allocas currently in the function. This is > + // probably way too aggressive. > + llvm::BasicBlock &Entry = CGF.CurFn->getEntryBlock(); > + for (llvm::BasicBlock::iterator > + I = Entry.begin(), E = Entry.end(); I != E; ++I) > + if (isa<llvm::AllocaInst>(*I) && !AllocasToIgnore.count(&*I)) > + Locals.push_back(&*I); > +} > + > +llvm::FunctionType *FragileHazards::GetAsmFnType() { > + std::vector<const llvm::Type *> Tys(Locals.size()); > + for (unsigned I = 0, E = Locals.size(); I != E; ++I) > + Tys[I] = Locals[I]->getType(); > + return llvm::FunctionType::get(CGF.Builder.getVoidTy(), Tys, false); > } > > /* > @@ -2737,6 +2887,12 @@ > ->setDoesNotThrow(); > } > > + // Create the fragile hazards. Note that this will not capture any > + // of the allocas required for exception processing, but will > + // capture the current basic block (which extends all the way to the > + // setjmp call) as "before the @try". > + FragileHazards Hazards(CGF); > + > // Allocate memory for the exception data and rethrow pointer. > llvm::Value *ExceptionData = CGF.CreateTempAlloca(ObjCTypes.ExceptionDataTy, > "exceptiondata.ptr"); > @@ -2792,6 +2948,9 @@ > // Emit the exception handler block. > CGF.EmitBlock(TryHandler); > > + // Don't optimize loads of the in-scope locals across this point. > + Hazards.emitWriteHazard(); > + > // Retrieve the exception object. We may emit multiple blocks but > // nothing can cross this so the value is already in SSA form. > llvm::CallInst *Caught = > @@ -2960,12 +3119,15 @@ > CGF.EmitBranchThroughCleanup(FinallyRethrow); > } > > + // Insert read hazards as required in the new blocks. > + Hazards.emitReadHazardsInNewBlocks(); > + > // Pop the cleanup. > CGF.PopCleanupBlock(); > CGF.EmitBlock(FinallyEnd.getBlock()); > > // Emit the rethrow block. > - CGF.Builder.ClearInsertionPoint(); > + CGBuilderTy::InsertPoint SavedIP = CGF.Builder.saveAndClearIP(); > CGF.EmitBlock(FinallyRethrow.getBlock(), true); > if (CGF.HaveInsertPoint()) { > CGF.Builder.CreateCall(ObjCTypes.getExceptionThrowFn(), > @@ -2974,7 +3136,7 @@ > CGF.Builder.CreateUnreachable(); > } > > - CGF.Builder.SetInsertPoint(FinallyEnd.getBlock()); > + CGF.Builder.restoreIP(SavedIP); > } > > void CGObjCMac::EmitThrowStmt(CodeGen::CodeGenFunction &CGF, > > Modified: cfe/trunk/test/CodeGenObjC/exceptions.m > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/exceptions.m?rev=109960&r1=109959&r2=109960&view=diff > ============================================================================== > --- cfe/trunk/test/CodeGenObjC/exceptions.m (original) > +++ cfe/trunk/test/CodeGenObjC/exceptions.m Sat Jul 31 18:20:56 2010 > @@ -37,3 +37,49 @@ > } > } > } > + > +// Test that modifications to local variables are respected under > +// optimization. rdar://problem/8160285 > + > +// CHECK: define i32 @f2() > +int f2() { > + extern void foo(void); > + > + // CHECK: [[X:%.*]] = alloca i32 > + // CHECK: store i32 0, i32* [[X]] > + int x = 0; > + > + // CHECK: [[SETJMP:%.*]] = call i32 @_setjmp > + // CHECK-NEXT: [[CAUGHT:%.*]] = icmp eq i32 [[SETJMP]], 0 > + // CHECK-NEXT: br i1 [[CAUGHT]] > + @try { > + // This load should be coalescable with the store of 0, so if the > + // optimizers ever figure out that out, that's okay. > + // CHECK: [[T1:%.*]] = load i32* [[X]] > + // CHECK-NEXT: [[T2:%.*]] = add nsw i32 [[T1]], 1 > + // CHECK-NEXT: store i32 [[T2]], i32* [[X]] > + x++; > + // CHECK-NEXT: call void asm sideeffect "", "*m"(i32* [[X]]) nounwind > + // CHECK-NEXT: call void @foo() > + foo(); > + } @catch (id) { > + // Landing pad. It turns out that the re-enter is unnecessary here. > + // CHECK: call void asm sideeffect "", "=*m"(i32* [[X]]) nounwind > + // CHECK-NEXT: call i8* @objc_exception_extract > + // CHECK-NEXT: call void @objc_exception_try_enter > + // CHECK-NEXT: call i32 @_setjmp > + // CHECK-NEXT: icmp eq i32 > + // CHECK-NEXT: br i1 > + > + // Catch handler. > + // CHECK: [[T1:%.*]] = load i32* [[X]] > + // CHECK-NEXT: [[T2:%.*]] = add nsw i32 [[T1]], -1 > + // CHECK-NEXT: store i32 [[T2]], i32* [[X]] > + // CHECK-NEXT: br label > + x--; > + } > + // CHECK: call void @objc_exception_try_exit > + // CHECK-NEXT: [[T:%.*]] = load i32* [[X]] > + // CHECK: ret i32 [[T]] > + return x; > +} > > Modified: cfe/trunk/test/CodeGenObjC/synchronized.m > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/synchronized.m?rev=109960&r1=109959&r2=109960&view=diff > ============================================================================== > --- cfe/trunk/test/CodeGenObjC/synchronized.m (original) > +++ cfe/trunk/test/CodeGenObjC/synchronized.m Sat Jul 31 18:20:56 2010 > @@ -1,6 +1,4 @@ > -// RUN: %clang_cc1 -emit-llvm -triple=i686-apple-darwin9 -o %t %s -O2 > -// RUN: grep 'ret i32' %t | count 1 > -// RUN: grep 'ret i32 1' %t | count 1 > +// RUN: %clang_cc1 -emit-llvm -triple=i686-apple-darwin9 -o - %s -O2 | > FileCheck %s > > @interface MyClass > { > @@ -10,31 +8,66 @@ > > @implementation MyClass > > +// CHECK: define internal void @"\01-[MyClass method]" > - (void)method > { > - @synchronized(self) > - { > - } > + // CHECK: call void @objc_sync_enter > + // CHECK: call void @objc_exception_try_enter > + // CHECK: call i32 @_setjmp > + @synchronized(self) { > + } > } > > @end > > +// CHECK: define void @foo( > void foo(id a) { > + // CHECK: [[A:%.*]] = alloca i8* > + > + // CHECK: call void @objc_sync_enter > + // CHECK: call void @objc_exception_try_enter > + // CHECK: call i32 @_setjmp > @synchronized(a) { > + // CHECK: call void @objc_exception_try_exit > + // CHECK: call void @objc_sync_exit > + // CHECK: ret void > return; > } > + > + // This is unreachable, but the optimizers can't know that. > + // CHECK: call void asm sideeffect "", "=*m"(i8** [[A]]) > + // CHECK: call i8* @objc_exception_extract > + // CHECK: call void @objc_sync_exit > + // CHECK: call void @objc_exception_throw > + // CHECK: unreachable > } > > +// CHECK: define i32 @f0( > int f0(id a) { > + // TODO: we can optimize the ret to a constant if we can figure out > + // either that x isn't stored to within the synchronized block or > + // that the synchronized block can't longjmp. > + > + // CHECK: [[X:%.*]] = alloca i32 > + // CHECK: store i32 1, i32* [[X]] > int x = 0; > @synchronized((x++, a)) { > } > - return x; // ret i32 1 > + > + // CHECK: [[T:%.*]] = load i32* [[X]] > + // CHECK: ret i32 [[T]] > + return x; > } > > +// CHECK: define void @f1( > 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. > + // Check that the return doesn't go through the cleanup. > + extern void opaque(void); > + opaque(); > + > + // CHECK: call void @opaque() > + // CHECK-NEXT: ret void > + > @synchronized(({ return; }), a) { > return; > } > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
