[Bug c++/103991] New: Bogus -Wreturn-type with constexpr if and local var with destructor

[Bug c++/103991] New: Bogus -Wreturn-type with constexpr if and local var with destructor

[sbergman at redhat dot com]

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

            Bug ID: 103991
           Summary: Bogus -Wreturn-type with constexpr if and local var
                    with destructor
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: sbergman at redhat dot com
  Target Milestone: ---

Apparently a recent regression on GCC trunk:

> $ cat test.cc
> struct S { ~S(); };
> int f() {
>     S s;
>     if constexpr (true) return 0;
>     else return 0;
> }

> $ g++ -c test.cc
> test.cc: In function ‘int f()’:
> test.cc:6:1: warning: control reaches end of non-void function [-Wreturn-type]
>     6 | }
>       | ^

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Target Milestone|---                         |12.0
             Status|UNCONFIRMED                 |NEW
            Summary|Bogus -Wreturn-type with    |[12 Regression] Bogus
                   |constexpr if and local var  |-Wreturn-type with
                   |with destructor             |constexpr if and local var
                   |                            |with destructor
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |jason at gcc dot gnu.org
           Priority|P3                          |P1
   Last reconfirmed|                            |2022-01-12

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r12-5638-ga3e75c1491cd2d501081210925a89a65b1c1e5e5

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Slightly bigger testcase that shows that also consteval if is affected:
struct S { ~S(); };
int
foo ()
{
  S s;
  if constexpr (true)
    return 0;
  else
    return 1;
}

#if __cpp_if_consteval >= 202106L
int
bar ()
{
  S s;
  if consteval
    {
      return 0;
    }
  else
    {
      return 1;
    }
}
#endif

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Maybe better:
struct S { ~S(); };
int
foo ()
{
  S s;
  if constexpr (true)
    return 0;
  else
    return 1;
}

#if __cpp_if_consteval >= 202106L
constexpr int
bar ()
{
  S s;
  if consteval
    {
      return 0;
    }
  else
    {
      return 1;
    }
}

int
baz ()
{
  return bar ();
}
#endif

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For IF_STMT_CONSTEXPR_P and IF_STMT_CONSTEVAL_P IF_STMTs, it is unclear what we
should do, because in either case we throw away the other branch if any.
Either we do for those what we used to do before r12-5638 and risk
-Wunreachable-code warnings (when/if it is readded), e.g. on code like:
  if constexpr (true)
    return 0;
  some code;
but we don't emit these -Wreturn-type false positives in cases where the
untaken block of code doesn't fall through.
Or the r12-5638 can result in such false positives.
Or perhaps we should track if we had the other block of code at all (if not, it
is ok to do what we do right now) and if possible otherwise try to figure out
if the other block could fall through and if it can't, perhaps replace the
void_node with __builtin_unreachable () call?
For IF_STMT_CONSTEVAL_P we still have the other branch around and could perhaps
call block_may_fallthru on it, but for IF_STMT_CONSTEXPR_P we discard it
earlier,
outside of templates already during parsing.

Now, as Richi's warning isn't in GCC 12, quickest/safest temporary fix would be
to revert to previous behavior for IF_STMT_CONSTEXPR_P and IF_STMT_CONSTEVAL_P,
  if (IF_STMT_CONSTEVAL_P (stmt))
    stmt = else_;
  else if (IF_STMT_CONSTEXPR_P (stmt))
    stmt = integer_nonzerop (cond) ? then_ ? else_;
  else
    stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_);

Jason, thoughts on this?

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For the more permanent solution, perhaps making the other branch
__builtin_unreachable() if it doesn't fall through is too expensive and we
should just arrange for the pre- r12-5638 behavior where we in GENERIC pretend
there wasn't an if at all.

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For consteval if I guess we could do:
--- gcc/cp/cp-objcp-common.c.jj 2022-01-11 23:11:22.091294356 +0100
+++ gcc/cp/cp-objcp-common.c    2022-01-12 17:57:18.232202275 +0100
@@ -313,6 +313,13 @@ cxx_block_may_fallthru (const_tree stmt)
       return false;

     case IF_STMT:
+      if (IF_STMT_CONSTEXPR_P (stmt))
+       {
+         if (integer_nonzerop (IF_COND (stmt)))
+           return block_may_fallthru (THEN_CLAUSE (stmt));
+         if (integer_zerop (IF_COND (stmt)))
+           return block_may_fallthru (ELSE_CLAUSE (stmt));
+       }
       if (block_may_fallthru (THEN_CLAUSE (stmt)))
        return true;
       return block_may_fallthru (ELSE_CLAUSE (stmt));
--- gcc/cp/cp-gimplify.c.jj     2022-01-11 23:11:22.090294370 +0100
+++ gcc/cp/cp-gimplify.c        2022-01-12 17:53:57.431036236 +0100
@@ -166,8 +166,13 @@ genericize_if_stmt (tree *stmt_p)
      can contain unfolded immediate function calls, we have to discard
      the then_ block regardless of whether else_ has side-effects or not.  */
   if (IF_STMT_CONSTEVAL_P (stmt))
-    stmt = build3 (COND_EXPR, void_type_node, boolean_false_node,
-                  void_node, else_);
+    {
+      if (block_may_fallthru (then_))
+       stmt = build3 (COND_EXPR, void_type_node, boolean_false_node,
+                      void_node, else_);
+      else
+       stmt = else_;
+    }
   else
     stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_);
   protected_set_expr_location_if_unset (stmt, locus);

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[rguenther at suse dot de]

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

