[Bug c++/105035] New: regression ICE segmentation fault with -Wduplicated-cond

[Bug c++/105035] New: regression ICE segmentation fault with -Wduplicated-cond

[dimitar.yordanov at sap dot com]

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

            Bug ID: 105035
           Summary: regression ICE segmentation fault with
                    -Wduplicated-cond
           Product: gcc
           Version: 11.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dimitar.yordanov at sap dot com
  Target Milestone: ---

Hi,

we hit an ICE with the following reduced example:

cat > foo.cc << EOF

class a {
  struct b {
    int c;
    int f;
  };
  template <typename> void d();
  b e;
};
template <typename> void a::d() {
  int g;
  if (&g == &e.c)
    ;
  else if (&g == &e.f)
    ;
}

EOF

g++ -Wduplicated-cond -c foo.cc

Best wishes
Dimitar

[Bug c++/105035] [11/12 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-03-23
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Target Milestone|---                         |11.3
            Summary|regression ICE segmentation |[11/12 Regression] tree
                   |fault with                  |check: expected field_decl,
                   |-Wduplicated-cond           |have identifier_node in
                   |                            |operand_equal_p, at
                   |                            |fold-const.c:3335 since
                   |                            |r11-5181-g0862d007b564eca8
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with r11-5181-g0862d007b564eca8.

[Bug c++/105035] [11/12 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at redhat dot com,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
It looks like the C++ frontend builds COMPONENT_REF with IDENTIFIER_NODE
operand one and the middle-end is not expecting this.  Maybe we call
operand_equal_p before layout is complete?

I suppose we could guard operand_equal_p for this case but then handing invalid
GENERIC to the middle-end is suspicious.

Jason?

[Bug c++/105035] [11/12 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |ice-on-valid-code
           Priority|P3                          |P2

[Bug c++/105035] [11/12 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The C++ FE when parsing templates (processing_template_decl is true) produces a
lot of trees the middle-end doesn't like.
If they aren't type or value dependent, usually (but not always) the trees are
such that they can be handed over to the generic code even when parsing
templates.
The -Wduplicated-cond warning implemented in PR64249 seems to do:
            condition = finish_if_stmt_cond (condition, statement);

            if (warn_duplicated_cond)
              warn_duplicated_cond_add_or_warn (token->location, condition,
                                                &chain);
only in the parser regardless of processing_template_decl.
So, the options are either to make sure at least operand_equal_p can cope even
with those trees and apparently operand_equal_p already has such spots in it,
e.g.:
  /* Similar, if either does not have a type (like a template id),
     they aren't equal.  */
  if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1))
    return false;
So, in this particular case, we'd punt if either field0 or field1 is not a
FIELD_DECL (this is in !OP_SAME (1) guarded code).
Another option is to make sure we don't call warn_duplicated_cond_add_or_warn
when processing_template_decl or say when value_dependent_expression_p or
similar, and instead call it during template instantiation in pt.cc after
finish_if_stmt_cond call there.

I think because operand_equal_p already has code to deal with the C++ special
trees and because it is such a low level API, changing it seems to be better.

[Bug c++/105035] [11/12 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[rguenther at suse dot de]

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

--- Comment #4 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 23 Mar 2022, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105035
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org,
>                    |                            |mpolacek at gcc dot gnu.org
> 
> --- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> I think because operand_equal_p already has code to deal with the C++ special
> trees and because it is such a low level API, changing it seems to be better.

Agreed.

[Bug c++/105035] [11/12 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 52671
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52671&action=edit
gcc12-pr105035.patch

Untested fix.

[Bug c++/105035] [11/12 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[ppalka at gcc dot gnu.org]

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

Patrick Palka <ppalka at gcc dot gnu.org> changed:

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

--- Comment #6 from Patrick Palka <ppalka at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #3)
> Another option is to make sure we don't call warn_duplicated_cond_add_or_warn
> when processing_template_decl or say when value_dependent_expression_p or
> similar, and instead call it during template instantiation in pt.cc after
> finish_if_stmt_cond call there.

I noticed the warning already gives up on conditions that have
TREE_SIDE_EFFECTS set, so I suppose it makes sense to do the same for
type-dependent expressions since we can't know if they have side effects until
instantiation time.

And I wonder if warn_duplicated_cond_add_or_warn should be using cp_tree_equal
instead operand_equal_p?  IIUC the latter doesn't handle C++-specific tree
codes.

[Bug c++/105035] [11/12 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[ppalka at gcc dot gnu.org]

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

--- Comment #7 from Patrick Palka <ppalka at gcc dot gnu.org> ---
(In reply to Patrick Palka from comment #6)
> (In reply to Jakub Jelinek from comment #3)
> > Another option is to make sure we don't call warn_duplicated_cond_add_or_warn
> > when processing_template_decl or say when value_dependent_expression_p or
> > similar, and instead call it during template instantiation in pt.cc after
> > finish_if_stmt_cond call there.
> 
> I noticed the warning already gives up on conditions that have
> TREE_SIDE_EFFECTS set, so I suppose it makes sense to do the same for
> type-dependent expressions since we can't know if they have side effects
> until instantiation time.

However, doing the check at instantiation time for dependent conditions might
be problematic since two different dependent conditions could instantiate to
the same non-dependent condition, and I don't think we'd want to issue a
-Wduplicated-cond warning in this case.  For example,

template<class T, class U>
void f() {
  if (T() == 5)
    ...
  else if (U() == 5)
    ...
}

I'm not sure issuing a warning at instantiation time for f<int, int>() would be
appropriate.  We'd might have to compare the dependent conditions even at
instantiation time, provided that the instantiated condition doesn't have
TREE_SIDE_EFFECTS set...  Seems quite messy to get right.

[Bug c++/105035] [11/12 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 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:8698ff67cdff4364c8adad2921ed532359a155ec

commit r12-7797-g8698ff67cdff4364c8adad2921ed532359a155ec
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Mar 24 12:23:51 2022 +0100

    fold-const: Handle C++ dependent COMPONENT_REFs in operand_equal_p
[PR105035]

    As mentioned in the PR, operand_equal_p already contains some hacks so that
    it can be called already on pre-instantiation C++ trees from templates,
    but the recent change to compare DECL_FIELD_OFFSET in the COMPONENT_REF
    case broke this.  Many such COMPONENT_REFs are already punted on earlier
    because they have NULL TREE_TYPE, but in this case the code knows what
    type they have but still uses an IDENTIFIER_NODE as second operand
    of COMPONENT_REF (I think SCOPE_REF is something that could be used too).

    The following patch looks at those DECL_FIELD_*OFFSET fields only if
    both field[01] args are FIELD_DECLs and otherwise keeps it to the
    earlier OP_SAME (1) check that guards this whole block.

    2022-03-24  Jakub Jelinek  <jakub@redhat.com>

            PR c++/105035
            * fold-const.cc (operand_equal_p) <case COMPONENT_REF>: If either
            field0 or field1 is not a FIELD_DECL, return false.

            * g++.dg/warn/Wduplicated-cond2.C: New test.

[Bug c++/105035] [11 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[11/12 Regression] tree     |[11 Regression] tree check:
                   |check: expected field_decl, |expected field_decl, have
                   |have identifier_node in     |identifier_node in
                   |operand_equal_p, at         |operand_equal_p, at
                   |fold-const.c:3335 since     |fold-const.c:3335 since
                   |r11-5181-g0862d007b564eca8  |r11-5181-g0862d007b564eca8

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

[Bug c++/105035] [11 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[cvs-commit at gcc dot gnu.org]

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

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

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

commit r11-9735-gcb2520db3a35758affc7b90b8fefe927fc972516
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Mar 24 12:23:51 2022 +0100

    fold-const: Handle C++ dependent COMPONENT_REFs in operand_equal_p
[PR105035]

    As mentioned in the PR, operand_equal_p already contains some hacks so that
    it can be called already on pre-instantiation C++ trees from templates,
    but the recent change to compare DECL_FIELD_OFFSET in the COMPONENT_REF
    case broke this.  Many such COMPONENT_REFs are already punted on earlier
    because they have NULL TREE_TYPE, but in this case the code knows what
    type they have but still uses an IDENTIFIER_NODE as second operand
    of COMPONENT_REF (I think SCOPE_REF is something that could be used too).

    The following patch looks at those DECL_FIELD_*OFFSET fields only if
    both field[01] args are FIELD_DECLs and otherwise keeps it to the
    earlier OP_SAME (1) check that guards this whole block.

    2022-03-24  Jakub Jelinek  <jakub@redhat.com>

            PR c++/105035
            * fold-const.c (operand_equal_p) <case COMPONENT_REF>: If either
            field0 or field1 is not a FIELD_DECL, return false.

            * g++.dg/warn/Wduplicated-cond2.C: New test.

    (cherry picked from commit 8698ff67cdff4364c8adad2921ed532359a155ec)

[Bug c++/105035] [11 Regression] tree check: expected field_decl, have identifier_node in operand_equal_p, at fold-const.c:3335 since r11-5181-g0862d007b564eca8

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 11.3 too.