warning option for traps (-Wtrap)

There is code which should not fail at run-time.  For this,
it is helpful to get a warning when a compiler inserts traps
(e.g. sanitizers, hardbools, __builtin_trap(), etc.).

Having a warning for this also has many other use cases, e.g.
one can use it with some sanitizer to rule out that some
piece of code has certain undefined behavior such as
signed overflow or undefined behavior in left-shifts
(one gets a warning if the optimizer does not prove the
trap is dead and it is emitted). 

Another potential use case could be writing tests.


Bootstrapped and regression tested on x64_84.


    Add warning option that warns when a trap is generated.
    
    This adds a warning option -Wtrap that is emitted in
    expand_builtin_trap.  It can be used to verify that traps
    are generated or - more importantly - are not generated
    under various conditions, e.g. for UBSan with -fsanitize-trap,
    hardbools, etc.
    
    gcc/ChangeLog:
            * common.opt (Wtrap): Add warning.
            * builtins.cc (expand_builtin_trap): Add warning.
            * doc/invoke.texi (Wtrap): Document warning.
    
    gcc/testsuite/ChangeLog:
            * gcc.dg/Wtrap.c: New test.

> Am 12.10.2024 um 16:43 schrieb Martin Uecker <uecker@tugraz.at>:
> 
> 
> There is code which should not fail at run-time.  For this,
> it is helpful to get a warning when a compiler inserts traps
> (e.g. sanitizers, hardbools, __builtin_trap(), etc.).
> 
> Having a warning for this also has many other use cases, e.g.
> one can use it with some sanitizer to rule out that some
> piece of code has certain undefined behavior such as
> signed overflow or undefined behavior in left-shifts
> (one gets a warning if the optimizer does not prove the
> trap is dead and it is emitted).
> 
> Another potential use case could be writing tests.
> 
> 
> Bootstrapped and regression tested on x64_84.
> 
> 
>    Add warning option that warns when a trap is generated.
> 
>    This adds a warning option -Wtrap that is emitted in
>    expand_builtin_trap.  It can be used to verify that traps
>    are generated or - more importantly - are not generated
>    under various conditions, e.g. for UBSan with -fsanitize-trap,
>    hardbools, etc.

Isn’t it better to diagnose with more context from the callers that insert the trap?

