ilovepi wrote: > I wouldn't say it's orthogonal. It's a very interesting implementation idea > but there are several reasons why we don't want to pursue it right now. > > 1. What you are proposing is significantly more complicated than what is > implemented in this PR. This work being part of a GSoC project means it is > time limited and we don't have time to fully implement what you are proposing. > 2. The "managed dollar scheme" used by `__builtin_verbose_trap` is an already > a well established scheme for encoding trap reasons that LLDB already > understands. This approach is also used by `-fbounds-safety` (not upstreamed > yet), and Swift. > 3. An advantage of the current scheme in this PR is that the coupling between > the debugger and compiler is very weak which means it is robust against > mixing compiler and debugger versions (something that unfortunately happens > in practice, despite my best efforts). The coupling is "weak" compared to > what you are proposing because the DWARF consumer (typically a debugger like > LLDB) just needs to parse the fake function name. The downside though is the > "trap reason" is limited to being a static string and thus can only include > information known at compile time. > 4. Your proposed scheme has a much tigher coupling between the debugger and > compiler because you're proposing different function names for each UBSan > trap type and arguments to the fake functions (essentially a more complicated > ABI). This is much less robust against mixing debugger and compiler versions. > OTOH I think being able to refer dynamic values would be quite valuable. >
I think the coupling is only between the compiler and DWARF, since the approach @PiJoules is describing only requires this to be spelled out in the debug information (unless I've misunderstood). Existing debuggers don't need to be taught how to handle DWARF encoding, whether the function is synthetic or not, so I don't see LLDB, GDB , or other debuggers that consume DWARF correctly needing to explicitly support anything new. > I think this PR is a substantial improvement over the status quo so I think > we should land this (once we've addressed other reviewer comments) but we > (the compiler) should reserve the right to change the implementation. I agree that we probably don't want to derail this patch right now, unless the new mechanism is simple/easy to implement (for a GSOC contributor I'd say it's definitely not). However, I do think that the proposed mechanism is something to strongly consider. FWIW, GSOC is somewhat flexible, since up til the Midterm evaluations we are able to adjust the length and scope of a project, and we should quickly determine if this direction warrants doing that (and both contributor and mentors should agree that its the right approach for their project). That said, with my maintainer's hat on, I'd prefer us to pursue solutions we don't think we'll be quickly replacing. I'd suggest discussing the idea further w/ @PiJoules to sketch out the details a bit more and then decide if this idea something to pursue down the line or prioritize now. If we think we should pivot now, then its going to be important to reach out to the LLVM GSOC coordinator to adapt the length/scope of the project prior to the midterm, which IIRC is next week (July 14-18). https://github.com/llvm/llvm-project/pull/145967 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
