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