Straw poll on shifts with out of range operands

Straw poll on shifts with out of range operands

[Andrew Pinski]

I am in the middle of improving the isolation path pass for shifts
with out of range operands.
There are 3 options we could do really:
1) isolate the path to __builtin_unreachable
2) isolate the path to __builtin_trap
    This is what is currently done for null pointer and divide by zero
3) isolate the path and turn the shift into zero constant
   This happens currently for explicit use in both match (in many
cases) and VRP for others.

All 3 are not hard to implement.
This comes up in the context of https://gcc.gnu.org/PR115636 where the
original requestor thinks it should be #3 but I suspect they didn't
realize it was undefined behavior then.
2 seems the best for user experience.
1 seems like the best for optimizations.
3 is more in line with how other parts of the compiler handle it.

So the question I have is which one should we go for? (is there
another option I missed besides not do anything)

Thanks,
Andrew Pinski

Re: Straw poll on shifts with out of range operands

[Jeff Law]

On 6/25/24 8:44 PM, Andrew Pinski via Gcc wrote:
> I am in the middle of improving the isolation path pass for shifts
> with out of range operands.
> There are 3 options we could do really:
> 1) isolate the path to __builtin_unreachable
> 2) isolate the path to __builtin_trap
>      This is what is currently done for null pointer and divide by zero
> 3) isolate the path and turn the shift into zero constant
>     This happens currently for explicit use in both match (in many
> cases) and VRP for others.
> 
> All 3 are not hard to implement.
> This comes up in the context of https://gcc.gnu.org/PR115636 where the
> original requestor thinks it should be #3 but I suspect they didn't
> realize it was undefined behavior then.
> 2 seems the best for user experience.
> 1 seems like the best for optimizations.
> 3 is more in line with how other parts of the compiler handle it.
> 
> So the question I have is which one should we go for? (is there
> another option I missed besides not do anything)
There was a time when we were thinking about having a knob that would 
allow one to select which of the 3 approaches makes the most sense given 
the expected execution environment.

While I prefer #2, some have (reasonably) argued that it's not 
appropriate behavior for code in a library.

I'm not a fan of #1 because it allows unpredictable code execution. 
Essentially you just fall into whatever code was after the bogus shift 
in the executable and hope for the best.  One day this is going to bite 
us hard from a security standpoint.

#3 isn't great, but it's not terrible either.

Jeff

Re: Straw poll on shifts with out of range operands

[Richard Biener]

On Wed, Jun 26, 2024 at 4:59 AM Jeff Law via Gcc <gcc@gcc.gnu.org> wrote:
>
>
>
> On 6/25/24 8:44 PM, Andrew Pinski via Gcc wrote:
> > I am in the middle of improving the isolation path pass for shifts
> > with out of range operands.
> > There are 3 options we could do really:
> > 1) isolate the path to __builtin_unreachable
> > 2) isolate the path to __builtin_trap
> >      This is what is currently done for null pointer and divide by zero
> > 3) isolate the path and turn the shift into zero constant
> >     This happens currently for explicit use in both match (in many
> > cases) and VRP for others.

How is isolation if we do 3) any useful?  IIRC the path isolation pass
would only look for literal out-of-range shifts?  Or do you plan to use
range info?  If we do 3) why not let range folding deal with this then?

> >
> > All 3 are not hard to implement.
> > This comes up in the context of https://gcc.gnu.org/PR115636 where the
> > original requestor thinks it should be #3 but I suspect they didn't
> > realize it was undefined behavior then.
> > 2 seems the best for user experience.
> > 1 seems like the best for optimizations.
> > 3 is more in line with how other parts of the compiler handle it.
> >
> > So the question I have is which one should we go for? (is there
> > another option I missed besides not do anything)
> There was a time when we were thinking about having a knob that would
> allow one to select which of the 3 approaches makes the most sense given
> the expected execution environment.

Since we do 3) already elsewhere I'd say we should do that by default and
have the other options behind a command-line switch - though we already
have UBSAN for that and that is going to be much more reliable than the late
path isolation done after folding catched most cases via 3)?

> While I prefer #2, some have (reasonably) argued that it's not
> appropriate behavior for code in a library.
>
> I'm not a fan of #1 because it allows unpredictable code execution.
> Essentially you just fall into whatever code was after the bogus shift
> in the executable and hope for the best.  One day this is going to bite
> us hard from a security standpoint.
>
> #3 isn't great, but it's not terrible either.
>
> Jeff