[Bug c++/99309] New: Segmentation fault with __builtin_constant_p usage at -O2

[Bug c++/99309] New: Segmentation fault with __builtin_constant_p usage at -O2

[gcc at mailinator dot com]

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

            Bug ID: 99309
           Summary: Segmentation fault with __builtin_constant_p usage at
                    -O2
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gcc at mailinator dot com
  Target Milestone: ---

Compile:

g++ -O2 a.cpp
./a.out


Current output:

1 610334368
Segmentation fault (core dumped)

Code:

#include<iostream>

struct T {
        int val;
        explicit constexpr operator int() const { return val; }
        constexpr T(int64_t v):val(v){}
        void operator*=(T m) {
                //std::cerr<<"start\n";
                if(__builtin_constant_p(m.val)){
                        std::cerr<<"normal\n";
                        return;
                }else if(__builtin_constant_p(val)){
                        std::cerr<<"recursive\n";
                        return;
                }else{
                        std::cerr<<"return\n";
                        return;
                }
        }
};


int main(){
        T constexpr step=610334368;
        T value=1;
        for(int _=0; _<100; ++_){
                std::cerr<<(int)value<<' '<<(int)step<<'\n';
                value*=step;
                std::cerr<<(int)value<<' '<<(int)step<<'\n';
        }
}

