r228003 - Merge ArtificialLocation into ApplyDebugLocation and make a clear
Adrian Prantl
aprantl at apple.com
Tue Feb 3 11:13:57 PST 2015
> On Feb 3, 2015, at 11:09 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Feb 3, 2015 at 10:40 AM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> 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 <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 <http://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 <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 <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 <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 <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”.
About the 2 names: My goal was to separate intent from implementation. The fact that the backend conflates no location and prologue locations shouldn’t leak into the frontend.
As mentioned in the commit message, this is a way of specifying two different kinds of default behavior if the location is invalid.
>
> 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)).
That’s a good idea! I’ll do that.
-- adrian
>
> 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 <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 <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 <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 <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 <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 <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 <http://nested-ehlocation.mm/>
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm?rev=228003&view=auto <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm?rev=228003&view=auto>
> ==============================================================================
> --- cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm <http://nested-ehlocation.mm/> (added)
> +++ cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm <http://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
> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits <http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150203/4b54d6fe/attachment.html>
More information about the cfe-commits
mailing list