<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'><br><hr id="zwchr"><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"David Blaikie" <dblaikie@gmail.com><br><b>To: </b>"Hal Finkel" <hfinkel@anl.gov><br><b>Cc: </b>"Richard Smith" <richard@metafoo.co.uk>, "Adrian Prantl" <aprantl@apple.com>, "Duncan P. N. Exon Smith" <dexonsmith@apple.com>, "Eric Christopher" <echristo@gmail.com>, "Jun Bum Lim" <junbuml@codeaurora.org>, "cfe-commits" <cfe-commits@lists.llvm.org>, reviews+D19708+public+e9ddc425037326c5@reviews.llvm.org<br><b>Sent: </b>Thursday, May 12, 2016 2:54:26 PM<br><b>Subject: </b>Re: [PATCH] D19708: [CGDebugInfo] Generate debug info for member calls in the context of the callee expression<br><br><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 2, 2016 at 1:17 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><span><hr id="zwchr"><br>
> From: "David Blaikie" <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
> To: <a href="mailto:reviews%2BD19708%2Bpublic%2Be9ddc425037326c5@reviews.llvm.org" target="_blank">reviews+D19708+public+e9ddc425037326c5@reviews.llvm.org</a>, "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br>
> Cc: "Richard Smith" <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>>, "Adrian Prantl" <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>>, "Duncan P. N. Exon Smith"<br>
> <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>>, "Eric Christopher" <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>>, "Jun Bum Lim" <<a href="mailto:junbuml@codeaurora.org" target="_blank">junbuml@codeaurora.org</a>>,<br>
> "cfe-commits" <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>><br>
> Sent: Friday, April 29, 2016 4:52:26 PM<br>
> Subject: Re: [PATCH] D19708: [CGDebugInfo] Generate debug info for member calls in the context of the callee<br>
> expression<br>
><br>
><br>
> You could simplify the test case further, down to just:<br>
><br>
> struct foo { void bar(); };<br>
> void f(foo *f) {<br>
> f->bar();<br>
> }<br>
><br>
> and check that the call instruction has the desired column (or if you<br>
> want a test that doesn't depend on column info (you can force it on<br>
> with a flag, but we might vary whether it's on by default based on<br>
> target, etc, I'm not sure) you could put 'bar();' on a separate line<br>
> from 'f->' and check the call was on the second line and not the<br>
> first).<br>
<br>
</span>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.</blockquote><div><br></div><div id="DWT20228">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.</div></div></div></div></blockquote>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.<br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> Given that this is still pretty simple, I think that covering it directly seems reasonable.<br></blockquote><div><br></div><div>I think it can make tests more confusing when it comes to tracking the actual behavior change/intent of the test, etc.</div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><span>> Richard might be able to tell us whether there's a preferred place<br>
> for a test for a change like this - should it be a debug info test,<br>
> a diagnostic test,<br>
<br>
</span>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:<br>
<br>
$ cat /tmp/loc.cpp<br>
<span> struct foo {<br>
const foo *x() const;<br>
void y();<br>
};<br>
<br>
void f(const foo *g) {<br>
g->x()->y();<br>
g->x()->x()->y();<br>
}<br>
<br>
</span>$ clang /tmp/loc.cpp -fsyntax-only<br>
/tmp/loc.cpp:7:3: error: member function 'y' not viable: 'this' argument has type 'const foo', but function is not marked const<br>
g->x()->y();<br>
^~~~~~<br>
/tmp/loc.cpp:3:8: note: 'y' declared here<br>
void y();<br>
^<br>
/tmp/loc.cpp:8:3: error: member function 'y' not viable: 'this' argument has type 'const foo', but function is not marked const<br>
<span> g->x()->x()->y();<br>
^~~~~~~~~~~<br>
</span> /tmp/loc.cpp:3:8: note: 'y' declared here<br>
void y();<br>
^<br>
2 errors generated.<br></blockquote><div><br></div><div id="DWT20229">Curious - perhaps we should fix the diagnostic too? I guess it's using the start location instead of the preferred location?<br><br>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)<br><br>Anyway - seems OK. Please commit.<br></div></div></div></div></blockquote>r270775, thanks!<br><br> -Hal<br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Thanks again,<br>
Hal<br>
<div><div><br>
> or perhaps just an ast dump test?<br>
><br>
> Perhaps a test for the case where there is no valid callee would be<br>
> good? Where does that come up - call through a member function<br>
> pointer?<br>
><br>
><br>
> On Fri, Apr 29, 2016 at 9:19 AM, Hal Finkel via cfe-commits <<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a> > wrote:<br>
><br>
><br>
> hfinkel updated this revision to Diff 55610.<br>
> hfinkel added a comment.<br>
><br>
> Use David's suggested approach: Modify the preferred expression<br>
> location for member calls. If the callee has a valid location (not<br>
> all do), then use that. Otherwise, fall back to the starting<br>
> location. This seems to cleanly fix the debug-info problem.<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D19708" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19708</a><br>
><br>
> Files:<br>
> include/clang/AST/ExprCXX.h<br>
><br>
><br>
> test/CodeGenCXX/debug-info-member-call.cpp<br>
><br>
> Index: test/CodeGenCXX/debug-info-member-call.cpp<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/CodeGenCXX/debug-info-member-call.cpp<br>
> @@ -0,0 +1,24 @@<br>
> +// RUN: %clang_cc1 -triple x86_64-unknown_unknown -emit-llvm<br>
> -debug-info-kind=standalone -dwarf-column-info %s -o - | FileCheck<br>
> %s<br>
> +void ext();<br>
> +<br>
> +struct Bar {<br>
> + void bar() { ext(); }<br>
> +};<br>
> +<br>
> +struct Foo {<br>
> + Bar *b;<br>
> +<br>
> + Bar *foo() { return b; }<br>
> +};<br>
> +<br>
> +void test(Foo *f) {<br>
> + f->foo()->bar();<br>
> +}<br>
> +<br>
> +// CHECK-LABEL: @_Z4testP3Foo<br>
> +// CHECK: call {{.*}} @_ZN3Foo3fooEv{{.*}}, !dbg ![[CALL1LOC:.*]]<br>
> +// CHECK: call void @_ZN3Bar3barEv{{.*}}, !dbg ![[CALL2LOC:.*]]<br>
> +<br>
> +// CHECK: ![[CALL1LOC]] = !DILocation(line: [[LINE:[0-9]+]], column:<br>
> 6,<br>
> +// CHECK: ![[CALL2LOC]] = !DILocation(line: [[LINE]], column: 13,<br>
> +<br>
> Index: include/clang/AST/ExprCXX.h<br>
> ===================================================================<br>
> --- include/clang/AST/ExprCXX.h<br>
> +++ include/clang/AST/ExprCXX.h<br>
> @@ -145,6 +145,14 @@<br>
> /// FIXME: Returns 0 for member pointer call exprs.<br>
> CXXRecordDecl *getRecordDecl() const;<br>
><br>
> + SourceLocation getExprLoc() const LLVM_READONLY {<br>
> + SourceLocation CLoc = getCallee()->getExprLoc();<br>
> + if (CLoc.isValid())<br>
> + return CLoc;<br>
> +<br>
> + return getLocStart();<br>
> + }<br>
> +<br>
> static bool classof(const Stmt *T) {<br>
> return T->getStmtClass() == CXXMemberCallExprClass;<br>
> }<br>
><br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
><br>
<br>
</div></div><span><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>
</blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></body></html>