r228181 - DebugInfo: Attribute cleanup code to the end of the scope, not the end of the function.
Adrian Prantl
aprantl at apple.com
Mon Feb 9 09:39:24 PST 2015
> On Feb 6, 2015, at 2:57 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, Feb 4, 2015 at 5:29 PM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>
> > On Feb 4, 2015, at 11:47 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> 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
> > 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/20150209/df3cc676/attachment.html>
More information about the cfe-commits
mailing list