From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 1C2803858D1E for ; Mon, 30 Jan 2023 16:00:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1C2803858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675094444; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ZHxTZKJ1m3zQRq+AiHqlR8A/I42tSBfnUvYcZXrLpFc=; b=CmolpP5CaDV4us7PwjrobyRC1b0Lkz+uIo5T3QARQLnqXqG5HgSMIuzA3mhy3Mz36/sePp H7r+BzGDmxZnixfUWQGcVBXAOsvoaX4eeIoJU43Om+CHX8+6DPnUHysZErayv2MyCGgGX2 ZKBSq+cxIZmjT3KXlk8YHUcNi4KJ7vg= Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-202-qxIDacx4P4mqn5SNm-U3LQ-1; Mon, 30 Jan 2023 11:00:43 -0500 X-MC-Unique: qxIDacx4P4mqn5SNm-U3LQ-1 Received: by mail-qv1-f71.google.com with SMTP id jo26-20020a056214501a00b0053aa15f61d4so2838242qvb.7 for ; Mon, 30 Jan 2023 08:00:43 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ZHxTZKJ1m3zQRq+AiHqlR8A/I42tSBfnUvYcZXrLpFc=; b=UvRTNoJz/z3WUWQDhV+cIBWxp8tcs/5ERrv1AW3xFQK5srof0m8qC1QkNt6L1oYVGp lkIQCczQL5o60dpfWPbr1XEgOl44fS7/HxQ8mnC5v5HTzbtT4PVveQLDhuMe/0UZe4Kg FrcqKvwYbmncpVZC+gRDXiARhxWzH9TKlZ+0VxyhaUSOjgN1hcMOhIxr+nUvFNxmuJG/ XS37Y3FyZCnraLsb8waJMHn8UAB0Zba9IbZHYvIu/cnvkbpDsIAJAt/h/pNDWyoehBfF FvdQQKH0NDECtBU0swwWqToDFO9Rlp5t9pq7+eHjyA6WVHMTjKH/lQJViILjHW/k8g2e HSOQ== X-Gm-Message-State: AO0yUKW+XYrJWzRi5q91Y18QThJptUQNWcibQ2FiGun5ISMpRAFB2EvH CcADTMv51YRcYaMXP/GKBB6c7j5ImUdW5u48YCS9EKalP7bIq6kupy0riN/RxPWvv5HgVQwM5SA gMqmZd+X7nLMM+a+bLw== X-Received: by 2002:ac8:7fc6:0:b0:3b8:5b81:a437 with SMTP id b6-20020ac87fc6000000b003b85b81a437mr10903432qtk.24.1675094440771; Mon, 30 Jan 2023 08:00:40 -0800 (PST) X-Google-Smtp-Source: AK7set9F5Py+H29fd7JgvPW8chVaxrs6oJx63gxNpyW5cJ/0pzZMCFYeP06oDeMfEO4Q2KKk/ssGPw== X-Received: by 2002:ac8:7fc6:0:b0:3b8:5b81:a437 with SMTP id b6-20020ac87fc6000000b003b85b81a437mr10903329qtk.24.1675094440293; Mon, 30 Jan 2023 08:00:40 -0800 (PST) Received: from redhat.com (2603-7000-9500-34a5-0000-0000-0000-1db4.res6.spectrum.com. [2603:7000:9500:34a5::1db4]) by smtp.gmail.com with ESMTPSA id o8-20020ac80248000000b003b64f1b1f40sm8230385qtg.40.2023.01.30.08.00.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Jan 2023 08:00:39 -0800 (PST) Date: Mon, 30 Jan 2023 11:00:38 -0500 From: Marek Polacek To: Patrick Palka Cc: Jason Merrill , GCC Patches Subject: [PATCH v2] c++: fix ICE with -Wduplicated-cond [PR107593] Message-ID: References: <20230126221732.617749-1-polacek@redhat.com> <6cd96b09-828b-8820-e1f7-7f11a90e0f54@idea> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/2.2.9 (2022-11-12) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, Jan 27, 2023 at 06:17:00PM -0500, Patrick Palka wrote: > On Fri, 27 Jan 2023, Marek Polacek wrote: > > > On Fri, Jan 27, 2023 at 05:15:00PM -0500, Patrick Palka wrote: > > > On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: > > > > > > > Here we crash because a CAST_EXPR, representing T(), doesn't have > > > > its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't > > > > expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) > > > > > > > > In the past we've adjusted o_e_p to better cope with template codes, > > > > but in this case I think we just want to avoid attempting to warn > > > > about inst-dependent expressions; I don't think I've ever envisioned > > > > -Wduplicated-cond to warn about them. > > > > > > > > The ICE started with r12-6022, two-stage name lookup for overloaded > > > > operators, which gave dependent operators a TREE_TYPE (in particular, > > > > DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: > > > > > > > > /* 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; > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > > > PR c++/107593 > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > * parser.cc (cp_parser_selection_statement): Don't do > > > > -Wduplicated-cond when the condition is dependent. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * g++.dg/warn/Wduplicated-cond3.C: New test. > > > > --- > > > > gcc/cp/parser.cc | 3 +- > > > > gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ > > > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > > > > > > > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > > > > index 4cdc1cd472f..3df85d49e16 100644 > > > > --- a/gcc/cp/parser.cc > > > > +++ b/gcc/cp/parser.cc > > > > @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, > > > > /* Add the condition. */ > > > > condition = finish_if_stmt_cond (condition, statement); > > > > > > > > - if (warn_duplicated_cond) > > > > + if (warn_duplicated_cond > > > > + && !instantiation_dependent_expression_p (condition)) > > > > warn_duplicated_cond_add_or_warn (token->location, condition, > > > > &chain); > > > > > > I noticed warn_duplicated_cond_add_or_warn already has logic to handle > > > TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder > > > if we'd want to do the same for instantiation-dep conditions? > > > > warn_duplicated_cond_add_or_warn lives in c-family/c-warn.cc so I can't > > use instantiation_dependent_expression_p there. Sure, I could write a > > C++ wrapper but with my patch we just won't add CONDITION to the chain > > which I thought would work just as well. > > Ah that's unfortunate :( ISTM desirable to conservatively assume an > inst-dep cond has side effects though (possibly directly from > cp_parser_selection_statement), to avoid false positives as in: > > int n; > > template bool g() { n = 42; } > > template > void f() { > if (n) > ; > else if (g()) > ; > else if (n) > ; > } You're right, we shouldn't warn there. So I've just added a new param with a default argument. A new PR was just opened for the same problem so I've added another test. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Here we crash because a CAST_EXPR, representing T(), doesn't have its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) In the past we've adjusted o_e_p to better cope with template codes, but in this case I think we just want to avoid attempting to warn about inst-dependent expressions; I don't think I've ever envisioned -Wduplicated-cond to warn about them. Also destroy the chain when an inst-dependent expression is encountered to not warn in Wduplicated-cond4.C. The ICE started with r12-6022, two-stage name lookup for overloaded operators, which gave dependent operators a TREE_TYPE (in particular, DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: /* 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; PR c++/107593 PR c++/108597 gcc/c-family/ChangeLog: * c-common.h (warn_duplicated_cond_add_or_warn): New parameter. * c-warn.cc (warn_duplicated_cond_add_or_warn): Add new parameter. Use it. gcc/cp/ChangeLog: * parser.cc (cp_parser_selection_statement): Pass the result of instantiation_dependent_expression_p down to warn_duplicated_cond_add_or_warn. gcc/testsuite/ChangeLog: * g++.dg/warn/Wduplicated-cond3.C: New test. * g++.dg/warn/Wduplicated-cond4.C: New test. * g++.dg/warn/Wduplicated-cond5.C: New test. --- gcc/c-family/c-common.h | 3 +- gcc/c-family/c-warn.cc | 5 ++- gcc/cp/parser.cc | 8 +++- gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C | 17 +++++++++ gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C | 16 ++++++++ 6 files changed, 82 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index bb6271d4a83..a9b47e15894 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1514,7 +1514,8 @@ extern void maybe_record_typedef_use (tree); extern void maybe_warn_unused_local_typedefs (void); extern void maybe_warn_bool_compare (location_t, enum tree_code, tree, tree); extern bool maybe_warn_shift_overflow (location_t, tree, tree); -extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec **); +extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec **, + bool = false); extern bool diagnose_mismatched_attributes (tree, tree); extern tree do_warn_duplicated_branches_r (tree *, int *, void *); extern void warn_for_multistatement_macros (location_t, location_t, diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc index 5ed7bcab16a..b7621656f89 100644 --- a/gcc/c-family/c-warn.cc +++ b/gcc/c-family/c-warn.cc @@ -2529,13 +2529,14 @@ maybe_warn_unused_local_typedefs (void) of COND. */ void -warn_duplicated_cond_add_or_warn (location_t loc, tree cond, vec **chain) +warn_duplicated_cond_add_or_warn (location_t loc, tree cond, vec **chain, + bool dependent_p /*=false*/) { /* No chain has been created yet. Do nothing. */ if (*chain == NULL) return; - if (TREE_SIDE_EFFECTS (cond)) + if (dependent_p || TREE_SIDE_EFFECTS (cond)) { /* Uh-oh! This condition has a side-effect, thus invalidates the whole chain. */ diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc index 4cdc1cd472f..d62db229518 100644 --- a/gcc/cp/parser.cc +++ b/gcc/cp/parser.cc @@ -13210,8 +13210,12 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, condition = finish_if_stmt_cond (condition, statement); if (warn_duplicated_cond) - warn_duplicated_cond_add_or_warn (token->location, condition, - &chain); + { + const bool dep + = instantiation_dependent_expression_p (condition); + warn_duplicated_cond_add_or_warn (token->location, condition, + &chain, dep); + } /* Parse the then-clause. */ in_statement = parser->in_statement; diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C new file mode 100644 index 00000000000..3da054e5485 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C @@ -0,0 +1,38 @@ +// PR c++/107593 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +template +void +foo () +{ + if (T() && T() && int()) + ; + else if (T() && T() && int()) + ; +} + +template +void bar(T a) +{ + if (a) + ; + else if (a) + ; +} + +template +void baz(int a) +{ + if (a) + ; + else if (a) // { dg-warning "duplicated" } + ; +} +void +f () +{ + foo(); + bar(1); + baz(1); +} diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C new file mode 100644 index 00000000000..41bb9f09b4f --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C @@ -0,0 +1,17 @@ +// PR c++/107593 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +int n; + +template bool g() { n = 42; return false; } + +template +void f() { + if (n) + ; + else if (g()) + ; + else if (n) + ; +} diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C new file mode 100644 index 00000000000..23a0bf212b5 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C @@ -0,0 +1,16 @@ +// PR c++/108597 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +template +struct MyStruct { + + void check(int &x) { + if (&x == &_a) { + } else if (&x == &_b) { + } + } + + int _a; + int _b; +}; base-commit: 5f8950b403f6351f125d8281d2e7430a43e7d125 -- 2.39.1