[Bug c/113878] New: missed optimization with sanitizer and signed integer overflow

[Bug c/113878] New: missed optimization with sanitizer and signed integer overflow

[muecker at gwdg dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

            Bug ID: 113878
           Summary: missed optimization with sanitizer and signed integer
                    overflow
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: muecker at gwdg dot de
  Target Milestone: ---

It would be nice if the following example could be optimized better with the
sanitizer. Without sanitizer the function is optimized to return 1. With
sanitizer I would expect it to only add the test for the underflow condition,
but it then fails to optimize well.

int test(int x)
{
    if (x > 100)
        return 1;
    x -= 10;
    if (x > 100)
        return 2;
    return 1;
}

https://godbolt.org/z/j4K96v7jn

[Bug sanitizer/113878] missed optimization with sanitizer and signed integer overflow

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|middle-end                  |sanitizer
                 CC|                            |dodji at gcc dot gnu.org,
                   |                            |dvyukov at gcc dot gnu.org,
                   |                            |jakub at gcc dot gnu.org,
                   |                            |kcc at gcc dot gnu.org

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The undefined sanitizer turns on -fwrapv to prevent optimizations from
optimizing based on undefined behavior and the overflow checks are added after
a few optimizations even.

[Bug sanitizer/113878] missed optimization with sanitizer and signed integer overflow

[uecker at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

uecker at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |uecker at gcc dot gnu.org

--- Comment #2 from uecker at gcc dot gnu.org ---

How does -fwrapv make sense if it is supposed to catch the overflow?

[Bug sanitizer/113878] missed optimization with sanitizer and signed integer overflow

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The sanitizers don't turn on -fwrapv.
There is just TYPE_OVERFLOW_SANITIZED which inhibits various optimizations, in
the constant folder and match.pd etc. so that stuff can be instrumented
properly, doesn't get folded away before the instrumentation is added.
And given the amount of open bugs we have for cases where sanitizers should
catch some UB and don't, I don't think there is energy for enhancements like
this.

[Bug sanitizer/113878] missed optimization with sanitizer and signed integer overflow

[uecker at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

--- Comment #4 from uecker at gcc dot gnu.org ---


Would it make sense to add the instrumentation earlier? Then it could be
optimized as usual which may give better results. 

Just adding a test explicitly shows that this works:
https://godbolt.org/z/av15nheTG

Anyway, I am generally not super convinced about how the sanitizers (bloated
library, unfortunate interaction with warnings, missing cases, poor
optimization...) 

I am somewhat tempted to try putting a light-weight instrumentation directly
into the C FE. That does not seem to hard and would avoid all those problems.
But I am probably missing something....  And I have no idea whether something
like this would be acceptable to GCC.

[Bug sanitizer/113878] missed optimization with sanitizer and signed integer overflow

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Different instrumentations are done at different times.
Some ubsan instrumentations are already done in the FEs (e.g. shifts, division,
...), others are added in the ubsan pass (the idea is that catching up all the
spots where the FE emits e.g. undefined overflow additions etc. is really hard,
unlike the shifts or divisions there are hundreds of spots and we want to catch
even what is added e.g. during gimplification).  Still, the ubsan pass is
fairly early.
asan is far later, ditto tsan.
The warning case wouldn't get better by instrumenting
additions/subtractions/multiplications already in the FEs, it would be exactly
the same.

[Bug sanitizer/113878] missed optimization with sanitizer and signed integer overflow

[uecker at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

--- Comment #6 from uecker at gcc dot gnu.org ---

My idea would be to explicitly add either traps or __builtin_unreachable
whenever there is UB that can be checked for in the C FE, and not add sanitizer
calls (that may return). Just a lightweight tool for safety that needs no
run-time and and be activated in production because it is optimized well.

[Bug sanitizer/113878] missed optimization with sanitizer and signed integer overflow

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to uecker from comment #6)
> My idea would be to explicitly add either traps or __builtin_unreachable
> whenever there is UB that can be checked for in the C FE, and not add
> sanitizer calls (that may return). Just a lightweight tool for safety that
> needs no run-time and and be activated in production because it is optimized
> well.

Something that traps is -fsanitize=undefined -fsanitize-trap=undefined (or
selected sanitizers), that doesn't need any runtime.  And it is still very
costly, it isn't lightweight, and it severely prevents optimizations.
Something that would add conditional __builtin_unreachable would be useless,
that is already implied from the operations.

[Bug sanitizer/113878] missed optimization with sanitizer and signed integer overflow

[uecker at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

--- Comment #8 from uecker at gcc dot gnu.org ---
(In reply to Jakub Jelinek from comment #7)
> (In reply to uecker from comment #6)
> > My idea would be to explicitly add either traps or __builtin_unreachable
> > whenever there is UB that can be checked for in the C FE, and not add
> > sanitizer calls (that may return). Just a lightweight tool for safety that
> > needs no run-time and and be activated in production because it is optimized
> > well.
> 
> Something that traps is -fsanitize=undefined -fsanitize-trap=undefined (or
> selected sanitizers), that doesn't need any runtime.  And it is still very
> costly, it isn't lightweight, and it severely prevents optimizations.
> Something that would add conditional __builtin_unreachable would be useless,
> that is already implied from the operations.

Sure, but -fsanitize=undefined -fsanitize-trap=undefined does optimize much
worse than directly adding the overflow checks (as this and other examples
show) and also sometimes does not have quite the ideal semantics because of
upstream sanitizer library dependency. I wouldn't mind if it could be fixed but
its complexity seems to make it unnecessary hard.

[Bug sanitizer/113878] missed optimization with sanitizer and signed integer overflow

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113878

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
I'd very much appreciate getting rid of TYPE_OVERFLOW_SANITIZED checks by doing
instrumentation in the frontends.

Note we do

#define TYPE_OVERFLOW_UNDEFINED(TYPE)                           \
  (POINTER_TYPE_P (TYPE)                                        \
   ? !flag_wrapv_pointer                                        \
   : (!ANY_INTEGRAL_TYPE_CHECK(TYPE)->base.u.bits.unsigned_flag \
      && !flag_wrapv && !flag_trapv))

it might be tempting to do && !flag_trapv && !(flag_sanitize &
SANITIZE_SI_OVERFLOW) instead to get more complete coverage of disabling
foldings.

_Maybe_ we could clear SANITIZE_SI_OVERFLOW once instrumentation is complete?