delcypher wrote:

@PiJoules 

> This might be a bit orthogonal to this patch, but I think rather than using 
> the mangled dollar scheme used by `__builtin_verbose_trap` to encode the 
> UBSan description, it might be cleaner to instead use dwarf's 
> `DW_AT_description` to actually store the UBSan description. I think ideally 
> we'd have the artificial function you create just be the same as any of the 
> typical ubsan handler functions (like `__ubsan_handle_cfi_check_fail`) and 
> its arguments are the registers, memory locations, or values relevant to this 
> particular ubsan check. Then lldb could just print the function with its 
> arguments as well as a the `DW_AT_description` and from there the dev sees 
> what they need to inspect in lldb to see what's wrong. Since the function is 
> artificial, I think lldb or llvm-symbolizer should also be able to tell that 
> this isn't a real function and instead point to the original function where 
> the actual ubsan intrinsic was located.

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 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.


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

Reply via email to