[Bug rtl-optimization/113551] New: Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[Bug rtl-optimization/113551] New: Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

            Bug ID: 113551
           Summary: Miscompilation with -O1 -funswitch-loops
                    -fno-strict-overflow
           Product: gcc
           Version: 13.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: yshuiv7 at gmail dot com
  Target Milestone: ---

Code:

struct obj {
        int __pad;
        int i;
};

/* aborts when called with NULL */
int assert_not_null(void *); 

void bug(struct obj **root, struct obj *dso) {
        while (1) {
                struct obj *this = *root;

                if (dso == (void *)0)
                        // should return here
                        return;

                if (dso == this)
                        return;

                // shouldn't reach here
                assert_not_null(dso);

                if (!&dso->i)
                        break;
        }
}

// call like this: bug(&obj, NULL);

Result:

* -O1: ok
* -O1 -funswitch-loops: ok
* -O1 -fno-strict-overflow: ok
* -O1 -funswitch-loops -fno-strict-overflow: abort

[Bug rtl-optimization/113551] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

--- Comment #1 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
code is reduced from perf, source file util/dsos.c

[Bug rtl-optimization/113551] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

--- Comment #2 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
regression from 12.3 -> 13.2

[Bug tree-optimization/113551] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|rtl-optimization            |tree-optimization
           Keywords|                            |wrong-code

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Looks like the unswitch is happening when it should not be ...

[Bug tree-optimization/113551] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |5.4.0

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The incorrect unswitch has been happening since at least GCC 5 ...

[Bug tree-optimization/113551] [13/14 Regression] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
            Summary|Miscompilation with -O1     |[13/14 Regression]
                   |-funswitch-loops            |Miscompilation with -O1
                   |-fno-strict-overflow        |-funswitch-loops
                   |                            |-fno-strict-overflow
   Last reconfirmed|                            |2024-01-23
             Status|UNCONFIRMED                 |NEW
      Known to fail|5.4.0                       |13.2.0
   Target Milestone|---                         |13.3

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed at least for the bad unswitch which causes the other wrong code to
happen.

[Bug tree-optimization/113551] [13/14 Regression] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |needs-bisection
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
trunk doesn't unswitch for me (needs bisection).  Let me check what happens on
the branch.

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|rguenth at gcc dot gnu.org         |unassigned at gcc dot gnu.org
             Status|ASSIGNED                    |NEW
           Priority|P3                          |P2
      Known to work|                            |14.0
            Summary|[13/14 Regression]          |[13 Regression]
                   |Miscompilation with -O1     |Miscompilation with -O1
                   |-funswitch-loops            |-funswitch-loops
                   |-fno-strict-overflow        |-fno-strict-overflow
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
The IL after unswitching looks OK, but we assume that when &dso->i is NULL
then dso == NULL and when &dso->i is not NULL then dso also isn't.

I think this is a ranger bug that has been fixed on trunk
but eventually not yet backported, thus we have a duplicate somewhere.

Bisection will tell.

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amacleod at redhat dot com

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Note on trunk we have jump-threaded this to move dso == (void *)0 || dso ==
this out of the loop so there's nothing to unswitch and the bad circumstances
likely do not trigger.

I still remember the ranger bug though.

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

Yuxuan Shui <yshuiv7 at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[13 Regression]             |[13 Regression]
                   |Miscompilation with -O1     |Miscompilation with -O1
                   |-funswitch-loops            |-fno-strict-overflow
                   |-fno-strict-overflow        |

--- Comment #9 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
So, to add some details to people said above, and to make sure I understood
this correctly.

What gcc did here is:

1. move the `if (!&dso->i)` branch out of the loop, duplicate the loop for the
then/else branches.
2. `&dso->i` cannot be 0, so the else branch is eliminated.
3. in the then branch, this condition confused the compiler. it thought since
`&dso->i` is not 0, `dso` is not 0 either, causing the bug.

I diffed `-fdump` outputs and it does match what I expected to see.

(minus is the correct one, plus the incorrect)

