This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by hfinkel on Apr 28 2016, 8:42 PM.

Details

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.

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 55524.Apr 28 2016, 8:42 PM
hfinkel retitled this revision from to [CGDebugInfo] Generate debug info for member calls in the context of the callee expression.
hfinkel updated this object.
hfinkel added a subscriber: cfe-commits.
dblaikie edited edge metadata.Apr 29 2016, 8:45 AM
dblaikie added a subscriber: 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 updated this revision to Diff 55610.Apr 29 2016, 9:19 AM
hfinkel edited edge metadata.

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 edited edge metadata.Apr 29 2016, 9:35 AM
echristo added a subscriber: echristo.

Seems a much more principled solution yes.

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?

This revision was automatically updated to reflect the committed changes.