On Tue, Oct 15, 2024 at 11:50:21AM +0200, Richard Biener wrote:
> > Would it be reasonable to approve this patch now and I try
> > to improve this later?
>
> On the patch itself:
>
> void
> expand_builtin_trap (void)
> {
> + if (warn_trap)
> + {
> + location_t current_location =
Formatting-wise, = shouldn't be at the end of line.
> + linemap_unwind_to_first_non_reserved_loc (line_table,
> input_location,
> + NULL);
> + warning_at (current_location, OPT_Wtrap, "trap generated");
> + }
> +
> if (targetm.have_trap ())
>
> this also diagnoses calls the user puts in by calling __builtin_trap (),
> the documentation should probably mention this. I see the only testcase
> exercises only this path. I have doubts -fsanitize-trap with any
> sanitizer will ever yield a clean binary, so I wonder about practical
> uses besides very small testcases?
Given that even simple
int foo (int x, int y) { return x + y; }
calls it for -fsanitize=undefined -fsanitize-trap=undefined, and more
importantly, we try not to optimize away sanitizer checks based on VRP and
other optimizations at least to some extent, because VRP and other
optimizations optimize on UB not happening while sanitizers try to catch
the UB, I have serious doubts about the warning.
One would need completely different approach, where we try as much as
possible to prove UB can't happen and only warn if we couldn't prove it
can't. Still, there would be tons of false positives where things just
aren't inlined and we can't prove UB won't happen, or warnings on dead
code...
Jakub