@@ -2686,7 +2686,11 @@
 ;; 4 succs { 5 6 }
 ;; 6 succs { 3 }
 ;; 5 succs { 1 }
-Removing basic block 6
+Folding predicate _1 == 0B to 0
+Exported global range table:
+============================
+_1  : [irange] int * [1, +INF]
+Merging blocks 4 and 6

* * *

this made me think whether loop unswitching is actually relevant here. since
12.3 also unswitches this loop, but doesn't miscompile. so I manually
unswitched the loop:


void bug(struct obj **root, struct obj *dso) {
        if (!&dso->i) {
                while (1) {
                        struct obj *this = *root;

                        if (dso == (void *)0)
                                // should return here
                                return;

                        if (dso == this)
                                return;

                        // shouldn't reach here
                        assert_not_null(dso);
                        break;
                }
        } else {
                while (1) {
                        struct obj *this = *root;

                        if (dso == (void *)0)
                                // should return here
                                return;

                        if (dso == this)
                                return;

                        // shouldn't reach here
                        assert_not_null(dso);
                }
        }
}

and now the miscompile happens without -funswitch-loops. I wonder if this
happens on trunk as well.

looks this this is a -fno-strict-overflow related ranger bug.

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

--- Comment #10 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
the manually unswitched version can probably be reduced further.

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

--- Comment #11 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
reduced it a bit:


void bug(struct obj **root, struct obj *dso) {
        if (&dso->i) {
                while (1) {
                        struct obj *this = *root;

                        if (dso == (void *)0)
                                // should return here
                                return;

                        if (dso == this)
                                return;

                        // shouldn't reach here
                        assert_not_null(dso);
                }
        }
}

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

--- Comment #12 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
I think this is the MRE:


void bug(struct obj *dso) {
        if (&dso->i) {
                if (dso == (void *)0)
                        return;

                assert_not_null(dso);
        }
}

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[pinskia at gcc dot gnu.org]

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

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Yuxuan Shui from comment #12)
> I think this is the MRE:
> 
> 
> void bug(struct obj *dso) {
> 	if (&dso->i) {
> 		if (dso == (void *)0)
> 			return;
> 
> 		assert_not_null(dso);
> 	}
> }

Except that is undefined ...
Manually unswitching introduces the undefined behavior in the code.
So even though the code was unswitched before GCC 13 incorrectly, GCC didn't
take into that account before hand.

I am 99% sure what is happening is GCC is see `if (a+1)` and then assuming a
can't be a nullptr. Which is due to undefinedness there of adding 1 to an null
ptr ... (for C that is).

Basically the unswitch is the issue ... Or we maybe we should turn `if (a+1)`
into just `if (a)` ...  Likewise for `if (&a->i)` into `if (a)`

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[mpolacek at gcc dot gnu.org]

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

--- Comment #14 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
I don't see a complete testcase that I could bisect.

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

