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

[hfinkel]

Summary

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 {
   4	  void bar() { ext(); }
   5	};
   6	
   7	struct Foo {
   8	  Bar *b;
   9	
  10	  Bar *foo() { return b; }
  11	};
  12	
  13	void test(Foo *f) {
  14	  f->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.

[dblaikie]

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.

[hfinkel]

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.

[echristo]

Seems a much more principled solution yes.

[dblaikie]

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?

lib/CodeGen/CGExprCXX.cpp

 }
 
 // Note: This function also emit constructor calls to support a MSVC
 // extensions allowing explicit constructor function call.
 RValue CodeGenFunction::EmitCXXMemberCallExpr(const CXXMemberCallExpr *CE,
                                               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<BinaryOperator>(callee))
     return EmitCXXMemberPointerCallExpr(CE, ReturnValue);
 
   const MemberExpr *ME = cast<MemberExpr>(callee);
   const CXXMethodDecl *MD = cast<CXXMethodDecl>(ME->getMemberDecl());
 
   if (MD->isStatic()) {
     // The method is static, emit it as we would a regular call.

test/CodeGenCXX/debug-info-member-call.cpp

+// 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,
+