--- Comment #7 from rguenther at suse dot de <rguenther at suse dot de> ---
> Am 12.01.2022 um 17:33 schrieb jakub at gcc dot gnu.org <gcc-bugzilla@gcc.gnu.org>:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103991
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                 CC|                            |rguenth at gcc dot gnu.org
> 
> --- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> For IF_STMT_CONSTEXPR_P and IF_STMT_CONSTEVAL_P IF_STMTs, it is unclear what we
> should do, because in either case we throw away the other branch if any.
> Either we do for those what we used to do before r12-5638 and risk
> -Wunreachable-code warnings (when/if it is readded), e.g. on code like:
>  if constexpr (true)
>    return 0;
>  some code;
> but we don't emit these -Wreturn-type false positives in cases where the
> untaken block of code doesn't fall through.
> Or the r12-5638 can result in such false positives.
> Or perhaps we should track if we had the other block of code at all (if not, it
> is ok to do what we do right now) and if possible otherwise try to figure out
> if the other block could fall through and if it can't, perhaps replace the
> void_node with __builtin_unreachable () call?
> For IF_STMT_CONSTEVAL_P we still have the other branch around and could perhaps
> call block_may_fallthru on it, but for IF_STMT_CONSTEXPR_P we discard it
> earlier,
> outside of templates already during parsing.
> 
> Now, as Richi's warning isn't in GCC 12, quickest/safest temporary fix would be
> to revert to previous behavior for IF_STMT_CONSTEXPR_P and IF_STMT_CONSTEVAL_P,
>  if (IF_STMT_CONSTEVAL_P (stmt))
>    stmt = else_;
>  else if (IF_STMT_CONSTEXPR_P (stmt))
>    stmt = integer_nonzerop (cond) ? then_ ? else_;
>  else
>    stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_);

I agree that reverting for GCC 12 is the most reasonable thing with adding a
Testcase

> Jason, thoughts on this?
> 
> -- 
> You are receiving this mail because:
> You are on the CC list for the bug.

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #7)
> > Now, as Richi's warning isn't in GCC 12, quickest/safest temporary fix would be
> > to revert to previous behavior for IF_STMT_CONSTEXPR_P and IF_STMT_CONSTEVAL_P,
> >  if (IF_STMT_CONSTEVAL_P (stmt))
> >    stmt = else_;
> >  else if (IF_STMT_CONSTEXPR_P (stmt))
> >    stmt = integer_nonzerop (cond) ? then_ ? else_;
> >  else
> >    stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_);
> 
> I agree that reverting for GCC 12 is the most reasonable thing with adding a
> Testcase

The above isn't a full reversion, it keeps the normal if (which isn't
problematic) as is and just reverts behavior for the special const{expr,eval}
ifs.

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:cbf06187d5f246634272e3d2892501563bff3d99

commit r12-6579-gcbf06187d5f246634272e3d2892501563bff3d99
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Jan 14 12:09:19 2022 +0100

    c++: Avoid some -Wreturn-type false positives with const{expr,eval} if
[PR103991]

    The changes done to genericize_if_stmt in order to improve
    -Wunreachable-code* warning (which Richi didn't actually commit
    for GCC 12) are I think fine for normal ifs, but for constexpr if
    and consteval if we have two competing warnings.
    The problem is that we replace the non-taken clause (then or else)
    with void_node and keep the if (cond) { something } else {}
    or if (cond) {} else { something }; in the IL.
    This helps -Wunreachable-code*, if something can't fallthru but the
    non-taken clause can, we don't warn about code after it because it
    is still (in theory) reachable.
    But if the non-taken branch can't fallthru, we can get false positive
    -Wreturn-type warnings (which are enabled by default) if there is
    nothing after the if and the taken branch can't fallthru either.

    One possibility to fix this is revert at least temporarily
    to the previous behavior for constexpr and consteval if, yes, we
    can get false positive -Wunreachable-code* warnings but the warning
    isn't present in GCC 12.
    The patch below implements that for constexpr if which throws its
    clauses very early (either during parsing or during instantiation),
    and for consteval if it decides based on block_may_fallthru on the
    non-taken (for constant evaluation only) clause - if the non-taken
    branch may fallthru, it does what you did in genericize_if_stmt
    for consteval if, if it can't fallthru, it uses the older way
    of pretending there wasn't an if and just replacing it with the
    taken clause.  There are some false positive risks with this though,
    block_may_fallthru is optimistic and doesn't handle some statements
    at all (like FOR_STMT, WHILE_STMT, DO_STMT - of course handling those
    is quite hard).
    For constexpr if (but perhaps for GCC 13?) we could try to
    block_may_fallthru before we throw it away and remember it in some
    flag on the IF_STMT, but am not sure how dangerous would it be to call
    it on the discarded stmts.  Or if it is too dangerous e.g. just
    remember whether the discarded block of consteval if wasn't present
    or was empty, in that case assume fallthru, and otherwise assume
    it can't fallthru (-Wunreachable-code possible false positives).

    2022-01-14  Jakub Jelinek  <jakub@redhat.com>

            PR c++/103991
            * cp-objcp-common.c (cxx_block_may_fallthru) <case IF_STMT>: For
            IF_STMT_CONSTEXPR_P with constant false or true condition only
            check if the taken clause may fall through.
            * cp-gimplify.c (genericize_if_stmt): For consteval if, revert
            to r12-5638^ behavior if then_ block can't fall through.  For
            constexpr if, revert to r12-5638^ behavior.

            * g++.dg/warn/Wreturn-type-13.C: New test.

[Bug c++/103991] [12 Regression] Bogus -Wreturn-type with constexpr if and local var with destructor

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.