Re: [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" 
> To: "Hal Finkel" 
> Cc: "Richard Smith" , "Adrian Prantl"
> , "Duncan P. N. Exon Smith"
> , "Eric Christopher" ,
> "Jun Bum Lim" , "cfe-commits"
> ,
> reviews+d19708+public+e9ddc42503732...@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 < hfin...@anl.gov > wrote:

> > - Original Message -
> 

> > > From: "David Blaikie" < dblai...@gmail.com >
> 
> > > To: reviews+d19708+public+e9ddc42503732...@reviews.llvm.org ,
> > > "Hal
> > > Finkel" < hfin...@anl.gov >
> 
> > > Cc: "Richard Smith" < rich...@metafoo.co.uk >, "Adrian Prantl" <
> > > apra...@apple.com >, "Duncan P. N. Exon Smith"
> 
> > > < dexonsm...@apple.com >, "Eric Christopher" < echri...@gmail.com
> > > >, "Jun Bum Lim" < junb...@codeaurora.org >,
> 
> > > "cfe-commits" < cfe-commits@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
> 
> > > 

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

[Hal Finkel via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL270775: [CGDebugInfo] Modify the preferred expression 
location for member calls. (authored by hfinkel).

Changed prior to commit:
  http://reviews.llvm.org/D19708?vs=55610=58521#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D19708

Files:
  cfe/trunk/include/clang/AST/ExprCXX.h
  cfe/trunk/test/CodeGenCXX/debug-info-member-call.cpp

Index: cfe/trunk/include/clang/AST/ExprCXX.h
===
--- cfe/trunk/include/clang/AST/ExprCXX.h
+++ cfe/trunk/include/clang/AST/ExprCXX.h
@@ -143,6 +143,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;
   }
Index: cfe/trunk/test/CodeGenCXX/debug-info-member-call.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-member-call.cpp
+++ cfe/trunk/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: cfe/trunk/include/clang/AST/ExprCXX.h
===
--- cfe/trunk/include/clang/AST/ExprCXX.h
+++ cfe/trunk/include/clang/AST/ExprCXX.h
@@ -143,6 +143,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;
   }
Index: cfe/trunk/test/CodeGenCXX/debug-info-member-call.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-member-call.cpp
+++ cfe/trunk/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,
+
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

On Mon, May 2, 2016 at 1:17 PM, Hal Finkel  wrote:

> - Original Message -
> > From: "David Blaikie" 
> > To: reviews+d19708+public+e9ddc42503732...@reviews.llvm.org, "Hal
> Finkel" 
> > Cc: "Richard Smith" , "Adrian Prantl" <
> apra...@apple.com>, "Duncan P. N. Exon Smith"
> > , "Eric Christopher" , "Jun
> Bum Lim" ,
> > "cfe-commits" 
> > 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.


> 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.

- 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@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 

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

[Hal Finkel via cfe-commits]

Ping.

Thanks again,
Hal

- Original Message -
> From: "Hal Finkel via cfe-commits" 
> To: "David Blaikie" 
> Cc: "Jun Bum Lim" , "Richard Smith" 
> , "cfe-commits"
> , 
> reviews+d19708+public+e9ddc42503732...@reviews.llvm.org
> Sent: Monday, May 2, 2016 3:17:22 PM
> Subject: Re: [PATCH] D19708: [CGDebugInfo] Generate debug info for member 
> calls in the context of the callee
> expression
> 
> - Original Message -
> > From: "David Blaikie" 
> > To: reviews+d19708+public+e9ddc42503732...@reviews.llvm.org, "Hal
> > Finkel" 
> > Cc: "Richard Smith" , "Adrian Prantl"
> > , "Duncan P. N. Exon Smith"
> > , "Eric Christopher" ,
> > "Jun Bum Lim" ,
> > "cfe-commits" 
> > 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. Given that this is still pretty
> simple, I think that covering it directly seems reasonable.
> 
> > 
> > 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.
> 
> 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@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
> > 

Re: [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" 
> To: reviews+d19708+public+e9ddc42503732...@reviews.llvm.org, "Hal Finkel" 
> 
> Cc: "Richard Smith" , "Adrian Prantl" 
> , "Duncan P. N. Exon Smith"
> , "Eric Christopher" , "Jun Bum 
> Lim" ,
> "cfe-commits" 
> 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. Given that this is still pretty simple, I think that covering it directly 
seems reasonable.

> 
> 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.

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@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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> 
> 
> 

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

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

[David Blaikie via cfe-commits]

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).

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, 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@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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Christopher via cfe-commits]

echristo added a subscriber: echristo.
echristo added a comment.

Seems a much more principled solution yes.


http://reviews.llvm.org/D19708



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Christopher via cfe-commits]

Seems a much more principled solution yes.

On Fri, Apr 29, 2016, 9:19 AM Hal Finkel  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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hal Finkel via cfe-commits]

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;
   }


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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

As mentioned in the bug, I /think/ the right thing to do here is to change
the preferred location of the CXXMemberCallExpr so that we improve
diagnostics as well. Any place where the preferred location of an
expression and the debug location of an expression are differing I'd really
like a pretty deep discussion of why those two uses cases should differ.

Eg, where this location turns up in diagnostics:

blaikie@blaikie-linux:~/dev$ cat loc.cpp
struct foo {
  const foo *x() const;
  void y();
};

