[PATCH] D67723: [DebugInfo] Add option to disable inline line tables.

[Adrian Prantl via cfe-commits]

> On Oct 24, 2019, at 3:02 PM, David Blaikie via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> dblaikie added a comment.
> 
> In D67723#1720509 <https://reviews.llvm.org/D67723#1720509>, @aprantl wrote:
> 
>> In D67723#1720353 <https://reviews.llvm.org/D67723#1720353>, @rnk wrote:
>> 
>>> In D67723#1717468 <https://reviews.llvm.org/D67723#1717468>, @aprantl wrote:
>>> 
>>>> I agree that it would make sense to have a `-ginline-info-threshold=<#insns>` or `-gno-small-inline-functions` with a hardcoded threshold to implement the feature Paul described, and this patch seems to be a step in that direction, with the threshold being hardcoded to 0.
>>> 
>>> 
>>> OK. :)
>>> 
>>>>> We are motivated by one tool in particular at the moment, but if we're going to take the time to add a knob, we might as well make it work for DWARF.
>>>> 
>>>> Here you got me confused: When I read "we might as well make it work for DWARF", I read that as "we should emit the inlined instructions with line 0 under a DWARF debugger tuning". But that reading seems to to contradict your next sentence:
>>>> 
>>>>> If the user cares enough to find this flag, it seems more user friendly to make it behave the same rather than making it format-dependent.
>>>> 
>>>> Can you clarify?
>>> 
>>> If we use line zero for DWARF, gdb will not behave in the way documented by the function attribute in LangRef. I was the one who suggested the wording there, so maybe we could come up with new wording that describes what the user should expect in the debugger when using line zero. However, given the behavior I show below, I have a hard time imagining the use case for it.
>> 
>> 
>> I didn't realize that GDB also had problems; I thought that this was a problem that only affected Windows debuggers.
> 
> 
> I don't think the behavior Reid described would be a "problem" - it seems to me like the only behavior the debugger could provide if those instructions are attributed to line zero.
> 
>> 
>> 
>>> I applied the version of this patch that uses getMergedLocation, compiled this program, and ran it under gdb:
>>> 
>>>  volatile int x;
>>>  static inline void foo() {
>>>    ++x;
>>>    *(volatile int*)0 = 42; // crash
>>>    ++x;
>>>  }
>>>  int main() {
>>>    ++x;  // line 8
>>>    foo();  // line 9
>>>    ++x;
>>>    return x;
>>>  }
>>> 
>>> 
>>> If we apply line zero, the debugger stops on line 8:
>>> 
>>>  Program received signal SIGSEGV, Segmentation fault.
>>>  0x000000000040111e in main () at t.cpp:8
>>>  8         ++x;
>>>  (gdb) bt
>>>  #0  0x000000000040111e in main () at t.cpp:8
>>> 
>>> 
>>> The inline frame is gone, as expected for this flag, but the current location does not reflect the site of the call to `foo`. So, if we want it to behave as documented, we have to put the call site location on some instructions.
>>> 
>>> Alternatively, if I arrange things like this, the crash is attributed to line `return x`, which is completely unrelated to the inline call site:
>>> 
>>>  static inline void foo() {
>>>    ++x;
>>>    if (x) {
>>>      *(volatile int*)0 = 42; // crash
>>>      __builtin_unreachable();
>>>    }
>>>    ++x;
>>>  }
>>> 
>>> 
>>> This means that if line zero is used, the source location shown in the debugger becomes sensitive to code layout, which is arbitrary.
>>> 
>>> These experiments are convincing me that, in general, line zero isn't that helpful for DWARF consumers. If the goal is to get smooth stepping, we may want to refocus on getting reliable is_stmt bits in the line table.
>> 
>> The Swift compiler is far more aggressive in using line 0 than Clang, and consequently LLDB is much better at handling line 0 than even GDB, and that can skew my perception :-)
> 
> What behavior does LLDB have in the example Reid gave?

I short-circuited a little by marking the function always_inline and putting a .loc 1 0 0  before the inlined instructions, so I hope that didn't butcher the example, but I didn't want to wait for clang to compile. LLDB says

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000100000f9f a.out`main at reid.c:0 [opt]

If the intended behavior is to show the crash at the call site like LangRef in the patch suggest then line 0 will certainly not achieve that. I implicitly assumed the wording in LangRef would follow the implementation if we switched to line 0. 

-- adrian

> 
>> Give how popular GDB is, I don't want to intentionally break compatibility with it, so I think this patch is okay. If we wanted we can put an if-debugger-tuning-is-LLDB-getMergedLocation condition in. Otherwise documenting that this is necessary for compatibility with popular debuggers, seems fine to me, too.
> 
> 
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D67723/new/
> 
> https://reviews.llvm.org/D67723
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191024/cf0b23d0/attachment-0001.html>