r228003 - Merge ArtificialLocation into ApplyDebugLocation and make a clear
David Blaikie
dblaikie at gmail.com
Tue Feb 3 11:09:36 PST 2015
On Tue, Feb 3, 2015 at 10:40 AM, Adrian Prantl <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
> 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
> cfe-commits at cs.uiuc.edu
> 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/3aa3f9dc/attachment.html>
More information about the cfe-commits
mailing list