On Tue, Feb 3, 2015 at 10:40 AM, Adrian Prantl <[email protected]> wrote:
> Author: adrian > Date: Tue Feb 3 12:40:42 2015 > New Revision: 228003 > > URL: http://llvm.org/viewvc/llvm-project?rev=228003&view=rev > Log: > Merge ArtificialLocation into ApplyDebugLocation and make a clear > distinction between the different use-cases. With the previous default > behavior we would occasionally emit empty debug locations in situations > where they actually were strictly required (= on invoke insns). > We now have a choice between defaulting to an empty location or an > artificial location. > > Specifically, this fixes a bug caused by a missing debug location when > emitting C++ EH cleanup blocks from within an artificial function, such as > an ObjC destroy helper function. > > rdar://problem/19670595 > > Added: > cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm > Modified: > cfe/trunk/lib/CodeGen/CGBlocks.cpp > cfe/trunk/lib/CodeGen/CGCleanup.cpp > cfe/trunk/lib/CodeGen/CGDebugInfo.cpp > cfe/trunk/lib/CodeGen/CGDebugInfo.h > cfe/trunk/lib/CodeGen/CGDecl.cpp > cfe/trunk/lib/CodeGen/CGDeclCXX.cpp > cfe/trunk/lib/CodeGen/CGException.cpp > cfe/trunk/lib/CodeGen/CGExprScalar.cpp > cfe/trunk/lib/CodeGen/CGStmt.cpp > cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp > > Modified: cfe/trunk/lib/CodeGen/CGBlocks.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBlocks.cpp?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGBlocks.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGBlocks.cpp Tue Feb 3 12:40:42 2015 > @@ -1178,7 +1178,7 @@ CodeGenFunction::GenerateBlockFunction(G > Alloca->setAlignment(Align); > // Set the DebugLocation to empty, so the store is recognized as a > // frame setup instruction by llvm::DwarfDebug::beginFunction(). > - ApplyDebugLocation NL(*this); > + ApplyDebugLocation NL(*this, ApplyDebugLocation::MarkAsPrologue); > Builder.CreateAlignedStore(BlockPointer, Alloca, Align); > BlockPointerDbgLoc = Alloca; > } > @@ -1328,10 +1328,10 @@ CodeGenFunction::GenerateCopyHelperFunct > nullptr, SC_Static, > false, > false); > - // Create a scope with an artificial location for the body of this > function. > - ApplyDebugLocation NL(*this); > + ApplyDebugLocation NL(*this, ApplyDebugLocation::MarkAsPrologue); > StartFunction(FD, C.VoidTy, Fn, FI, args); > - ArtificialLocation AL(*this); > + // Create a scope with an artificial location for the body of this > function. > + ApplyDebugLocation AL(*this, ApplyDebugLocation::Artificial); > > llvm::Type *structPtrTy = blockInfo.StructureType->getPointerTo(); > > @@ -1500,9 +1500,9 @@ CodeGenFunction::GenerateDestroyHelperFu > nullptr, SC_Static, > false, false); > // Create a scope with an artificial location for the body of this > function. > - ApplyDebugLocation NL(*this); > + ApplyDebugLocation NL(*this, ApplyDebugLocation::MarkAsPrologue); > StartFunction(FD, C.VoidTy, Fn, FI, args); > - ArtificialLocation AL(*this); > + ApplyDebugLocation AL(*this, ApplyDebugLocation::Artificial); > > llvm::Type *structPtrTy = blockInfo.StructureType->getPointerTo(); > > > Modified: cfe/trunk/lib/CodeGen/CGCleanup.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCleanup.cpp?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGCleanup.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGCleanup.cpp Tue Feb 3 12:40:42 2015 > @@ -861,7 +861,7 @@ void CodeGenFunction::PopCleanupBlock(bo > > // Emit the EH cleanup if required. > if (RequiresEHCleanup) { > - ApplyDebugLocation AutoRestoreLocation(*this, CurEHLocation); > + ApplyDebugLocation AL(*this, ApplyDebugLocation::Artificial, > CurEHLocation); > > CGBuilderTy::InsertPoint SavedIP = Builder.saveAndClearIP(); > > > Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Tue Feb 3 12:40:42 2015 > @@ -52,28 +52,34 @@ CGDebugInfo::~CGDebugInfo() { > "Region stack mismatch, stack not empty!"); > } > > -ArtificialLocation::ArtificialLocation(CodeGenFunction &CGF) > - : ApplyDebugLocation(CGF) { > - if (auto *DI = CGF.getDebugInfo()) { > - // Construct a location that has a valid scope, but no line info. > - assert(!DI->LexicalBlockStack.empty()); > - llvm::DIDescriptor Scope(DI->LexicalBlockStack.back()); > - CGF.Builder.SetCurrentDebugLocation(llvm::DebugLoc::get(0, 0, Scope)); > - } > +ApplyDebugLocation::ApplyDebugLocation(CodeGenFunction &CGF, > + SourceLocation TemporaryLocation) > + : CGF(CGF) { > + assert(!TemporaryLocation.isInvalid() && "invalid location"); > + init(TemporaryLocation); > } > > ApplyDebugLocation::ApplyDebugLocation(CodeGenFunction &CGF, > + bool MarkAsPrologue, > SourceLocation TemporaryLocation) > : CGF(CGF) { > - init(TemporaryLocation); > + init(TemporaryLocation, MarkAsPrologue); > } > > -void ApplyDebugLocation::init(SourceLocation TemporaryLocation) { > +void ApplyDebugLocation::init(SourceLocation TemporaryLocation, > + bool MarkAsPrologue) { > if (auto *DI = CGF.getDebugInfo()) { > OriginalLocation = CGF.Builder.getCurrentDebugLocation(); > - if (TemporaryLocation.isInvalid()) > - CGF.Builder.SetCurrentDebugLocation(llvm::DebugLoc()); > - else > + if (TemporaryLocation.isInvalid()) { > + if (MarkAsPrologue) > + CGF.Builder.SetCurrentDebugLocation(llvm::DebugLoc()); > + else { > + // Construct a location that has a valid scope, but no line info. > + assert(!DI->LexicalBlockStack.empty()); > + llvm::DIDescriptor Scope(DI->LexicalBlockStack.back()); > + CGF.Builder.SetCurrentDebugLocation(llvm::DebugLoc::get(0, 0, > Scope)); > + } > + } else > DI->EmitLocation(CGF.Builder, TemporaryLocation); > } > } > > Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.h?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGDebugInfo.h (original) > +++ cfe/trunk/lib/CodeGen/CGDebugInfo.h Tue Feb 3 12:40:42 2015 > @@ -47,7 +47,7 @@ namespace CodeGen { > /// and is responsible for emitting to llvm globals or pass directly to > /// the backend. > class CGDebugInfo { > - friend class ArtificialLocation; > + friend class ApplyDebugLocation; > friend class SaveAndRestoreLocation; > CodeGenModule &CGM; > const CodeGenOptions::DebugInfoKind DebugKind; > @@ -447,38 +447,36 @@ private: > /// location or preferred location of the specified Expr. > class ApplyDebugLocation { > private: > - void init(SourceLocation TemporaryLocation); > + void init(SourceLocation TemporaryLocation, bool MarkAsPrologue = > false); > > protected: > llvm::DebugLoc OriginalLocation; > CodeGenFunction &CGF; > > public: > - /// If TemporaryLocation is invalid, the IRBuilder will be set to not > attach > - /// debug locations, thus marking the instructions as prologue. > - ApplyDebugLocation(CodeGenFunction &CGF, > + enum { Artificial = false, MarkAsPrologue = true, NoLocation = true }; > + > + /// \brief Set the location to the (valid) TemporaryLocation. > + ApplyDebugLocation(CodeGenFunction &CGF, SourceLocation > TemporaryLocation); > + /// \brief Apply TemporaryLocation if it is valid, or apply a default > + /// location: If MarkAsPrologue is true, the IRBuilder will be set to > not > + /// attach debug locations, thus marking the instructions as > + /// prologue. Otherwise this switches to an artificial debug location > that has > + /// a valid scope, but no line information. > + /// > + /// Artificial locations are useful when emitting compiler-generated > helper > + /// functions that have no source location associated with them. The > DWARF > + /// specification allows the compiler to use the special line number 0 > to > + /// indicate code that can not be attributed to any source location. > Note that > + /// passing an empty SourceLocation to CGDebugInfo::setLocation() will > result > + /// in the last valid location being reused. > + ApplyDebugLocation(CodeGenFunction &CGF, bool MarkAsPrologue, > SourceLocation TemporaryLocation = SourceLocation()); > This seems confusing to me - Both having 2 names for the same constant value (true), and having the ability to specify a location even when passing a parameter that means, either "don't specify any location" or "use line zero". Also, having a runtime property for a compile-time use case is a bit awkward (no caller is going to get a dynamic value for these parameters, hopefully - so I'd be inclined to either use named ctors (static functions that return by the scoped device by value) or type-based ctor tags like std::pair's piecewise_construct thing)). > ApplyDebugLocation(CodeGenFunction &CGF, const Expr *E); > ApplyDebugLocation(CodeGenFunction &CGF, llvm::DebugLoc Loc); > ~ApplyDebugLocation(); > }; > > -/// \brief An RAII object that temporarily switches to > -/// an artificial debug location that has a valid scope, but no line > -/// information. This is useful when emitting compiler-generated > -/// helper functions that have no source location associated with > -/// them. The DWARF specification allows the compiler to use the > -/// special line number 0 to indicate code that can not be attributed > -/// to any source location. > -/// > -/// This is necessary because passing an empty SourceLocation to > -/// CGDebugInfo::setLocation() will result in the last valid location > -/// being reused. > -class ArtificialLocation : public ApplyDebugLocation { > -public: > - ArtificialLocation(CodeGenFunction &CGF); > -}; > - > > } // namespace CodeGen > } // namespace clang > > Modified: cfe/trunk/lib/CodeGen/CGDecl.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDecl.cpp?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGDecl.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGDecl.cpp Tue Feb 3 12:40:42 2015 > @@ -1090,7 +1090,7 @@ void CodeGenFunction::EmitAutoVarInit(co > if (emission.wasEmittedAsGlobal()) return; > > const VarDecl &D = *emission.Variable; > - ApplyDebugLocation DL(*this, D.getLocation()); > + ApplyDebugLocation DL(*this, ApplyDebugLocation::Artificial, > D.getLocation()); > QualType type = D.getType(); > > // If this local has an initializer, emit it now. > > Modified: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDeclCXX.cpp?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp Tue Feb 3 12:40:42 2015 > @@ -469,11 +469,11 @@ CodeGenFunction::GenerateCXXGlobalInitFu > ArrayRef<llvm::Function *> > Decls, > llvm::GlobalVariable *Guard) { > { > - ApplyDebugLocation NL(*this); > + ApplyDebugLocation NL(*this, ApplyDebugLocation::MarkAsPrologue); > StartFunction(GlobalDecl(), getContext().VoidTy, Fn, > getTypes().arrangeNullaryFunction(), FunctionArgList()); > // Emit an artificial location for this function. > - ArtificialLocation AL(*this); > + ApplyDebugLocation AL(*this, ApplyDebugLocation::Artificial); > > llvm::BasicBlock *ExitBlock = nullptr; > if (Guard) { > @@ -520,11 +520,11 @@ void CodeGenFunction::GenerateCXXGlobalD > const std::vector<std::pair<llvm::WeakVH, > llvm::Constant*> > > &DtorsAndObjects) { > { > - ApplyDebugLocation NL(*this); > + ApplyDebugLocation NL(*this, ApplyDebugLocation::MarkAsPrologue); > StartFunction(GlobalDecl(), getContext().VoidTy, Fn, > getTypes().arrangeNullaryFunction(), FunctionArgList()); > // Emit an artificial location for this function. > - ArtificialLocation AL(*this); > + ApplyDebugLocation AL(*this, ApplyDebugLocation::Artificial); > > // Emit the dtors, in reverse order from construction. > for (unsigned i = 0, e = DtorsAndObjects.size(); i != e; ++i) { > > Modified: cfe/trunk/lib/CodeGen/CGException.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGException.cpp?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGException.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGException.cpp Tue Feb 3 12:40:42 2015 > @@ -770,7 +770,7 @@ llvm::BasicBlock *CodeGenFunction::EmitL > > // Save the current IR generation state. > CGBuilderTy::InsertPoint savedIP = Builder.saveAndClearIP(); > - ApplyDebugLocation AutoRestoreLocation(*this, CurEHLocation); > + ApplyDebugLocation AL(*this, ApplyDebugLocation::Artificial, > CurEHLocation); > > const EHPersonality &personality = EHPersonality::get(CGM); > > > Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Tue Feb 3 12:40:42 2015 > @@ -3043,7 +3043,7 @@ Value *ScalarExprEmitter::VisitBinLAnd(c > // Emit an unconditional branch from this block to ContBlock. > { > // There is no need to emit line number for unconditional branch. > - ApplyDebugLocation DL(CGF); > + ApplyDebugLocation NL(CGF, ApplyDebugLocation::NoLocation); > CGF.EmitBlock(ContBlock); > } > // Insert an entry into the phi node for the edge with the value of > RHSCond. > > Modified: cfe/trunk/lib/CodeGen/CGStmt.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGStmt.cpp?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGStmt.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGStmt.cpp Tue Feb 3 12:40:42 2015 > @@ -564,8 +564,8 @@ void CodeGenFunction::EmitIfStmt(const I > // Emit the 'else' code if present. > if (const Stmt *Else = S.getElse()) { > { > - // There is no need to emit line number for unconditional branch. > - ApplyDebugLocation DL(*this); > + // There is no need to emit line number for an unconditional branch. > + ApplyDebugLocation NL(*this, ApplyDebugLocation::NoLocation); > EmitBlock(ElseBlock); > } > { > @@ -573,8 +573,8 @@ void CodeGenFunction::EmitIfStmt(const I > EmitStmt(Else); > } > { > - // There is no need to emit line number for unconditional branch. > - ApplyDebugLocation DL(*this); > + // There is no need to emit line number for an unconditional branch. > + ApplyDebugLocation NL(*this, ApplyDebugLocation::NoLocation); > EmitBranch(ContBlock); > } > } > > Modified: cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp?rev=228003&r1=228002&r2=228003&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp Tue Feb 3 12:40:42 2015 > @@ -86,13 +86,13 @@ static void EmitOMPIfClause(CodeGenFunct > // Emit the 'else' code if present. > { > // There is no need to emit line number for unconditional branch. > - ApplyDebugLocation DL(CGF); > + ApplyDebugLocation NL(CGF, ApplyDebugLocation::NoLocation); > CGF.EmitBlock(ElseBlock); > } > CodeGen(/*ThenBlock*/ false); > { > // There is no need to emit line number for unconditional branch. > - ApplyDebugLocation DL(CGF); > + ApplyDebugLocation NL(CGF, ApplyDebugLocation::NoLocation); > CGF.EmitBranch(ContBlock); > } > // Emit the continuation block for code after the if. > > Added: cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm?rev=228003&view=auto > > ============================================================================== > --- cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm (added) > +++ cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm Tue Feb 3 12:40:42 > 2015 > @@ -0,0 +1,24 @@ > +// RUN: %clang_cc1 -triple x86_64-apple-macosx -emit-llvm -g > -stdlib=libc++ -fblocks -fexceptions -x objective-c++ -o - %s | FileCheck %s > + > +// Verify that all invoke instructions have a debug location. > +// Literally: There are no unwind lines that don't end with ", (!dbg > 123)". > +// CHECK-NOT: {{to label %.* unwind label [^,]+$}} > Seems like we should actually test that these instructions have the location we desire rather than just "anything location will do, so long as it has a location". I'm also not sure which cases this test is trying to cover/which parts are necessary, might be helpful to either split it out into separate tests (they can be in the same file, but separated so each has a clear, singular purpose like my debug-info-line.cpp/.c tests) and/or comment it a bit so the important pieces are clear/motivated. - David > + > +void block(void (^)(void)); > +extern void foo(); > +struct A { > + ~A(void) { foo(); } > + void bar() const {} > +}; > +void baz(void const *const) {} > +struct B : A {}; > +void test() { > + A a; > + B b; > + block(^(void) { > + baz(&b); > + block(^() { > + a.bar(); > + }); > + }); > +} > > > _______________________________________________ > 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
