> On Feb 6, 2015, at 2:57 PM, David Blaikie <[email protected]> wrote: > > > > On Wed, Feb 4, 2015 at 5:29 PM, Adrian Prantl <[email protected] > <mailto:[email protected]>> wrote: > > > On Feb 4, 2015, at 11:47 AM, David Blaikie <[email protected] > > <mailto:[email protected]>> wrote: > > > > Author: dblaikie > > Date: Wed Feb 4 13:47:54 2015 > > New Revision: 228181 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=228181&view=rev > > <http://llvm.org/viewvc/llvm-project?rev=228181&view=rev> > > Log: > > DebugInfo: Attribute cleanup code to the end of the scope, not the end of > > the function. > > > > Now if you break on a dtor and go 'up' in your debugger (or you get an > > asan failure in a dtor) during an exception unwind, you'll have more > > context. Instead of all dtors appearing to be called from the '}' of the > > function, they'll be attributed to the end of the scope of the variable, > > the same as the non-exceptional dtor call. > > > > This doesn't /quite/ remove all uses of CurEHLocation (which might be > > nice to remove, for a few reasons) - it's still used to choose the > > location for some other work in the landing pad. It'd be nice to > > attribute that code to the same location as the exception calls within > > the block and to remove CurEHLocation. > > > > Modified: > > cfe/trunk/lib/CodeGen/CGCleanup.cpp > > cfe/trunk/lib/CodeGen/CGDebugInfo.cpp > > cfe/trunk/lib/CodeGen/CodeGenFunction.cpp > > cfe/trunk/lib/CodeGen/CodeGenFunction.h > > cfe/trunk/test/CodeGenCXX/debug-info-line.cpp > > cfe/trunk/test/CodeGenCXX/linetable-cleanup.cpp > > cfe/trunk/test/CodeGenObjC/arc-linetable.m > > > > Modified: cfe/trunk/lib/CodeGen/CGCleanup.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCleanup.cpp?rev=228181&r1=228180&r2=228181&view=diff > > > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCleanup.cpp?rev=228181&r1=228180&r2=228181&view=diff> > > ============================================================================== > > --- cfe/trunk/lib/CodeGen/CGCleanup.cpp (original) > > +++ cfe/trunk/lib/CodeGen/CGCleanup.cpp Wed Feb 4 13:47:54 2015 > > @@ -861,8 +861,6 @@ void CodeGenFunction::PopCleanupBlock(bo > > > > // Emit the EH cleanup if required. > > if (RequiresEHCleanup) { > > - auto AL = ApplyDebugLocation::CreateDefaultArtificial(*this, > > CurEHLocation); > > - > > You can’t just remove this here: If CurEHLocation is a nullptr ((as it would > be in an artificial function), or after removing this - the current location) > we still need to set the location to *something* valid or we will create an > unattributed call which will after inlining trigger the famous "scope does > not describe function” assertion. > > When does that happen (CurEHLocation being nullptr or the current location > being unset)?
In the wonderful language that is Objective C++; when there is an invocation to a C++ destructor in an ObjC destroy block helper, such as in the cfe/trunk/test/CodeGenObjCXX/nested-ehlocation.mm testcase. Copy/Destroy helpers are artificial locations, so CurEHLocation is an empty SourceLocation(). I might be wrong about that the current location ever being unset after all the recent changes, but I would feel more comfortable if we at least had an assertion about it there, to make sure that there is a valid location. The earlier we can catch the problem, the better. > > > > > CGBuilderTy::InsertPoint SavedIP = Builder.saveAndClearIP(); > > > > EmitBlock(EHEntry); > > > > Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=228181&r1=228180&r2=228181&view=diff > > > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=228181&r1=228180&r2=228181&view=diff> > > ============================================================================== > > --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original) > > +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Wed Feb 4 13:47:54 2015 > > @@ -55,7 +55,6 @@ CGDebugInfo::~CGDebugInfo() { > > ApplyDebugLocation::ApplyDebugLocation(CodeGenFunction &CGF, > > SourceLocation TemporaryLocation) > > : CGF(CGF) { > > - assert(!TemporaryLocation.isInvalid() && "invalid location”); > > Please don’t. I’d like to keep a clear separation between the uses of > ApplyDebugLocation were a location is mandatory (for example, when we will be > emitting a call/invoke) and the ones were it is optional. > > Given that the ApplyDebugLocation(CGF, Expr) ctor already allows expressions > with invalid SourceLocations as no-ops, this seemed consistent. Is there a > reason to treat these differently? Or should we have 4 flavors here > ({SL,Expr}x{mandatory,optional})? The Expr flavor is only (hopefully) used within the body of a function where it is safe to assume that a previous location was set and can be reused as a somewhat reasonable default, so I think we can get away with just having only one version. But for anything that might be used in the function epilogue/cleanup blocks we need to be more careful. -- adrian > > > init(TemporaryLocation); > > } > > > > > > Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=228181&r1=228180&r2=228181&view=diff > > > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=228181&r1=228180&r2=228181&view=diff> > > ============================================================================== > > --- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original) > > +++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Wed Feb 4 13:47:54 2015 > > @@ -241,8 +241,6 @@ void CodeGenFunction::FinishFunction(Sou > > // edges will be *really* confused. > > bool EmitRetDbgLoc = true; > > if (EHStack.stable_begin() != PrologueCleanupDepth) { > > - PopCleanupBlocks(PrologueCleanupDepth); > > - > > // Make sure the line table doesn't jump back into the body for > > // the ret after it's been at EndLoc. > > EmitRetDbgLoc = false; > > @@ -250,6 +248,8 @@ void CodeGenFunction::FinishFunction(Sou > > if (CGDebugInfo *DI = getDebugInfo()) > > if (OnlySimpleReturnStmts) > > DI->EmitLocation(Builder, EndLoc); > > + > > + PopCleanupBlocks(PrologueCleanupDepth); > > } > > > > // Emit function epilog (to return). > > > > Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=228181&r1=228180&r2=228181&view=diff > > > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=228181&r1=228180&r2=228181&view=diff> > > ============================================================================== > > --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original) > > +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Wed Feb 4 13:47:54 2015 > > @@ -567,7 +567,10 @@ public: > > > > // If we should perform a cleanup, force them now. Note that > > // this ends the cleanup scope before rescoping any labels. > > - if (PerformCleanup) ForceCleanup(); > > + if (PerformCleanup) { > > + ApplyDebugLocation DL(CGF, Range.getEnd()); > > + ForceCleanup(); > > + } > > This also looks super-dangerous for the same reason. What if the last > location was empty? > > -- adrian > > } > > > > /// \brief Force the emission of cleanups now, instead of waiting > > > > Modified: cfe/trunk/test/CodeGenCXX/debug-info-line.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-line.cpp?rev=228181&r1=228180&r2=228181&view=diff > > > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-line.cpp?rev=228181&r1=228180&r2=228181&view=diff> > > ============================================================================== > > --- cfe/trunk/test/CodeGenCXX/debug-info-line.cpp (original) > > +++ cfe/trunk/test/CodeGenCXX/debug-info-line.cpp Wed Feb 4 13:47:54 2015 > > @@ -259,6 +259,21 @@ void f21() { > > f21_b(); > > } > > > > +// CHECK-LABEL: define > > +struct f22_dtor { > > + ~f22_dtor(); > > +}; > > +void f22() { > > + { > > + f22_dtor f; > > + src(); > > +// CHECK: call {{.*}}src > > +// CHECK: call {{.*}}, !dbg [[DBG_F22:![0-9]*]] > > +// CHECK: call {{.*}}, !dbg [[DBG_F22]] > > +#line 2400 > > + } > > +} > > + > > // CHECK: [[DBG_F1]] = !MDLocation(line: 100, > > // CHECK: [[DBG_FOO_VALUE]] = !MDLocation(line: 200, > > // CHECK: [[DBG_FOO_REF]] = !MDLocation(line: 202, > > > > Modified: cfe/trunk/test/CodeGenCXX/linetable-cleanup.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/linetable-cleanup.cpp?rev=228181&r1=228180&r2=228181&view=diff > > > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/linetable-cleanup.cpp?rev=228181&r1=228180&r2=228181&view=diff> > > ============================================================================== > > --- cfe/trunk/test/CodeGenCXX/linetable-cleanup.cpp (original) > > +++ cfe/trunk/test/CodeGenCXX/linetable-cleanup.cpp Wed Feb 4 13:47:54 2015 > > @@ -4,8 +4,8 @@ > > // simple return expressions. > > > > // CHECK: define {{.*}}foo > > -// CHECK: call void @_ZN1CD1Ev(%class.C* {{.*}}), !dbg ![[CLEANUP:[0-9]+]] > > -// CHECK: ret i32 0, !dbg ![[RET:[0-9]+]] > > +// CHECK: call void @_ZN1CD1Ev(%class.C* {{.*}}), !dbg ![[RET:[0-9]+]] > > +// CHECK: ret i32 0, !dbg ![[RET]] > > > > // CHECK: define {{.*}}bar > > // CHECK: ret void, !dbg ![[RETBAR:[0-9]+]] > > @@ -23,9 +23,8 @@ int foo() > > { > > C c; > > c.i = 42; > > - // This breakpoint should be at/before the cleanup code. > > - // CHECK: ![[CLEANUP]] = !MDLocation(line: [[@LINE+1]], scope: !{{.*}}) > > return 0; > > + // This breakpoint should be at/before the cleanup code. > > // CHECK: ![[RET]] = !MDLocation(line: [[@LINE+1]], scope: !{{.*}}) > > } > > > > > > Modified: cfe/trunk/test/CodeGenObjC/arc-linetable.m > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/arc-linetable.m?rev=228181&r1=228180&r2=228181&view=diff > > > > <http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/arc-linetable.m?rev=228181&r1=228180&r2=228181&view=diff> > > ============================================================================== > > --- cfe/trunk/test/CodeGenObjC/arc-linetable.m (original) > > +++ cfe/trunk/test/CodeGenObjC/arc-linetable.m Wed Feb 4 13:47:54 2015 > > @@ -4,8 +4,8 @@ > > > > // CHECK: define {{.*}}testNoSideEffect > > // CHECK: call void @objc_storeStrong{{.*}} > > -// CHECK: call void @objc_storeStrong{{.*}} !dbg ![[ARC1:[0-9]+]] > > -// CHECK: ret {{.*}} !dbg ![[RET1:[0-9]+]] > > +// CHECK: call void @objc_storeStrong{{.*}} !dbg ![[RET1:[0-9]+]] > > +// CHECK: ret {{.*}} !dbg ![[RET1]] > > > > // CHECK: define {{.*}}testNoCleanup > > // CHECK: ret {{.*}} !dbg ![[RET2:[0-9]+]] > > @@ -21,8 +21,8 @@ > > > > // CHECK: define {{.*}}testVoid > > // CHECK: call void @objc_storeStrong{{.*}} > > -// CHECK: call void @objc_storeStrong{{.*}} !dbg ![[ARC5:[0-9]+]] > > -// CHECK: ret {{.*}} !dbg ![[RET5:[0-9]+]] > > +// CHECK: call void @objc_storeStrong{{.*}} !dbg ![[RET5:[0-9]+]] > > +// CHECK: ret {{.*}} !dbg ![[RET5]] > > > > // CHECK: define {{.*}}testVoidNoReturn > > // CHECK: @objc_msgSend{{.*}} !dbg ![[MSG6:[0-9]+]] > > @@ -57,9 +57,8 @@ typedef signed char BOOL; > > // CHECK: ![[TESTNOSIDEEFFECT:.*]] = {{.*}}[ DW_TAG_subprogram ] [line > > [[@LINE+1]]] [local] [def] [-[AppDelegate testNoSideEffect:]] > > - (int)testNoSideEffect:(NSString *)foo { > > int x = 1; > > - // CHECK: ![[ARC1]] = !MDLocation(line: [[@LINE+1]], scope: > > ![[TESTNOSIDEEFFECT]]) > > return 1; // Return expression > > - // CHECK: ![[RET1]] = !MDLocation(line: [[@LINE+1]], scope: !{{.*}}) > > + // CHECK: ![[RET1]] = !MDLocation(line: [[@LINE+1]], scope: > > ![[TESTNOSIDEEFFECT]]) > > } // Cleanup + Ret > > > > - (int)testNoCleanup { > > @@ -82,7 +81,6 @@ typedef signed char BOOL; > > } > > > > - (void)testVoid:(NSString *)foo { > > - // CHECK: ![[ARC5]] = !MDLocation(line: [[@LINE+1]], scope: !{{.*}}) > > return; > > // CHECK: ![[RET5]] = !MDLocation(line: [[@LINE+1]], scope: !{{.*}}) > > } > > > > > > _______________________________________________ > > cfe-commits mailing list > > [email protected] <mailto:[email protected]> > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > <http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