[Bug c++/99309] [10/11 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |11.0
   Target Milestone|---                         |10.3
            Summary|Segmentation fault with     |[10/11 Regression]
                   |__builtin_constant_p usage  |Segmentation fault with
                   |at -O2                      |__builtin_constant_p usage
                   |                            |at -O2
      Known to work|                            |9.3.1
           Priority|P3                          |P2
   Last reconfirmed|                            |2021-03-01
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
           Keywords|                            |wrong-code

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
confirmed.

[Bug c++/99309] [10/11 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Testcase without headers:

struct T {
  int val;
  explicit constexpr operator int() const { return val; }
  constexpr T(long v):val(v){}
  void operator*=(T m) {
    if(__builtin_constant_p(m.val)){
      __builtin_printf ("normal\n");
      return;
    }else if(__builtin_constant_p(val)){
      __builtin_printf ("recursive\n");
      return;
    }else{
      __builtin_printf ("return\n");
      return;
    }
  }
};


int main(){
  T constexpr step=610334368;
  T value=1;
  for(int _=0; _<100; ++_){
    __builtin_printf ("%d %d\n", (int) value, (int) step);
    value*=step;
    __builtin_printf ("%d %d\n", (int) value, (int) step);
  }
}

Bet yet another IPA/inlining bug, like e.g. PR98834, the __builtin_unreachable
call is added during inlining:
Introduced new external node (void __builtin_unreachable()/11).

[Bug ipa/99309] [10/11 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c++                         |ipa
                 CC|                            |mpolacek at gcc dot gnu.org

--- Comment #3 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Moving into ipa then.

[Bug ipa/99309] [10/11 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
In particular, we have:
  step.val = 610334368;
and
  _1 = m.val;
  _2 = __builtin_constant_p (_1);
before inline, and inline assumes that this __builtin_constant_p will evaluate
to true.  It will, but only if SRA optimizes it, e.g. fre5 can't do it.
So after inlining we have:
  step.val = 610334368;
...
  m = step;
  _12 = m.val;
  _13 = __builtin_constant_p (_12);
and if _13 is 0, there is __builtin_unreachable (); added by the inliner.
where the latter chunk is in a loop, but nothing ever modifies step.
Now, with -O1 it works fine, the __builtin_constant_p is kept until sra pass
which optimizes that to
  step$val_2 = 610334368;
...
  m$val_1 = step$val_2;
  _12 = m$val_1;
  _13 = __builtin_constant_p (_12);
and dom2 afterwards optimizes that.
But, with -O2, vrp1 comes before sra and folds __builtin_constant_p to 0
because it still is not a constant.

One bug is that IPA/inliner should not make assumptions what the following
optimizations will do, it can make some optimistic assumptions during cost
computation, but it should be prepared that the optimizations it relies on can
be disabled, or just not doing its job for whatever other reason.  Unless IPA
wants to do that optimization by itself, i.e. propagate the constants there. 
But it really shouldn't do that just for __builtin_constant_p that guards
something, but also for any other uses of the same reference, otherwise code
guarded with __builtin_constant_p check could resolve that check to 1, but the
uses of the argument elsewhere in the expressions actually would be
non-constant.

Another is that perhaps FRE/sccvn should be able to look the memory.
I believe sccvn has code to handle
  step.val = 610334368;
...
  _12 = step.val;
already, so it is just the extra m = step; copy in between that probably
prevents it from working.  Could sccvn remember for the aggregate copies that
instead of looking at vara for value look at varb for value (with checks for
invalidation), or just copy over all individual pieces on the aggrecate copy?

[Bug ipa/99309] [10/11 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[hubicka at gcc dot gnu.org]

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

--- Comment #5 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
As discussed, I can prepare patch to make inliner to redirect
__builtin_constant_p to __builtin_true whenever inliner detect that the
expression is compile time ocnstant.  This will avoid us eventually hitting
unreachable when late optimizations forget to make the transformation.

I was worried about this idea since this will still lead to some inconsistency
since uses guarded by the __builtin_constnat_p may or may not be constant
propagated and it seems logical to assume that in the block guarded by
builtin_constnat_p the expression will indeed evaluate to compile time
constant.

However we can get similar inconsistencies with alias oracle walking limits as
well, so these constructions are generally fragile (but seems increasingly
common in C++ codebases).

It would be still nice to have fre5 to constant propagate this. IPA analysis
are very simplistics.
Richi, any idea on this?

[Bug ipa/99309] [10/11 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #5)
> As discussed, I can prepare patch to make inliner to redirect
> __builtin_constant_p to __builtin_true whenever inliner detect that the
> expression is compile time ocnstant.  This will avoid us eventually hitting
> unreachable when late optimizations forget to make the transformation.

I'm quite worried about that, the point of guarding something with
__builtin_constant_p is about the hope that the argument of that builtin will
evaluate to constant in the conditional block guarded by it.
By folding __builtin_constant_p to true if it sees it as constant but not
actually propagating that constant to all uses of that expression we could have
the condition folded to true, but if SRA or FRE etc. is disabled or isn't able
to optimize it, we wouldn't have it constant.

[Bug ipa/99309] [10/11 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[hubicka at ucw dot cz]

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

--- Comment #7 from Jan Hubicka <hubicka at ucw dot cz> ---
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99309
> 
> --- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> (In reply to Jan Hubicka from comment #5)
> > As discussed, I can prepare patch to make inliner to redirect
> > __builtin_constant_p to __builtin_true whenever inliner detect that the
> > expression is compile time ocnstant.  This will avoid us eventually hitting
> > unreachable when late optimizations forget to make the transformation.
> 
> I'm quite worried about that, the point of guarding something with
> __builtin_constant_p is about the hope that the argument of that builtin will
> evaluate to constant in the conditional block guarded by it.
> By folding __builtin_constant_p to true if it sees it as constant but not
> actually propagating that constant to all uses of that expression we could have
> the condition folded to true, but if SRA or FRE etc. is disabled or isn't able
> to optimize it, we wouldn't have it constant.
Yes, it is not good (this is why I did not implement it earlier).
However you can trigger same problem without inline.  Just put enough
statements between the conditional and use so AO walk oracles gives
up...

Honza

[Bug ipa/99309] [10/11 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.3                        |10.4

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10.3 is being released, retargeting bugs to GCC 10.4.

[Bug ipa/99309] [10/11 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[hubicka at gcc dot gnu.org]

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

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |hubicka at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #9 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Have WIP patch to attach predicates to buildtin_constant_p and redirect to true
if inliner works out that it is a constat (still relying on late passes to
optimize the if branch well).

>From all the options I can think of this seems best even though it may end up
in relatively rare cases that we do the (very simple) propagation at IPA time
and late optimizations won't.  Without explicitly disabling passes (where I
think this is fine to happen) all testcases we seen so far was of the form that
constant was eventually propagated but only after we folded builtin_constant_p
to false.

Overall it is not possible to assure that builtin_constant_p on memory will
fold to true only if all uses of the memory later in the if branch will ford to
constant since AO has walking limits.

[Bug ipa/99309] [10/11/12 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[rguenth at gcc dot gnu.org]

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

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
The issue for FRE is that we have

  <bb 2> [local count: 10737416]:
  step.val = 610334368;
  value.val = 1;
  goto <bb 6>; [100.00%]

  <bb 6> [local count: 1073741824]:
  # __1 = PHI <0(2), __11(4)>
  if (__1 != 100)
    goto <bb 3>; [99.00%]
  else
    goto <bb 7>; [1.00%]

  <bb 3> [local count: 1063004409]:
  __builtin_printf ("%d %d\n", 1, 610334368);
  m = step;
  _12 = m.val;
  _13 = __builtin_constant_p (_12);
  if (_13 != 0)
    goto <bb 4>; [100.00%]
  else
    goto <bb 5>; [0.00%]

  <bb 4> [local count: 1063004409]:
  __builtin_puts (&"normal"[0]);
  m ={v} {CLOBBER(eol)};
  __builtin_printf ("%d %d\n", 1, 610334368);
  __11 = __1 + 1;
  goto <bb 6>; [100.00%]

  <bb 5> [count: 0]:
  __builtin_unreachable ();

and while the first FRE iteration correctly determines that m.val is 610334368
we then make the backedge of the loop executable and because we translated
the lookup expression from m.val to step.val we are refusing to handle loops
(the abort_on_visited argument to get_continuation_for_phi, set from
translated in walk_non_aliased_vuses).

I don't remember why I disregarded the fix in comment#3 of PR54498, a fix
along that line would enable CSE here.  The concern was probably
compile-time (but we limit the amount of alias queries done).

It's a bit late to do such change for GCC 12 I think.  I will queue this
for GCC 13.

[Bug ipa/99309] [10/11/12/13 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug ipa/99309] [11/12/13/14 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.

[Bug ipa/99309] [11/12/13/14 Regression] Segmentation fault with __builtin_constant_p usage at -O2

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=111036
                 CC|                            |bmei at broadcom dot com

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 111036 has been marked as a duplicate of this bug. ***