>    gcc/ChangeLog:
>            * common.opt (Wtrap): Add warning.
>            * builtins.cc (expand_builtin_trap): Add warning.
>            * doc/invoke.texi (Wtrap): Document warning.
> 
>    gcc/testsuite/ChangeLog:
>            * gcc.dg/Wtrap.c: New test.
> 
> diff --git a/gcc/builtins.cc b/gcc/builtins.cc
> index 37c7c98e5c7..76268f62481 100644
> --- a/gcc/builtins.cc
> +++ b/gcc/builtins.cc
> @@ -5896,6 +5896,14 @@ expand_builtin_assume_aligned (tree exp, rtx target)
> void
> expand_builtin_trap (void)
> {
> +  if (warn_trap)
> +    {
> +      location_t current_location =
> +    linemap_unwind_to_first_non_reserved_loc (line_table, input_location,
> +                          NULL);
> +    warning_at (current_location, OPT_Wtrap, "trap generated");
> +    }
> +
>   if (targetm.have_trap ())
>     {
>       rtx_insn *insn = emit_insn (targetm.gen_trap ());
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 12b25ff486d..1cc4936ebbf 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -812,6 +812,10 @@ Wsystem-headers
> Common Var(warn_system_headers) Warning
> Do not suppress warnings from system headers.
> 
> +Wtrap
> +Common Var(warn_trap) Warning
> +Warn whenever a trap is generated.
> +
> Wtrampolines
> Common Var(warn_trampolines) Warning
> Warn whenever a trampoline is generated.
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 575dffd2a2f..68833c4350f 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -8784,6 +8784,14 @@ made up of data only and thus requires no special treatment.  But, for
> most targets, it is made up of code and thus requires the stack to be
> made executable in order for the program to work properly.
> 
> +@opindex Wtrap
> +@opindex Wno-trap
> +@item -Wtrap
> +Warn when explicit traps are generated.  Traps may be generated for
> +a variety of reasons, e.g. when using the undefined behavior sanitizer
> +together with @option{-fsanitize-trap=undefined}.
> +
> +
> @opindex Wfloat-equal
> @opindex Wno-float-equal
> @item -Wfloat-equal
> diff --git a/gcc/testsuite/gcc.dg/Wtrap.c b/gcc/testsuite/gcc.dg/Wtrap.c
> new file mode 100644
> index 00000000000..2f2878f9d45
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/Wtrap.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wtrap" } */
> +
> +void foo(void)
> +{
> +    __builtin_trap();    /* { dg-warning "trap generated" } */
> +}
> +
>

Am Samstag, dem 12.10.2024 um 18:44 +0200 schrieb Richard Biener:
> 
> > Am 12.10.2024 um 16:43 schrieb Martin Uecker <uecker@tugraz.at>:
> > 
> > 
> > There is code which should not fail at run-time.  For this,
> > it is helpful to get a warning when a compiler inserts traps
> > (e.g. sanitizers, hardbools, __builtin_trap(), etc.).
> > 
> > Having a warning for this also has many other use cases, e.g.
> > one can use it with some sanitizer to rule out that some
> > piece of code has certain undefined behavior such as
> > signed overflow or undefined behavior in left-shifts
> > (one gets a warning if the optimizer does not prove the
> > trap is dead and it is emitted).
> > 
> > Another potential use case could be writing tests.
> > 
> > 
> > Bootstrapped and regression tested on x64_84.
> > 
> > 
> >    Add warning option that warns when a trap is generated.
> > 
> >    This adds a warning option -Wtrap that is emitted in
> >    expand_builtin_trap.  It can be used to verify that traps
> >    are generated or - more importantly - are not generated
> >    under various conditions, e.g. for UBSan with -fsanitize-trap,
> >    hardbools, etc.
> 
> Isn’t it better to diagnose with more context from the callers that insert the trap?

More context would be better.  Should there be additional
arguments when creating the call to the builtin?

Martin


> 
> >    gcc/ChangeLog:
> >            * common.opt (Wtrap): Add warning.
> >            * builtins.cc (expand_builtin_trap): Add warning.
> >            * doc/invoke.texi (Wtrap): Document warning.
> > 
> >    gcc/testsuite/ChangeLog:
> >            * gcc.dg/Wtrap.c: New test.
> > 
> > diff --git a/gcc/builtins.cc b/gcc/builtins.cc
> > index 37c7c98e5c7..76268f62481 100644
> > --- a/gcc/builtins.cc
> > +++ b/gcc/builtins.cc
> > @@ -5896,6 +5896,14 @@ expand_builtin_assume_aligned (tree exp, rtx target)
> > void
> > expand_builtin_trap (void)
> > {
> > +  if (warn_trap)
> > +    {
> > +      location_t current_location =
> > +    linemap_unwind_to_first_non_reserved_loc (line_table, input_location,
> > +                          NULL);
> > +    warning_at (current_location, OPT_Wtrap, "trap generated");
> > +    }
> > +
> >   if (targetm.have_trap ())
> >     {
> >       rtx_insn *insn = emit_insn (targetm.gen_trap ());
> > diff --git a/gcc/common.opt b/gcc/common.opt
> > index 12b25ff486d..1cc4936ebbf 100644
> > --- a/gcc/common.opt
> > +++ b/gcc/common.opt
> > @@ -812,6 +812,10 @@ Wsystem-headers
> > Common Var(warn_system_headers) Warning
> > Do not suppress warnings from system headers.
> > 
> > +Wtrap
> > +Common Var(warn_trap) Warning
> > +Warn whenever a trap is generated.
> > +
> > Wtrampolines
> > Common Var(warn_trampolines) Warning
> > Warn whenever a trampoline is generated.
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index 575dffd2a2f..68833c4350f 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -8784,6 +8784,14 @@ made up of data only and thus requires no special treatment.  But, for
> > most targets, it is made up of code and thus requires the stack to be
> > made executable in order for the program to work properly.
> > 
> > +@opindex Wtrap
> > +@opindex Wno-trap
> > +@item -Wtrap
> > +Warn when explicit traps are generated.  Traps may be generated for
> > +a variety of reasons, e.g. when using the undefined behavior sanitizer
> > +together with @option{-fsanitize-trap=undefined}.
> > +
> > +
> > @opindex Wfloat-equal
> > @opindex Wno-float-equal
> > @item -Wfloat-equal
> > diff --git a/gcc/testsuite/gcc.dg/Wtrap.c b/gcc/testsuite/gcc.dg/Wtrap.c
> > new file mode 100644
> > index 00000000000..2f2878f9d45
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/Wtrap.c
> > @@ -0,0 +1,8 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -Wtrap" } */
> > +
> > +void foo(void)
> > +{
> > +    __builtin_trap();    /* { dg-warning "trap generated" } */
> > +}
> > +
> >

On Sat, 12 Oct 2024, Martin Uecker wrote:

> Am Samstag, dem 12.10.2024 um 18:44 +0200 schrieb Richard Biener:
> > 
> > > Am 12.10.2024 um 16:43 schrieb Martin Uecker <uecker@tugraz.at>:
> > > 
> > > 
> > > There is code which should not fail at run-time.  For this,
> > > it is helpful to get a warning when a compiler inserts traps
> > > (e.g. sanitizers, hardbools, __builtin_trap(), etc.).
> > > 
> > > Having a warning for this also has many other use cases, e.g.
> > > one can use it with some sanitizer to rule out that some
> > > piece of code has certain undefined behavior such as
> > > signed overflow or undefined behavior in left-shifts
> > > (one gets a warning if the optimizer does not prove the
> > > trap is dead and it is emitted).
> > > 
> > > Another potential use case could be writing tests.
> > > 
> > > 
> > > Bootstrapped and regression tested on x64_84.
> > > 
> > > 
> > >    Add warning option that warns when a trap is generated.
> > > 
> > >    This adds a warning option -Wtrap that is emitted in
> > >    expand_builtin_trap.  It can be used to verify that traps
> > >    are generated or - more importantly - are not generated
> > >    under various conditions, e.g. for UBSan with -fsanitize-trap,
> > >    hardbools, etc.
> > 
> > Isn’t it better to diagnose with more context from the callers that insert the trap?
> 
> More context would be better.  Should there be additional
> arguments when creating the call to the builtin?

Why not diagnose when we create the call?  But sure, adding a diagnostic
argument would work, it might also work to distinguish calls we want to
diagnose from those we don't.

Richard.

Am Sonntag, dem 13.10.2024 um 10:56 +0200 schrieb Richard Biener:
> On Sat, 12 Oct 2024, Martin Uecker wrote:
> 
> > Am Samstag, dem 12.10.2024 um 18:44 +0200 schrieb Richard Biener:
> > > 
> > > > Am 12.10.2024 um 16:43 schrieb Martin Uecker <uecker@tugraz.at>:
> > > > 
> > > > 
> > > > There is code which should not fail at run-time.  For this,
> > > > it is helpful to get a warning when a compiler inserts traps
> > > > (e.g. sanitizers, hardbools, __builtin_trap(), etc.).
> > > > 
> > > > Having a warning for this also has many other use cases, e.g.
> > > > one can use it with some sanitizer to rule out that some
> > > > piece of code has certain undefined behavior such as
> > > > signed overflow or undefined behavior in left-shifts
> > > > (one gets a warning if the optimizer does not prove the
> > > > trap is dead and it is emitted).
> > > > 
> > > > Another potential use case could be writing tests.
> > > > 
> > > > 
> > > > Bootstrapped and regression tested on x64_84.
> > > > 
> > > > 
> > > >    Add warning option that warns when a trap is generated.
> > > > 
> > > >    This adds a warning option -Wtrap that is emitted in
> > > >    expand_builtin_trap.  It can be used to verify that traps
> > > >    are generated or - more importantly - are not generated
> > > >    under various conditions, e.g. for UBSan with -fsanitize-trap,
> > > >    hardbools, etc.
> > > 
> > > Isn’t it better to diagnose with more context from the callers that insert the trap?
> > 
> > More context would be better.  Should there be additional
> > arguments when creating the call to the builtin?
> 
> Why not diagnose when we create the call? 

I agree that having optional warnings for all situation where there
could be run-time UB (or a trap) would be useful.  But having a
generic warning for all such situations would produce many warnings
and also cover cases where we already have more specific warnings.

Doing it when the trap is generated directly gives me somewhat
different information that I sometimes need: Is there a trap left
in the generated binary?

We have a similar warning already for generating trampolines.

Before adding the warning to my local tree, I often looked at the
generated assembly to look for  generated "ud2" instructions.  But this
is painful and gives me even less context.

A practical example from failing to properly take integer
promotions into account (adapted from a old bug in a crypto
library) is:

uint32_t bar(int N, unsigned char key)
{
    unsigned int kappa = key << 24;
    return kappa;
}

which has UB that the warning tells me about and 
where adding a cast is required to eliminate it:
https://godbolt.org/z/osvEsdcqc


>  But sure, adding a diagnostic
> argument would work, it might also work to distinguish calls we want to
> diagnose from those we don't.

Would it be reasonable to approve this patch now and I try
to improve this later?

Martin

On Sun, 13 Oct 2024, Martin Uecker wrote:

> Am Sonntag, dem 13.10.2024 um 10:56 +0200 schrieb Richard Biener:
> > On Sat, 12 Oct 2024, Martin Uecker wrote:
> > 
> > > Am Samstag, dem 12.10.2024 um 18:44 +0200 schrieb Richard Biener:
> > > > 
> > > > > Am 12.10.2024 um 16:43 schrieb Martin Uecker <uecker@tugraz.at>:
> > > > > 
> > > > > 
> > > > > There is code which should not fail at run-time.  For this,
> > > > > it is helpful to get a warning when a compiler inserts traps
> > > > > (e.g. sanitizers, hardbools, __builtin_trap(), etc.).
> > > > > 
> > > > > Having a warning for this also has many other use cases, e.g.
> > > > > one can use it with some sanitizer to rule out that some
> > > > > piece of code has certain undefined behavior such as
> > > > > signed overflow or undefined behavior in left-shifts
> > > > > (one gets a warning if the optimizer does not prove the
> > > > > trap is dead and it is emitted).
> > > > > 
> > > > > Another potential use case could be writing tests.
> > > > > 
> > > > > 
> > > > > Bootstrapped and regression tested on x64_84.
> > > > > 
> > > > > 
> > > > >    Add warning option that warns when a trap is generated.
> > > > > 
> > > > >    This adds a warning option -Wtrap that is emitted in
> > > > >    expand_builtin_trap.  It can be used to verify that traps
> > > > >    are generated or - more importantly - are not generated
> > > > >    under various conditions, e.g. for UBSan with -fsanitize-trap,
> > > > >    hardbools, etc.
> > > > 
> > > > Isn’t it better to diagnose with more context from the callers that insert the trap?
> > > 
> > > More context would be better.  Should there be additional
> > > arguments when creating the call to the builtin?
> > 
> > Why not diagnose when we create the call? 
> 
> I agree that having optional warnings for all situation where there
> could be run-time UB (or a trap) would be useful.  But having a
> generic warning for all such situations would produce many warnings
> and also cover cases where we already have more specific warnings.
> 
> Doing it when the trap is generated directly gives me somewhat
> different information that I sometimes need: Is there a trap left
> in the generated binary?
> 
> We have a similar warning already for generating trampolines.
> 
> Before adding the warning to my local tree, I often looked at the
> generated assembly to look for  generated "ud2" instructions.  But this
> is painful and gives me even less context.
> 
> A practical example from failing to properly take integer
> promotions into account (adapted from a old bug in a crypto
> library) is:
> 
> uint32_t bar(int N, unsigned char key)
> {
>     unsigned int kappa = key << 24;
>     return kappa;
> }
> 
> which has UB that the warning tells me about and 
> where adding a cast is required to eliminate it:
> https://godbolt.org/z/osvEsdcqc
> 
> 
> >  But sure, adding a diagnostic
> > argument would work, it might also work to distinguish calls we want to
> > diagnose from those we don't.
> 
> 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 =
+       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?

Is there any reason to use linemap_unwind_to_first_non_reserved_loc
here?

Thanks,
Richard.

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

Am Dienstag, dem 15.10.2024 um 12:15 +0200 schrieb Jakub Jelinek:
> 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...
> 

It would not be practical with some sanitizers (and sanitizers are also not
the only usecase anyhow). But attached below is the list of emitted warnings
for different sanitizers depending on the optimization level for
BART (https://github.com/mrirecon/bart).  The first number is optimization,
then the number of warnings, and then the number with different warnings
from the same source location (e.g. macros) combined.  The project has
>400 C source files.

For some sanitizers the number is rather low and e.g. shift is something
I was looking at specifically and where I already found this useful.  But
also for  some the others the numbers are low enough to investigate each case. 
For new code, an average number of warnings per file of 10 would also
still seem entirely reasonable, so I think one could eliminate all
integer overflow cases if one wanted to. But also doing this only for
criticial pieces of code (e.g. using #pragma GCC diagnostic ...) would
seem useful.

One also sees that the optimizer actually does remove a lot of traps. That
the total number sometimes goes up with -O2 is mostly likely due to code
duplication, but the unique cases always go down with more optimization.
(except for object-size which does not run fully for -O1 I believe)

I have to admit I did not understand your comment about VRP, but
in my experience we remove UB sanitization based on value ranges, e.g.
in this example for signed overflow.

https://godbolt.org/z/zeMvas3nq

Martin



shift 0 525 31
shift 1 298 19
shift 2 221 19
signed-integer-overflow 0 12212 11350
signed-integer-overflow 1 6310 5913
signed-integer-overflow 2 3803 3172
integer-divide-by-zero 0 332 332
integer-divide-by-zero 1 135 135
integer-divide-by-zero 2 95 95
unreachable 0 0 0
unreachable 1 0 0
unreachable 2 4 4
vla-bound 0 2671 2426
vla-bound 1 1206 1143
vla-bound 2 1024 906
null 0 32041 26739
null 1 8926 7666
null 2 8622 6769
return 0 0 0
return 1 0 0
return 2 4 4
bounds 0 5031 4830
bounds 1 4776 4551
bounds 2 4986 4547
bounds-strict 0 5031 4830
bounds-strict 1 4776 4551
bounds-strict 2 4986 4547
alignment 0 31818 26524
alignment 1 9848 8441
alignment 2 10031 7855
object-size 0 465 453
object-size 1 5928 5491
object-size 2 6013 5366
float-divide-by-zero 0 532 532
float-divide-by-zero 1 430 429
float-divide-by-zero 2 250 245
float-cast-overflow 0 107 98
float-cast-overflow 1 99 91
float-cast-overflow 2 65 59
nonnull-attribute 0 13433 7672
nonnull-attribute 1 5199 3466
nonnull-attribute 2 1085 1005
return-nonnull-attribute 0 0 0
return-nonnull-attribute 1 0 0
return-nonnull-attribute 2 0 0
bool 0 1053 1036
bool 1 988 972
bool 2 371 363
enum 0 0 0
enum 1 0 0
enum 2 4 4
pointer-overflow 0 29095 25298
pointer-overflow 1 17284 16254
pointer-overflow 2 18598 16110
builtin 0 0 0
builtin 1 0 0
builtin 2 4 4