--- Comment #15 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
(In reply to Marek Polacek from comment #14)
> I don't see a complete testcase that I could bisect.

Please use the code sample in the original comment. since there are questions
that the manually unswitched version is undefined.

link that code with this:

#include<stdlib.h>
struct obj {
        int __pad;
        int i;
};
/* aborts when called with NULL */
int assert_not_null(void *n) {
        if (!n)
                abort();
}
void bug(struct obj **root, struct obj *dso);

int main() {
        struct obj x = {};
        struct obj *y = &x;
        bug(y, NULL);
}

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

--- Comment #16 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
(In reply to Andrew Pinski from comment #13)
> (In reply to Yuxuan Shui from comment #12)
>> ...
> 
> Except that is undefined ...
> Manually unswitching introduces the undefined behavior in the code.
> So even though the code was unswitched before GCC 13 incorrectly, GCC didn't
> take into that account before hand.
> 
> I am 99% sure what is happening is GCC is see `if (a+1)` and then assuming a
> can't be a nullptr. Which is due to undefinedness there of adding 1 to an
> null ptr ... (for C that is).
> 
> Basically the unswitch is the issue ... Or we maybe we should turn `if
> (a+1)` into just `if (a)` ...  Likewise for `if (&a->i)` into `if (a)`

I see. but if it's undefined, why was the `if (dso)` only removed when
-fno-strict-overflow is enabled? and it still happens with
`-fno-delete-null-pointer-checks`

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[pinskia at gcc dot gnu.org]

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

--- Comment #17 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Yuxuan Shui from comment #16)
> I see. but if it's undefined, why was the `if (dso)` only removed when
> -fno-strict-overflow is enabled? and it still happens with
> `-fno-delete-null-pointer-checks`

So -fno-strict-overflow does -fno-wrapv-pointer so we can assume pointer
arithmetic wraps now and then `a+1` could in theory wrap to nullptr.
So many different hooks/options. It is better to use -fwrapv if you only want
signed integer overflow being defined (as wrapping) rather than pointer
arithmetic overflow being defined too.

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -fno-strict-overflow

[yshuiv7 at gmail dot com]

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

--- Comment #18 from Yuxuan Shui <yshuiv7 at gmail dot com> ---
(In reply to Andrew Pinski from comment #17)
> (In reply to Yuxuan Shui from comment #16)
> > ...
> 
> So -fno-strict-overflow does -fno-wrapv-pointer so we can assume pointer
> arithmetic wraps now and then `a+1` could in theory wrap to nullptr.

hmm, but why does that make the null check that previously was not removable,
removable?

also another observation, if I change `struct obj *dso` to `int *dso`, and
`&dso->i` to `&dso[1]`, then the null check will be preserved. despite this
code still being undefined?

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[rguenther at suse dot de]

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

--- Comment #19 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 23 Jan 2024, pinskia at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113551
> 
> --- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
> (In reply to Yuxuan Shui from comment #12)
> > I think this is the MRE:
> > 
> > 
> > void bug(struct obj *dso) {
> > 	if (&dso->i) {
> > 		if (dso == (void *)0)
> > 			return;
> > 
> > 		assert_not_null(dso);
> > 	}
> > }
> 
> Except that is undefined ...
> Manually unswitching introduces the undefined behavior in the code.
> So even though the code was unswitched before GCC 13 incorrectly, GCC didn't
> take into that account before hand.
> 
> I am 99% sure what is happening is GCC is see `if (a+1)` and then assuming a
> can't be a nullptr. Which is due to undefinedness there of adding 1 to an null
> ptr ... (for C that is).
> 
> Basically the unswitch is the issue ... Or we maybe we should turn `if (a+1)`
> into just `if (a)` ...  Likewise for `if (&a->i)` into `if (a)`

It's not undefined on GIMPLE.  As said I believe this was fixed for GCC 14
but appearantly not backported.  bisection should tell

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[mikpelinux at gmail dot com]

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

Mikael Pettersson <mikpelinux at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikpelinux at gmail dot com

--- Comment #20 from Mikael Pettersson <mikpelinux at gmail dot com> ---
For gcc-14 this test case was fixed by 

dc48d1d1d4458773f89f21b2f019f66ddf88f2e5 is the first new commit
commit dc48d1d1d4458773f89f21b2f019f66ddf88f2e5
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Thu Aug 17 11:13:14 2023 -0400

    Fix range-ops operator_addr.

    Lack of symbolic information prevents op1_range from beig able to draw
    the same conclusions as fold_range can.

            PR tree-optimization/111009
            gcc/
            * range-op.cc (operator_addr_expr::op1_range): Be more restrictive.

That commit was backported to gcc-13 in
421311a31a12b96143eb901fde0e020771fe71d4, and that fixed this test case there
too.

Not sure if this is the proper fix or just makes the issue latent.

[Bug tree-optimization/113551] [13 Regression] Miscompilation with -O1 -funswitch-loops -fno-strict-overflow

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
      Known to work|                            |13.2.1

--- Comment #21 from Richard Biener <rguenth at gcc dot gnu.org> ---
yes, it looks suspiciously close to what I'd expect.