r228181 - DebugInfo: Attribute cleanup code to the end of the scope, not the end of the function.
David Blaikie
dblaikie at gmail.com
Mon Feb 9 11:07:13 PST 2015
On Mon, Feb 9, 2015 at 9:39 AM, Adrian Prantl <aprantl at apple.com> wrote:
>
> 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> wrote:
>
>>
>> > On Feb 4, 2015, at 11:47 AM, David Blaikie <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
>> > 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
>> > 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/20150209/404a203d/attachment.html>
More information about the cfe-commits
mailing list