On Mon, Feb 9, 2015 at 9:39 AM, Adrian Prantl <[email protected]> wrote:
> > 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]> wrote: > >> >> > On Feb 4, 2015, at 11:47 AM, David Blaikie <[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 >> > 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 >> > >> ============================================================================== >> > --- 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. > Could an assert in here that just calls Builder.getCurrentDebugLocation().isValid(), if that suits... > > > >> >> >> > 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 >> > >> ============================================================================== >> > --- 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. > I'm not quite sure I see the distinction between those two cases - the problem occurs with any call instruction without a debug location (where the call site is in a function with debug info, an the call is to a function with debug info, and the call is inlinable - unfortunately I tried enforcing stronger contracts such that any call within a function with debug info needed a debug location, but that didn't hold - it might be possible to make it hold). So the epilogue/cleanup isn't especilaly special - any call, anywhere in the function, could have this issue. & I'm not sure why Expr locations are more likely to be preceeded by an existing location than cleanup/epilogue instructions are. - David > > -- 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 >> > >> ============================================================================== >> > --- 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 >> > >> ============================================================================== >> > --- 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 >> > >> ============================================================================== >> > --- 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 >> > >> ============================================================================== >> > --- 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 >> > >> ============================================================================== >> > --- 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] >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