void f(const foo *g) {
  g->x()->y();
  g->x()->x()->y();
}
blaikie@blaikie-linux:~/dev$ clang++-tot loc.cpp -fsyntax-only
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();
  ^~
...
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();
  ^~~
...

It seems like pointing to the 'y' in both these cases would be an
improvement? The source range highlighting the 'this' part of the
expression is certainly helpful, but could still be confusing if the
functions had more similar/the same name, etc.



On Thu, Apr 28, 2016 at 8:42 PM, Hal Finkel via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> hfinkel created this revision.
> hfinkel added reviewers: rsmith, aprantl, dexonsmith, dblaikie, echristo.
> hfinkel added a subscriber: cfe-commits.
> Herald added subscribers: joker.eph, mcrosier.
>
> We currently generate debug info for member calls in the context of the
> call expression. For member calls, this has an odd effect, which becomes
> especially obvious when generating source locations for optimizer-feedback
> remarks regarding inlining.
>
> Given this:
>
>   $ cat -n /tmp/i.cpp
>  1  void ext();
>  2
>  3  struct Bar {
>  4void bar() { ext(); }
>  5  };
>  6
>  7  struct Foo {
>  8Bar *b;
>  9
> 10Bar *foo() { return b; }
> 11  };
> 12
> 13  void test(Foo *f) {
> 14f->foo()->bar();
> 15  }
>
>   $ clang -g /tmp/i.cpp -S -emit-llvm -o -
>
>   define void @_Z4testP3Foo(%struct.Foo* %f) #0 !dbg !6 {
> ...
> %call = call %struct.Bar* @_ZN3Foo3fooEv(%struct.Foo* %0), !dbg !27
> call void @_ZN3Bar3barEv(%struct.Bar* %call), !dbg !28
> ...
>   !27 = !DILocation(line: 14, column: 3, scope: !6)
>   !28 = !DILocation(line: 14, column: 3, scope: !29)
>
> but we want instead for the calls to point to the callee expressions
> (foo() and bar() in this case). With this change, that's what happens.
>
> Fixes PR27567.
>
> http://reviews.llvm.org/D19708
>
> Files:
>   lib/CodeGen/CGExprCXX.cpp
>   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: lib/CodeGen/CGExprCXX.cpp
> ===
> --- lib/CodeGen/CGExprCXX.cpp
> +++ lib/CodeGen/CGExprCXX.cpp
> @@ -107,6 +107,10 @@
>ReturnValueSlot
> ReturnValue) {
>const Expr *callee = CE->getCallee()->IgnoreParens();
>
> +  // The debug information for the call instruction should point to the
> callee
> +  // expression.
> +  ApplyDebugLocation DL(*this, callee);
> +
>if (isa(callee))
>  return EmitCXXMemberPointerCallExpr(CE, ReturnValue);
>
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hal Finkel via cfe-commits]

hfinkel created this revision.
hfinkel added reviewers: rsmith, aprantl, dexonsmith, dblaikie, echristo.
hfinkel added a subscriber: cfe-commits.
Herald added subscribers: joker.eph, mcrosier.

We currently generate debug info for member calls in the context of the call 
expression. For member calls, this has an odd effect, which becomes especially 
obvious when generating source locations for optimizer-feedback remarks 
regarding inlining.

Given this:

  $ cat -n /tmp/i.cpp 
 1  void ext();
 2  
 3  struct Bar {
 4void bar() { ext(); }
 5  };
 6  
 7  struct Foo {
 8Bar *b;
 9  
10Bar *foo() { return b; }
11  };
12  
13  void test(Foo *f) {
14f->foo()->bar();
15  }

  $ clang -g /tmp/i.cpp -S -emit-llvm -o - 
  
  define void @_Z4testP3Foo(%struct.Foo* %f) #0 !dbg !6 {
...
%call = call %struct.Bar* @_ZN3Foo3fooEv(%struct.Foo* %0), !dbg !27
call void @_ZN3Bar3barEv(%struct.Bar* %call), !dbg !28
...
  !27 = !DILocation(line: 14, column: 3, scope: !6)
  !28 = !DILocation(line: 14, column: 3, scope: !29)

but we want instead for the calls to point to the callee expressions (foo() and 
bar() in this case). With this change, that's what happens.

Fixes PR27567.

http://reviews.llvm.org/D19708

Files:
  lib/CodeGen/CGExprCXX.cpp
  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: lib/CodeGen/CGExprCXX.cpp
===
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -107,6 +107,10 @@
   ReturnValueSlot ReturnValue) {
   const Expr *callee = CE->getCallee()->IgnoreParens();
 
+  // The debug information for the call instruction should point to the callee
+  // expression.
+  ApplyDebugLocation DL(*this, callee);
+
   if (isa(callee))
 return EmitCXXMemberPointerCallExpr(CE, ReturnValue);
 


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: lib/CodeGen/CGExprCXX.cpp
===
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -107,6 +107,10 @@
   ReturnValueSlot ReturnValue) {
   const Expr *callee = CE->getCallee()->IgnoreParens();
 
+  // The debug information for the call instruction should point to the callee
+  // expression.
+  ApplyDebugLocation DL(*this, callee);
+
   if (isa(callee))
 return EmitCXXMemberPointerCallExpr(CE, ReturnValue);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits