[PATCH] D19708: [CGDebugInfo] Generate debug info for member calls in the context of the callee expression

[Hal Finkel via cfe-commits]

----- Original Message -----

> From: "David Blaikie" <dblaikie at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Richard Smith" <richard at metafoo.co.uk>, "Adrian Prantl"
> <aprantl at apple.com>, "Duncan P. N. Exon Smith"
> <dexonsmith at apple.com>, "Eric Christopher" <echristo at gmail.com>,
> "Jun Bum Lim" <junbuml at codeaurora.org>, "cfe-commits"
> <cfe-commits at lists.llvm.org>,
> reviews+D19708+public+e9ddc425037326c5 at reviews.llvm.org
> Sent: Thursday, May 12, 2016 2:54:26 PM
> Subject: Re: [PATCH] D19708: [CGDebugInfo] Generate debug info for
> member calls in the context of the callee expression

> On Mon, May 2, 2016 at 1:17 PM, Hal Finkel < hfinkel at anl.gov > wrote:

> > ----- Original Message -----
> 

> > > From: "David Blaikie" < dblaikie at gmail.com >
> 
> > > To: reviews+D19708+public+e9ddc425037326c5 at reviews.llvm.org ,
> > > "Hal
> > > Finkel" < hfinkel at anl.gov >
> 
> > > Cc: "Richard Smith" < richard at metafoo.co.uk >, "Adrian Prantl" <
> > > aprantl at apple.com >, "Duncan P. N. Exon Smith"
> 
> > > < dexonsmith at apple.com >, "Eric Christopher" < echristo at gmail.com
> > > >, "Jun Bum Lim" < junbuml at codeaurora.org >,
> 
> > > "cfe-commits" < cfe-commits at lists.llvm.org >
> 
> > > Sent: Friday, April 29, 2016 4:52:26 PM
> 
> > > Subject: Re: [PATCH] D19708: [CGDebugInfo] Generate debug info
> > > for
> > > member calls in the context of the callee
> 
> > > expression
> 
> > >
> 
> > >
> 
> > > You could simplify the test case further, down to just:
> 
> > >
> 
> > > struct foo { void bar(); };
> 
> > > void f(foo *f) {
> 
> > > f->bar();
> 
> > > }
> 
> > >
> 
> > > and check that the call instruction has the desired column (or if
> > > you
> 
> > > want a test that doesn't depend on column info (you can force it
> > > on
> 
> > > with a flag, but we might vary whether it's on by default based
> > > on
> 
> > > target, etc, I'm not sure) you could put 'bar();' on a separate
> > > line
> 
> > > from 'f->' and check the call was on the second line and not the
> 
> > > first).
> 

> > Certainly. I'm not sure much we want to reduce the test case,
> > however, because I particularly want to cover the case of two calls
> > on the same line with column info.
> 
> Why is it helpful to cover two calls? That's certainly where the
> issue was observed, but it's not the bug/fix as such. Once we put
> the location at the start of the function name we incidentally
> improve the situation where there are two calls.
True. My underlying thought process here is: Do I think it is plausible that someone might break this in such a way that all of the calls on one line would end up pointing at the first call expression? This does not seem outside the realm of what is possible, as far as such bugs go, and so I'd like to explicitly cover this case. Given that the test is still relatively simple, it seems worthwhile. I certainly see your point, however. 

> > Given that this is still pretty simple, I think that covering it
> > directly seems reasonable.
> 

> I think it can make tests more confusing when it comes to tracking
> the actual behavior change/intent of the test, etc.

> > > Richard might be able to tell us whether there's a preferred
> > > place
> 
> > > for a test for a change like this - should it be a debug info
> > > test,
> 
> > > a diagnostic test,
> 

> > At least for the test you suggested in a previous e-mail, this
> > patch
> > has no effect on the output diagnostics (although it certainly does
> > have the desired effect on the debug info). Specifically, even with
> > this patch, we still have:
> 

> > $ cat /tmp/loc.cpp
> 
> > struct foo {
> 
> > const foo *x() const;
> 
> > void y();
> 
> > };
> 

> > void f(const foo *g) {
> 
> > g->x()->y();
> 
> > g->x()->x()->y();
> 
> > }
> 

> > $ clang /tmp/loc.cpp -fsyntax-only
> 
> > /tmp/loc.cpp:7:3: error: member function 'y' not viable: 'this'
> > argument has type 'const foo', but function is not marked const
> 
> > g->x()->y();
> 
> > ^~~~~~
> 
> > /tmp/loc.cpp:3:8: note: 'y' declared here
> 
> > void y();
> 
> > ^
> 
> > /tmp/loc.cpp:8:3: error: member function 'y' not viable: 'this'
> > argument has type 'const foo', but function is not marked const
> 
> > g->x()->x()->y();
> 
> > ^~~~~~~~~~~
> 
> > /tmp/loc.cpp:3:8: note: 'y' declared here
> 
> > void y();
> 
> > ^
> 
> > 2 errors generated.
> 

