> On Oct 24, 2019, at 3:02 PM, David Blaikie via Phabricator
> <revi...@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
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
