aprantl added a comment. 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 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 :-) 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
