<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 24, 2019 at 3:24 PM Adrian Prantl via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><br><div><br><blockquote type="cite"><div>On Oct 24, 2019, at 3:02 PM, David Blaikie via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:</div><br><div><div>dblaikie added a comment.<br><br>In D67723#1720509 <<a href="https://reviews.llvm.org/D67723#1720509" target="_blank">https://reviews.llvm.org/D67723#1720509</a>>, @aprantl wrote:<br><br><blockquote type="cite">In D67723#1720353 <<a href="https://reviews.llvm.org/D67723#1720353" target="_blank">https://reviews.llvm.org/D67723#1720353</a>>, @rnk wrote:<br><br><blockquote type="cite">In D67723#1717468 <<a href="https://reviews.llvm.org/D67723#1717468" target="_blank">https://reviews.llvm.org/D67723#1717468</a>>, @aprantl wrote:<br><br><blockquote type="cite">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.<br></blockquote><br><br>OK. :)<br><br><blockquote type="cite"><blockquote type="cite">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.<br></blockquote><br>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:<br><br><blockquote type="cite">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.<br></blockquote><br>Can you clarify?<br></blockquote><br>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.<br></blockquote><br><br>I didn't realize that GDB also had problems; I thought that this was a problem that only affected Windows debuggers.<br></blockquote><br><br>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.<br><br><blockquote type="cite"><br><br><blockquote type="cite">I applied the version of this patch that uses getMergedLocation, compiled this program, and ran it under gdb:<br><br> volatile int x;<br> static inline void foo() {<br> ++x;<br> *(volatile int*)0 = 42; // crash<br> ++x;<br> }<br> int main() {<br> ++x; // line 8<br> foo(); // line 9<br> ++x;<br> return x;<br> }<br><br><br>If we apply line zero, the debugger stops on line 8:<br><br> Program received signal SIGSEGV, Segmentation fault.<br> 0x000000000040111e in main () at t.cpp:8<br> 8 ++x;<br> (gdb) bt<br> #0 0x000000000040111e in main () at t.cpp:8<br><br><br>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.<br><br>Alternatively, if I arrange things like this, the crash is attributed to line `return x`, which is completely unrelated to the inline call site:<br><br> static inline void foo() {<br> ++x;<br> if (x) {<br> *(volatile int*)0 = 42; // crash<br> __builtin_unreachable();<br> }<br> ++x;<br> }<br><br><br>This means that if line zero is used, the source location shown in the debugger becomes sensitive to code layout, which is arbitrary.<br><br>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.<br></blockquote><br>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 :-)<br></blockquote><br>What behavior does LLDB have in the example Reid gave?<br></div></div></blockquote><div><br></div><div>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</div><div style="font-size:10px"><br></div><div><div style="margin:0px;font-stretch:normal;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures">* thread #1, queue = </span><span style="font-variant-ligatures:no-common-ligatures;color:rgb(57,192,38)">'com.apple.main-thread'</span><span style="font-variant-ligatures:no-common-ligatures">, stop reason = </span><span style="font-variant-ligatures:no-common-ligatures;color:rgb(202,51,35)">EXC_BAD_ACCESS (code=1, address=0x0)</span></div><div style="margin:0px;font-stretch:normal;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> frame #0: </span><span style="font-variant-ligatures:no-common-ligatures;color:rgb(170,171,37)">0x0000000100000f9f</span><span style="font-variant-ligatures:no-common-ligatures"> a.out`main at </span><span style="font-variant-ligatures:no-common-ligatures;color:rgb(56,185,199)">reid.c</span><span style="font-variant-ligatures:no-common-ligatures">:</span><span style="font-variant-ligatures:no-common-ligatures;color:rgb(170,171,37)">0</span><span style="font-variant-ligatures:no-common-ligatures"> [opt]</span></div></div></div></div></blockquote><div><br></div><div>I meant more as a comparison with the stepping (rather than backtracing) behavior that Reid mentioned from GDB.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div>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. <br></div></div></div></blockquote><div><br>Right - I think there's some reasonable discussion about what would be suitable/useful behavior - and line zero isn't necessarily an experience a user would want in this situation. (if there's no user for the feature, why build it - if we have a quality/space tradeoff that seems useful to CodeView users & we know inlining info is expensive in DWARF too (maybe not as expensive as CodeView, I'm not sure) & so DWARF users /might/ want the same tradeoff, seems like it might be worth providing the same feature/behavior in both)<br><br>Put another way: I don't know/don't think Reid's users would want the feature using line zero even if CV had support for something equivalent to DWARF's line zero. The "attribute it to the call site" behavior is more meaningful/useful than line zero to a user.<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div><div></div><div><br></div><div>-- adrian</div><div><br></div><blockquote type="cite"><div><div><br><blockquote type="cite">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.<br></blockquote><br><br><br><br>Repository:<br> rG LLVM Github Monorepo<br><br>CHANGES SINCE LAST ACTION<br> <a href="https://reviews.llvm.org/D67723/new/" target="_blank">https://reviews.llvm.org/D67723/new/</a><br><br><a href="https://reviews.llvm.org/D67723" target="_blank">https://reviews.llvm.org/D67723</a><br><br><br><br></div></div></blockquote></div><br></div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>