r177164 - Force column info only for direct inlined functions. This should strike

[David Blaikie]

On Wed, Mar 20, 2013 at 2:01 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
> On Mar 19, 2013, at 8:57 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Fri, Mar 15, 2013 at 11:31 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>>
>>> On Mar 15, 2013, at 11:26 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>> On Fri, Mar 15, 2013 at 10:46 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>>
>>>>> On Mar 15, 2013, at 10:25 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>
>>>>>> On Fri, Mar 15, 2013 at 10:09 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>>>> Author: adrian
>>>>>>> Date: Fri Mar 15 12:09:05 2013
>>>>>>> New Revision: 177164
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=177164&view=rev
>>>>>>> Log:
>>>>>>> Force column info only for direct inlined functions. This should strike
>>>>>>> the balance between expected behavior and compatibility with the gdb
>>>>>>> testsuite.
>>>>>>>
>>>>>>> (GDB gets confused if we break an expression into multiple debug
>>>>>>> stmts so we enable this behavior only for inlined functions. For the
>>>>>>> full experience people can still use -gcolumn-info.)
>>>>>>
>>>>>> I'm not sure I understand how this will address the issue... Perhaps
>>>>>> I'm misunderstanding something about this change. I have a few
>>>>>> questions/uncertainties:
>>>>>
>>>>>> 1) You can't really determine in the frontend if a function will
>>>>>> actually be inlined - trying to predicate debug info on that seems
>>>>>> like we'll get surprising/varying debug behavior based on whether the
>>>>>> backend chooses to inline the function
>>>>>
>>>>> That is correct. My thinking was that the backend would only inline at >O0 and if you need more precise debug info you could always use -gcolumn-info. But for those cases where function is inlined at O0 (which happens for inline-attributed functions and is done by the frontend) users will get the expected behavior.
>>>>> This is not very elegant, but I’m trying to find a middle ground between not breaking gdb’s testsuite and fixing the debug experience for inlined functions.
>>>>
>>>> Sure - I'm not suggesting that we should not fix the original bug you
>>>> set out to fix, just that the solution that Eric & I were
>>>> discussing/suggesting wasn't what you've implemented here. I thought
>>>> what we were discussing was changing the backend to not emit separate
>>>> line table entries when the line is the same but the column is
>>>> different. This would be consistent with the debug experience users
>>>> expect (given GDB's lack of column information) in all cases, no
>>>> matter what things were inlined or not inlined, if I'm not mistaken.
>>>
>>> As I mentioned earlier, the problem is that for inlined functions we need to do exactly that otherwise a breakpoint at the inlined function (appearing twice on one line) would be hit only once.
>>
>> Still feels  wrong in some way - mostly because of the inlining
>> possibilities (both not inlining when your frontend heuristic would
>> indicate that it does, and inlining when it doesn't), even at -O0 (we
>> still do inlining, even on non-always_inline functions, at -O0 - for
>> simple functions like get/set, etc)
>
> No need to convince me that it’s ugly...
>
>> FWIW, GCC does exhibit the same behavior you described when it inlines
>> a function (if I use the func(foo(), bar()) example & make foo/bar
>> always_inline, then stepping through goes foo/func call/bar and
>> stepping through without the always_inline goes foo/bar)
>
> That is interesting! Does it create only a single if_stmt in the line table in both cases?

I assume not - if it's jumping back to the call site there must be a
line entry for that attached to some instruction between the inlining
of the first foo and the inlining of the second foo.

>> I would like this to be done right so it behaves appropriately
>> regardless of whether the backend chooses to inline the foo/bar calls.
>
> Absolutely, I just want to make sure to find a solution that works well with gdb _and_ behaves as expected with inlined functions in the func(foo(), foo()) case.

I was picturing something more backend centric than what you've done -
where the backend would notice that two instructions came from
different function calls to the same function & would decide to emit
them separately.

Coming back to the original statement of the problem from one of your
other emails:

"To summarize the problem:
LexicalScopes::getOrCreateInlinedScope() uses the DebugLoc as an index
into InlinedLexicalScopeMap.
DebugLoc::getFromDILocation(MDNode *N) builds a new DebugLoc from
LineNumber, ColumnNumber, Scope, and OrigLocation of InlinedAt. In my
test case, Line, Scope and OrigLocation are all identical for two
inlined calls to the same function at the same line."

OK - so what's the information in the "InlinedAt" node? Can we attach
some more information to that to disambiguate it when we create an
inlined location? We must, I assume, be mapping all inlined
instructions onto the debug loc of the single call instruction - could
we add extra unique information during that step to identify only that
call instruction?

Then when we emit the line table, we could create two different
statement entries.

But now I'm confused. I've gone to try to reproduce this to
investigate it some more & I'm getting unuseful behavior from Clang:

__attribute__((always_inline))
int foo() {
  return 0;
}

int func(int a, int b) {
  return a + b;
}

int main() {
  return func(foo(), foo());
}

Building with clang ToT and running gdb 7.5 "start" then "step" it
starts at the func call and steps straight into it. I assume this is
just due to constant emission code in fast isel (*threatens Dan*). If
I replace "return 0;" with "return j;" and declare a global "int j =
7;" (just to bypass the constant emission code) and repeat the
experiment, I'm step only once into foo, then back to the func
callsite then into the func implementation... so I don't even know how
to get back to a reproduction of the new issue your change created (or
the original issue it was designed to fix).

- David