> Curious - perhaps we should fix the diagnostic too? I guess it's
> using the start location instead of the preferred location?

> Hmm, yeah, maybe that's not right anyway - it's specifically trying
> to describe the left operand, though that's different from any case
> where a normal operand is mismatched by constness. Then it just says
> it can't call the function & points to the function (with a note
> explaining)

> Anyway - seems OK. Please commit.

r270775, thanks! 

-Hal 

> - David

> > Thanks again,
> 
> > Hal
> 

> > > or perhaps just an ast dump test?
> 
> > >
> 
> > > Perhaps a test for the case where there is no valid callee would
> > > be
> 
> > > good? Where does that come up - call through a member function
> 
> > > pointer?
> 
> > >
> 
> > >
> 
> > > On Fri, Apr 29, 2016 at 9:19 AM, Hal Finkel via cfe-commits <
> 
> > > cfe-commits at lists.llvm.org > wrote:
> 
> > >
> 
> > >
> 
> > > hfinkel updated this revision to Diff 55610.
> 
> > > hfinkel added a comment.
> 
> > >
> 
> > > Use David's suggested approach: Modify the preferred expression
> 
> > > location for member calls. If the callee has a valid location
> > > (not
> 
> > > all do), then use that. Otherwise, fall back to the starting
> 
> > > location. This seems to cleanly fix the debug-info problem.
> 
> > >
> 
> > >
> 
> > > http://reviews.llvm.org/D19708
> 
> > >
> 
> > > Files:
> 
> > > include/clang/AST/ExprCXX.h
> 
> > >
> 
> > >
> 
> > > test/CodeGenCXX/debug-info-member-call.cpp
> 
> > >
> 
> > > Index: test/CodeGenCXX/debug-info-member-call.cpp
> 
> > > ===================================================================
> 
> > > --- /dev/null
> 
> > > +++ test/CodeGenCXX/debug-info-member-call.cpp
> 
> > > @@ -0,0 +1,24 @@
> 
> > > +// RUN: %clang_cc1 -triple x86_64-unknown_unknown -emit-llvm
> 
> > > -debug-info-kind=standalone -dwarf-column-info %s -o - |
> > > FileCheck
> 
> > > %s
> 
> > > +void ext();
> 
> > > +
> 
> > > +struct Bar {
> 
> > > + void bar() { ext(); }
> 
> > > +};
> 
> > > +
> 
> > > +struct Foo {
> 
> > > + Bar *b;
> 
> > > +
> 
> > > + Bar *foo() { return b; }
> 
> > > +};
> 
> > > +
> 
> > > +void test(Foo *f) {
> 
> > > + f->foo()->bar();
> 
> > > +}
> 
> > > +
> 
> > > +// CHECK-LABEL: @_Z4testP3Foo
> 
> > > +// CHECK: call {{.*}} @_ZN3Foo3fooEv{{.*}}, !dbg
> > > ![[CALL1LOC:.*]]
> 
> > > +// CHECK: call void @_ZN3Bar3barEv{{.*}}, !dbg ![[CALL2LOC:.*]]
> 
> > > +
> 
> > > +// CHECK: ![[CALL1LOC]] = !DILocation(line: [[LINE:[0-9]+]],
> > > column:
> 
> > > 6,
> 
> > > +// CHECK: ![[CALL2LOC]] = !DILocation(line: [[LINE]], column:
> > > 13,
> 
> > > +
> 
> > > Index: include/clang/AST/ExprCXX.h
> 
> > > ===================================================================
> 
> > > --- include/clang/AST/ExprCXX.h
> 
> > > +++ include/clang/AST/ExprCXX.h
> 
> > > @@ -145,6 +145,14 @@
> 
> > > /// FIXME: Returns 0 for member pointer call exprs.
> 
> > > CXXRecordDecl *getRecordDecl() const;
> 
> > >
> 
> > > + SourceLocation getExprLoc() const LLVM_READONLY {
> 
> > > + SourceLocation CLoc = getCallee()->getExprLoc();
> 
> > > + if (CLoc.isValid())
> 
> > > + return CLoc;
> 
> > > +
> 
> > > + return getLocStart();
> 
> > > + }
> 
> > > +
> 
> > > static bool classof(const Stmt *T) {
> 
> > > return T->getStmtClass() == CXXMemberCallExprClass;
> 
> > > }
> 
> > >
> 
> > >
> 
> > >
> 
> > > _______________________________________________
> 
> > > cfe-commits mailing list
> 
> > > cfe-commits at lists.llvm.org
> 
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> 
> > >
> 
> > >
> 
> > >
> 

> > --
> 
> > Hal Finkel
> 
> > Assistant Computational Scientist
> 
> > Leadership Computing Facility
> 
> > Argonne National Laboratory
> 

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160525/3e5600b4/attachment.html>