<HTML><BODY><div><div>>Пятница, 17 июля 2020, 19:42 +03:00 от David Blaikie <dblaikie@gmail.com>:</div><div class="js-helper js-readmsg-msg"><div>></div><div>>On Fri, Jul 17, 2020 at 12:03 AM Fangrui Song <<a href="/compose?To=maskray@google.com">maskray@google.com</a>> wrote:</div><div><div id="style_15950041601572241378_BODY">>><br>>> Thanks for the write-up!<br>>><br>>> On 2020-07-16, David Blaikie wrote:<br>>> >In short: Perhaps we should switch lld to the bfd-style tombstoning<br>>> >behavior for a release or two, letting users opt-in to testing with the new<br>>> >-1/-2 tombstoning in the interim, before switching to the new tombstone by<br>>> >default (while still having the flag to switch back when users find<br>>> >surprise places that can't handle the new behavior).<br>>> ><br>>> >In long:<br>>> ><a href="https://reviews.llvm.org/D81784" target="_blank">https://reviews.llvm.org/D81784</a> and follow-on patches modified the behavior<br>>> >of lld with regards to resolving relocations from debug sections to dead<br>>> >code (either comdat deduplicated, or gc-sections use).<br>>> ><br>>> >A very quick summary of the situation:<br>>> ><br>>> >Original Behavior:<br>>> ><br>>> > - bfd: 1 for debug_ranges(0 would prematurely terminate the list), 0<br>>> > elsewhere<br>>> > - gold/lld: 0+addend everywhere<br>>> ><br>>> >Limitations/bugs:<br>>> ><br>>> > - bfd/gold/lld<br>>> > - doesn't support 0 as a valid executable address without ambiguities<br>>> > - gold/lld<br>>> > - ambiguities with large gc'd functions combined with a .text mapping<br>>> > that starts in relative low addresses<br>>> > - premature debug_range termination with zero-length functions (Clang<br>>> > produces these with __builtin_unreachable or non-void return<br>>> >type functions<br>>> > without a return statement)<br>>> ><br>>> >New behavior:<br>>> ><br>>> > - -2 for DWARFv4 debug_loc, debug_ranges (-1 is a base address specifier<br>>> > there)<br>>> > - -1 elsewhere<br>>> > - linker flag to customize to other values if desired<br>>> ><br>>> >Known issues:<br>>> ><br>>> > - lldb's line table parsing can't handle -1 well at all (essentially<br>>> > unusable)<br>>><br>>> Pavel Labath will fix this soon <a href="https://reviews.llvm.org/D83957" target="_blank">https://reviews.llvm.org/D83957</a><br>>> This is an unhandled address-space wraparound problem.<br>>> This pattern is potentially common - and other downstream DWARF<br>>> consumers might make similar line table handling mistakes.<br>><br>>That's the thing - I'm not sure we can really classify them as<br>>"mistakes". I think bfd.ld's tombstoning behavior is about the only<br>>thing we can reasonably say DWARF consumers should /probably/ be<br>>expected to handle - and I'd imagine in many cases they haven't been<br>>written intentionally to handle it, but whatever behavior they have<br>>has accidentally been sufficient for their needs.<br><br>>> > - gdb's line table parsing ends up with different handling when breaking<br>>> > on gc'd functions (minor functionality issue)<br>>><br>>> This is just a behavior difference, not affecting users.<br>>> It did break a test if linked with LLD (gdb intrinsically has lots of<br>>> failing tests even if built with GCC+GNU ld).<br>>><br>>> Previous behavior (when an address is zero): a breakpoint on a<br>>> --gc-sections discarded function will be redirected to a larger line<br>>> number with debug info, even if that line can be an unrelated different<br>>> function.<br>>> New behavior is that the breakpoint is on a wrapped-around small address.<br>>><br>>> GDB 9.3 will restore the previous behavior<br>>> (<a href="https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=a8caed5d7faa639a1e6769eba551d15d8ddd9510" target="_blank">https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=a8caed5d7faa639a1e6769eba551d15d8ddd9510</a>)<br>>><br>>> ><br>>> >I think there's enough risk in this work (even given the small number of<br>>> >bugs found so far), given there's a pretty wide array of debug info<br>>> >consumers out there, that we should change lld's default to match the<br>>> >long-lived bfd strategy. This would address my original motivation for<br>>> >raising all this (empty functions prematurely terminating the list), while<br>>> >letting users who want to experiment with it, or need it (like Alexey), can<br>>> >opt-in to the -1/-2 behavior.<br>>><br>>> I think we can only confidently say that there is enough risk in using<br>>> tombstone value -1 in .debug_line, but I'd not say tombstone value -1 in<br>>> other .debug_* can cause problems.<br><br>>Given how many DWARF parsers we've had to cleanup or migrate off in<br>>transitioning to DWARFv5 inside Google, I think that's a fair bit of<br>>evidence the set of parsers isn't as narrow/closed as we'd like and<br>>thus the number of places that might have issues isn't known/easily<br>>exhaustively tested. So I'm not so concerned about the bugs we've<br>>seen, but what that might indicate about the things that we don't know<br>>about and can't test (because we don't know about them). (of course,<br>>the flipside of that is that if we don't know about them, we can't<br>>tell people who own them to go check if they work with this opt-in<br>>feature - so they'll break whenever we turn this on by default - but<br>>perhaps in the interim we can get at least a few big LLD customers to<br>>deploy the feature and flush out some of the issues - happy for Google<br>>to use this internally, hopefully we can encourage Apple folks, Sony<br>>of course already has this semantic so nothing to find there most<br>>likely - Chromium, maybe Firefox, etc)</div></div></div></div><div> </div><div>From the other side — when we already switched new behavior ON as default —</div><div>then it is easier to discover all these unknown cases. We have an option restoring</div><div>old behavior(which should be fine for all of the current users). Thus, everybody</div><div>who needs old behavior is able to continue using it. </div><div> </div><div>That makes transition more understandable. All problems would be explicitly seen. Then,</div><div>If we will know about new problems - we could adapt the current solution. Similar to this suggestion:</div><div> </div><div>a) .debug_ranges&.debug_loc => -2, .debug_line => 0, other .debug_* -> -1</div><div> </div><div>This would help while dwarf users are preparing their code to the new solution.</div><div> </div><div>Is option restoring old behavior not enough to solve problems caused by new tombstone value?</div><div> </div><div><br>>> With consideration for satefy for the upcoming release/11.x, we can make<br>>> two choices:<br>>><br>>> a) .debug_ranges&.debug_loc => -2, .debug_line => 0, other .debug_* -> -1<br>>> b) .debug_ranges&.debug_loc => -2, other .debug_* => 0<br>>><br>>> Delaying .debug_line => -1 for one or two release sounds good to me.<br>>> So LLD 11 or 12 linked binaries can be debugged by LLDB 10. This is a<br>>> nice property.<br>>><br>>> This write-up proposes b), but I'd say a) is likely sufficient. With the<br>>> available information, I cannot yet say that a) will have more risk.<br>><br>>Risk is about the unknowns - and it still seems like a lot of<br>>unknowns. While there are probably many more consumers that read<br>>.debug_line than other sections, reading debug_info (for instance) is<br>>necessary for inline frames in symbolizing - still probably one of the<br>>most common uses of DWARF I'd guess. (what about stack unwinding using<br>>debug_frame? that'd worry me a bit if anyone got /that/ wrong because<br>>of this change)<br>><br>>> > - chromium/firefox have some tools that were broken:<br>>> > <a href="https://bugs.chromium.org/p/chromium/issues/detail?id=1102223#c5" target="_blank">https://bugs.chromium.org/p/chromium/issues/detail?id=1102223#c5</a><br>>><br>>> This is potentially related to other .debug_* (not .debug_line)<br>>> I hope Chromium developers can chime in here:) The breakage was<br>>> unfortunate but I don't know how we could have avoided that. IMHO this<br>>> is no different from "clang started to emit a new DW_FORM_* and a<br>>> postprocessing tool of .debug chokes on that" Whether we want to<br>>> suppress that particular DW_FORM_* definitely should depend on how<br>>> likely it can cause problems, but we can't yet say we have to hold off<br>>> on a feature for a solved (precisely, mitigated) problem.<br>><br>>LLVM has no custom forms and I'd be super cautious about adding any<br>>that were on by default because of how bad that breakage would be.<br>><br>>I'm not so concerned about the problems we know - but what they tell<br>>us about the problems that might arise from use cases we don't know.<br>>All the other projects out there that might have custom DWARF parsers<br>>to do some ad-hoc things.<br>><br>>(also, ultimately - given how far-reaching this is, I think we'll want<br>>some tidier flags that are more user-focussed. I'd hope for a flag<br>>that gives BFD-like semantics (though I'd be OK with fixing debug_loc<br>>(using 1 instead of 0) to work the same as debug_ranges while we're<br>>there - a minor divergence from BFD, but highly likely to not cause<br>>problems/fall out naturally from a simple implementation of parsing<br>>that section) - something that's been in-use and tested by basically<br>>everyone for decades. And another flag for the new semantics (-2 for<br>>debug_loc/debug_ranges, -1 everywhere else). Customizable per-section<br>>expression-based support I think is a recipe for platform divergence &<br>>I'd rather it not be available/supported at all, but if you really<br>>want to keep it in, I'd at least rather it not be the feature we<br>>promote to users about how they can test/opt in/out of the behavior<br>>when they're seeing breakages or want to test the future semantics)<br>><br>>> >I'm not sure how to get the word out to DWARF consumers that they should<br>>> >consider this new experimental behavior. Ray's done a good job<br>>> >evangelizing/discussing this with gdb and lldb at least - and of course<br>>> >having turned it on by default briefly has found some users (like Chromium)<br>>> >that we probably wouldn't have found no matter how long we left this as an<br>>> >experimental option... so some things are going to break when we switch no<br>>> >matter what.<br>>><br>>> Thank you for following up with some GNU folks on their lists!<br>>> If folks want to follow along the thread:<br>>> <a href="https://sourceware.org/pipermail/binutils/2020-June/111376.html" target="_blank">https://sourceware.org/pipermail/binutils/2020-June/111376.html</a><br>>><br>>> We have informed binutils, elfutils-devel (elfutils has a few debug<br>>> tools) and gdb. I don't recall that anyone has thought about problems<br>>> with a tombstone value.<br>>><br>>> ><br>>> >P.S: Sony's already been using the -1 technique with their debugger and<br>>> >linker for a while, so they may want to keep this on by default for SCE -<br>>> >but I'm not sure how to do that in-tree.<br>>> ><br>>> ><br>>> >Clang doesn't know which lld<br>>> >version it's running, so whether the flag can be specified, I would think?<br>>> >(so it'd be hard to have Clang go "if SCE and LLD, pass the flag to use<br>>> >-1", I think) - if there is a way to make that decision in the compiler<br>>.> >driver+linker, then we'd have a question of "default new behavior except<br>>> >when tuning for LLDB and GDB" or "default bfd behavior except when tuning<br>>> >for SCE".<br>>><br>>> I've been involed in another thread on SHF_LINK_ORDER (<a href="https://sourceware.org/pipermail/binutils/2020-">https://sourceware.org/pipermail/binutils/2020-</a></div><div>>July/112415.html ).<br>>> We may need a way to tell codegen about the used linker.<br>>><br>>> pcc proposed -mbinutils-version= - This is nice in that some MC<br>>> decisions related to -fno-integrated-as can use this option as well.<br>>> jyknight proposed -mlinker-version= and syntax like -fuse-ld=bfd:2.34<br>>><br>>> This may get more complex if the generated object file want to be linked</div><div>>> with more than one linker. This discussion probably deserves its own<br>>> thread.</div><div> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Alexey Lapshin</div></div></div><div> </div></div></BODY></HTML>