r228003 - Merge ArtificialLocation into ApplyDebugLocation and make a clear
David Blaikie
dblaikie at gmail.com
Tue Feb 3 11:24:19 PST 2015
On Tue, Feb 3, 2015 at 11:13 AM, Adrian Prantl <aprantl at apple.com> wrote:
>
> 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> 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”.
>
>
> 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.
>
Not sure it's much of a cognitive load & feels a bit confusing to me to
make these higher level constructs when they aren't really, but OK.
> As mentioned in the commit message, this is a way of specifying two
> different kinds of default behavior if the location is invalid.
>
It's still confusing/not clear to me based on the comment & documentation
how this function works/interacts with the source location given the
different values of the MarkAsPrologue parameter..
A few of the sources of confusion, I think, are:
Passing any value for the "MarkAsPrologue" parameter seems to imply that no
SourceLocation should be passed. I don't understand why a SourceLocation is
provided when ::Artificial is specified - Artificial means a zero line, so
why is there a SourceLocation being passed if it's just going to be
ignored? (or if it's not ignored, then it's not going to be the artificial
location that the caller asked for)
Using the term "MarkAsPrologue" as the implementation detail name of the
ctor parameter and the init function parameter is also confusing - since
now we've conflated MarkAsPrologue and NoLocation into the semantic
MarkAsPrologue concept when "no location" is really the underlying concept
we're implementing here. Once these are broken out into named ctors or
type-based ctor overloads this might be more obvious, as both ctors will
call init and can have a comment saying "// prologue instructions have no
debug location" as they pass down "/* NoLocation */true" to the init
function.
>
> 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
>>
>> ==============================================================================
>> --- 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());
>>
> 't bother to fix the self-referential definitions and grammar
>
> because my eyes started to bleed.
>
> // 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/b3e825a5/attachment.html>
More information about the cfe-commits
mailing list