Message ID | 676699df-01c3-690a-d49f-8d00d1891246@linux.ibm.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0C7D43858411 for <patchwork@sourceware.org>; Tue, 21 Sep 2021 22:36:33 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0C7D43858411 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1632263793; bh=pP743f5nmEXJb6G7jhSmNV+uHTaf5LQzCHPbeaKPXck=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=jNr8Eb8W/cEeqcJrdazchZE5l45WAYtKWs5Sv8kmDzI5LfcG+8ZxmpoI0JimBAjWt tM62YUOKGYmeMLodkstBlpmuVWvt47V5FZ4TNH2uXcMtgQCneVC9k8PxEu3TWfCcPh 2NsSpIDeblttFzmNQsr4VN+/k6KFOclk0+H5iXe4= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id EB9E33858D39 for <gcc-patches@gcc.gnu.org>; Tue, 21 Sep 2021 22:36:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EB9E33858D39 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 18LLVgvb015624; Tue, 21 Sep 2021 18:36:00 -0400 Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3b7qcxs23x-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 21 Sep 2021 18:35:59 -0400 Received: from m0098394.ppops.net (m0098394.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 18LMZHDJ031737; Tue, 21 Sep 2021 18:35:59 -0400 Received: from ppma05wdc.us.ibm.com (1b.90.2fa9.ip4.static.sl-reverse.com [169.47.144.27]) by mx0a-001b2d01.pphosted.com with ESMTP id 3b7qcxs23e-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 21 Sep 2021 18:35:59 -0400 Received: from pps.filterd (ppma05wdc.us.ibm.com [127.0.0.1]) by ppma05wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 18LMXr0u013726; Tue, 21 Sep 2021 22:35:57 GMT Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by ppma05wdc.us.ibm.com with ESMTP id 3b7q6ps1p4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 21 Sep 2021 22:35:57 +0000 Received: from b03ledav002.gho.boulder.ibm.com (b03ledav002.gho.boulder.ibm.com [9.17.130.233]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 18LMZvSZ20644320 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 21 Sep 2021 22:35:57 GMT Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id EF7F7136076; Tue, 21 Sep 2021 22:35:56 +0000 (GMT) Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A5D85136068; Tue, 21 Sep 2021 22:35:56 +0000 (GMT) Received: from Bills-MacBook-Pro.local (unknown [9.211.85.128]) by b03ledav002.gho.boulder.ibm.com (Postfix) with ESMTP; Tue, 21 Sep 2021 22:35:56 +0000 (GMT) To: GCC Patches <gcc-patches@gcc.gnu.org> Subject: [PATCH] rs6000: Add psabi diagnostic for C++ zero-width bit field ABI change (PR102024) Message-ID: <676699df-01c3-690a-d49f-8d00d1891246@linux.ibm.com> Date: Tue, 21 Sep 2021 17:35:56 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 Content-Language: en-GB X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: Mf9MsWLtHAbwX1LHqjIV2Mb91Wz65MiT X-Proofpoint-GUID: Ns9z8LTO-G9-qDsBp8MBnKAojxq931__ X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.391,FMLib:17.0.607.475 definitions=2021-09-21_07,2021-09-20_01,2020-04-07_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 mlxscore=0 phishscore=0 mlxlogscore=999 bulkscore=0 clxscore=1015 priorityscore=1501 spamscore=0 adultscore=0 malwarescore=0 suspectscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109200000 definitions=main-2109210134 X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, HTML_MESSAGE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Bill Schmidt via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: wschmidt@linux.ibm.com Cc: Jakub Jelinek <jakub@redhat.com>, Segher Boessenkool <segher@kernel.crashing.org>, David Edelsohn <dje.gcc@gmail.com>, willschm@linux.ibm.com Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
rs6000: Add psabi diagnostic for C++ zero-width bit field ABI change (PR102024)
|
|
Commit Message
Li, Pan2 via Gcc-patches
Sept. 21, 2021, 10:35 p.m. UTC
Hi! Previously zero-width bit fields were removed from structs, so that otherwise homogeneous aggregates were treated as such and passed in FPRs and VSRs. This was incorrect behavior per the ELFv2 ABI. Now that these fields are no longer being removed, we generate the correct parameter passing code. Alert the unwary user in the rare cases where this behavior changes. As noted in the PR, once the GCC 12 Changes page has text describing this issue, we can update the diagnostic message to reference that URL. I'll handle that in a follow-up patch. Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions. Is this okay for trunk? Thanks! Bill 2021-09-21 Bill Schmidt <wschmidt@linux.ibm.com> gcc/ PR target/102024 * config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Detect zero-width bit fields and return indicator. (rs6000_discover_homogeneous_aggregate): Diagnose when the presence of a zero-width bit field changes parameter passing in GCC 12. gcc/testsuite/ PR target/102024 * g++.target/powerpc/pr102024.C: New. --- gcc/config/rs6000/rs6000-call.c | 39 ++++++++++++++++++--- gcc/testsuite/g++.target/powerpc/pr102024.C | 23 ++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.target/powerpc/pr102024.C
Comments
On Tue, 2021-09-21 at 17:35 -0500, Bill Schmidt wrote: > Hi! > > Previously zero-width bit fields were removed from structs, so that otherwise > homogeneous aggregates were treated as such and passed in FPRs and VSRs. > This was incorrect behavior per the ELFv2 ABI. Now that these fields are no > longer being removed, we generate the correct parameter passing code. Alert > the unwary user in the rare cases where this behavior changes. > > As noted in the PR, once the GCC 12 Changes page has text describing this issue, > we can update the diagnostic message to reference that URL. I'll handle that > in a follow-up patch. > > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions. > Is this okay for trunk? How previously? is this one that will need all the backports? > > Thanks! > Bill > > > 2021-09-21 Bill Schmidt <wschmidt@linux.ibm.com> > > gcc/ > PR target/102024 > * config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Detect > zero-width bit fields and return indicator. > (rs6000_discover_homogeneous_aggregate): Diagnose when the > presence of a zero-width bit field changes parameter passing in > GCC 12. > > gcc/testsuite/ > PR target/102024 > * g++.target/powerpc/pr102024.C: New. ok > --- > gcc/config/rs6000/rs6000-call.c | 39 ++++++++++++++++++--- > gcc/testsuite/g++.target/powerpc/pr102024.C | 23 ++++++++++++ > 2 files changed, 57 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/g++.target/powerpc/pr102024.C > > diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c > index 7d485480225..c02b202b0cd 100644 > --- a/gcc/config/rs6000/rs6000-call.c > +++ b/gcc/config/rs6000/rs6000-call.c > @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = { > > static int > rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > - int *empty_base_seen) > + int *empty_base_seen, int *zero_width_bf_seen) > { > machine_mode mode; > HOST_WIDE_INT size; > @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > return -1; > > count = rs6000_aggregate_candidate (TREE_TYPE (type), modep, > - empty_base_seen); > + empty_base_seen, > + zero_width_bf_seen); > if (count == -1 > || !index > || !TYPE_MAX_VALUE (index) > @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > if (TREE_CODE (field) != FIELD_DECL) > continue; > > + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field)) > + { > + *zero_width_bf_seen = 1; > + continue; > + } > + Noting that the definition comes from tree.h and is #define SET_DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD(NODE, VAL) \ do { \ gcc_checking_assert (DECL_BIT_FIELD (NODE)); \ FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0 = (VAL); \ } while (0) ok. > if (DECL_FIELD_ABI_IGNORED (field)) > { > if (lookup_attribute ("no_unique_address", > @@ -6347,7 +6354,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > } > > sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep, > - empty_base_seen); > + empty_base_seen, > + zero_width_bf_seen); > if (sub_count < 0) > return -1; > count += sub_count; > @@ -6381,7 +6389,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > continue; > > sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep, > - empty_base_seen); > + empty_base_seen, > + zero_width_bf_seen); > if (sub_count < 0) > return -1; > count = count > sub_count ? count : sub_count; > @@ -6423,8 +6432,10 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type, > { > machine_mode field_mode = VOIDmode; > int empty_base_seen = 0; > + int zero_width_bf_seen = 0; > int field_count = rs6000_aggregate_candidate (type, &field_mode, > - &empty_base_seen); > + &empty_base_seen, > + &zero_width_bf_seen); > That appears to be all of the callers of rs6000_aggregate_candidate. (ok). > if (field_count > 0) > { > @@ -6460,6 +6471,24 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type, > last_reported_type_uid = uid; > } > } > + if (zero_width_bf_seen && warn_psabi) > + { > + static unsigned last_reported_type_uid; > + unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); > + if (uid != last_reported_type_uid) > + { > + inform (input_location, > + "parameter passing for an argument containing " > + "zero-width bit fields but that is otherwise a " > + "homogeneous aggregate changed in GCC 12.1"); > + last_reported_type_uid = uid; > + } > + if (elt_mode) > + *elt_mode = mode; > + if (n_elts) > + *n_elts = 1; > + return false; > + } In comparison to the other nearby warn_psabi logic, this seems reasonable. (ok) > return true; > } > } > diff --git a/gcc/testsuite/g++.target/powerpc/pr102024.C b/gcc/testsuite/g++.target/powerpc/pr102024.C > new file mode 100644 > index 00000000000..29c9678acfd > --- /dev/null > +++ b/gcc/testsuite/g++.target/powerpc/pr102024.C > @@ -0,0 +1,23 @@ > +// PR target/102024 > +// { dg-do compile { target powerpc_elfv2 } } > +// { dg-options "-O2" } > + > +// Test that a zero-width bit field in an otherwise homogeneous aggregate > +// generates a psabi warning and passes arguments in GPRs. > + > +// { dg-final { scan-assembler-times {\mstd\M} 4 } } > + > +struct a_thing > +{ > + double x; > + double y; > + double z; > + int : 0; > + double w; > +}; > + > +double > +foo (a_thing a) // { dg-message "parameter passing for an argument containing zero-width bit fields but that is otherwise a homogeneous aggregate changed in GCC 12.1" } > +{ > + return a.x * a.y + a.z - a.w; > +} lgtm thanks -Will
On 9/22/21 9:35 AM, will schmidt wrote: > On Tue, 2021-09-21 at 17:35 -0500, Bill Schmidt wrote: >> Hi! >> >> Previously zero-width bit fields were removed from structs, so that otherwise >> homogeneous aggregates were treated as such and passed in FPRs and VSRs. >> This was incorrect behavior per the ELFv2 ABI. Now that these fields are no >> longer being removed, we generate the correct parameter passing code. Alert >> the unwary user in the rare cases where this behavior changes. >> >> As noted in the PR, once the GCC 12 Changes page has text describing this issue, >> we can update the diagnostic message to reference that URL. I'll handle that >> in a follow-up patch. >> >> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions. >> Is this okay for trunk? > How previously? is this one that will need all the backports? No, the change happened recently on trunk. Thanks very much for the review! Bill > >> Thanks! >> Bill >> >> >> 2021-09-21 Bill Schmidt <wschmidt@linux.ibm.com> >> >> gcc/ >> PR target/102024 >> * config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Detect >> zero-width bit fields and return indicator. >> (rs6000_discover_homogeneous_aggregate): Diagnose when the >> presence of a zero-width bit field changes parameter passing in >> GCC 12. >> >> gcc/testsuite/ >> PR target/102024 >> * g++.target/powerpc/pr102024.C: New. > > ok > >> --- >> gcc/config/rs6000/rs6000-call.c | 39 ++++++++++++++++++--- >> gcc/testsuite/g++.target/powerpc/pr102024.C | 23 ++++++++++++ >> 2 files changed, 57 insertions(+), 5 deletions(-) >> create mode 100644 gcc/testsuite/g++.target/powerpc/pr102024.C >> >> diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c >> index 7d485480225..c02b202b0cd 100644 >> --- a/gcc/config/rs6000/rs6000-call.c >> +++ b/gcc/config/rs6000/rs6000-call.c >> @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = { >> >> static int >> rs6000_aggregate_candidate (const_tree type, machine_mode *modep, >> - int *empty_base_seen) >> + int *empty_base_seen, int *zero_width_bf_seen) >> { >> machine_mode mode; >> HOST_WIDE_INT size; >> @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, >> return -1; >> >> count = rs6000_aggregate_candidate (TREE_TYPE (type), modep, >> - empty_base_seen); >> + empty_base_seen, >> + zero_width_bf_seen); >> if (count == -1 >> || !index >> || !TYPE_MAX_VALUE (index) >> @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, >> if (TREE_CODE (field) != FIELD_DECL) >> continue; >> >> + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field)) >> + { >> + *zero_width_bf_seen = 1; >> + continue; >> + } >> + > Noting that the definition comes from tree.h and is > #define SET_DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD(NODE, VAL) \ > do { \ > gcc_checking_assert (DECL_BIT_FIELD (NODE)); \ > FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_0 = (VAL); \ > } while (0) > > ok. > > > >> if (DECL_FIELD_ABI_IGNORED (field)) >> { >> if (lookup_attribute ("no_unique_address", >> @@ -6347,7 +6354,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, >> } >> >> sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep, >> - empty_base_seen); >> + empty_base_seen, >> + zero_width_bf_seen); >> if (sub_count < 0) >> return -1; >> count += sub_count; >> @@ -6381,7 +6389,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, >> continue; >> >> sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep, >> - empty_base_seen); >> + empty_base_seen, >> + zero_width_bf_seen); >> if (sub_count < 0) >> return -1; >> count = count > sub_count ? count : sub_count; >> @@ -6423,8 +6432,10 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type, >> { >> machine_mode field_mode = VOIDmode; >> int empty_base_seen = 0; >> + int zero_width_bf_seen = 0; >> int field_count = rs6000_aggregate_candidate (type, &field_mode, >> - &empty_base_seen); >> + &empty_base_seen, >> + &zero_width_bf_seen); >> > That appears to be all of the callers of rs6000_aggregate_candidate. > (ok). > >> if (field_count > 0) >> { >> @@ -6460,6 +6471,24 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type, >> last_reported_type_uid = uid; >> } >> } >> + if (zero_width_bf_seen && warn_psabi) >> + { >> + static unsigned last_reported_type_uid; >> + unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); >> + if (uid != last_reported_type_uid) >> + { >> + inform (input_location, >> + "parameter passing for an argument containing " >> + "zero-width bit fields but that is otherwise a " >> + "homogeneous aggregate changed in GCC 12.1"); >> + last_reported_type_uid = uid; >> + } >> + if (elt_mode) >> + *elt_mode = mode; >> + if (n_elts) >> + *n_elts = 1; >> + return false; >> + } > In comparison to the other nearby warn_psabi logic, this seems > reasonable. (ok) > > >> return true; >> } >> } >> diff --git a/gcc/testsuite/g++.target/powerpc/pr102024.C b/gcc/testsuite/g++.target/powerpc/pr102024.C >> new file mode 100644 >> index 00000000000..29c9678acfd >> --- /dev/null >> +++ b/gcc/testsuite/g++.target/powerpc/pr102024.C >> @@ -0,0 +1,23 @@ >> +// PR target/102024 >> +// { dg-do compile { target powerpc_elfv2 } } >> +// { dg-options "-O2" } >> + >> +// Test that a zero-width bit field in an otherwise homogeneous aggregate >> +// generates a psabi warning and passes arguments in GPRs. >> + >> +// { dg-final { scan-assembler-times {\mstd\M} 4 } } >> + >> +struct a_thing >> +{ >> + double x; >> + double y; >> + double z; >> + int : 0; >> + double w; >> +}; >> + >> +double >> +foo (a_thing a) // { dg-message "parameter passing for an argument containing zero-width bit fields but that is otherwise a homogeneous aggregate changed in GCC 12.1" } >> +{ >> + return a.x * a.y + a.z - a.w; >> +} > lgtm > thanks > -Will > > >
On Wed, Sep 22, 2021 at 09:43:23AM -0500, Bill Schmidt wrote: > > How previously? is this one that will need all the backports? > > No, the change happened recently on trunk. It is actually more complex. Both C and C++ FEs thought they were removing zero bit fields, but neither did that, then the non-working code from C FE has been removed, and finally in 4.5 the C++ FE has been "fixed" to remove zero bit fields "correctly". And 12 is going to remove the removal, but marks FIELD_DECLs that were in 4.5-11 removed and now aren't for -Wpsabi purposes. So, for most backends, C and C++ was ABI compatible in presence of :0 initially, then for several got incompatible in 4.5 and now is time to decide for each backend what to do according to their psABI, if :0 should be ignored or not during the function arg/return value passing decisions. > > > --- a/gcc/config/rs6000/rs6000-call.c > > > +++ b/gcc/config/rs6000/rs6000-call.c > > > @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = { > > > static int > > > rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > > > - int *empty_base_seen) > > > + int *empty_base_seen, int *zero_width_bf_seen) > > > { > > > machine_mode mode; > > > HOST_WIDE_INT size; > > > @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > > > return -1; > > > count = rs6000_aggregate_candidate (TREE_TYPE (type), modep, > > > - empty_base_seen); > > > + empty_base_seen, > > > + zero_width_bf_seen); > > > if (count == -1 > > > || !index > > > || !TYPE_MAX_VALUE (index) > > > @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > > > if (TREE_CODE (field) != FIELD_DECL) > > > continue; > > > + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field)) > > > + { > > > + *zero_width_bf_seen = 1; > > > + continue; > > > + } So, from what you wrote, :0 in the ppc* psABIs the intent is that :0 is not ignored, right? In that case I don't really understand the above (the continue in particular). Because the continue means it is ignored for C++ and not ignored for C, so basically you return to the 4.5-11 ABI incompatibility between C and C++. C++ :0 will have DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD set, C :0 will not... Jakub
On Wed, Sep 22, 2021 at 05:02:15PM +0200, Jakub Jelinek via Gcc-patches wrote: > > > > @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > > > > return -1; > > > > count = rs6000_aggregate_candidate (TREE_TYPE (type), modep, > > > > - empty_base_seen); > > > > + empty_base_seen, > > > > + zero_width_bf_seen); > > > > if (count == -1 > > > > || !index > > > > || !TYPE_MAX_VALUE (index) > > > > @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > > > > if (TREE_CODE (field) != FIELD_DECL) > > > > continue; > > > > + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field)) > > > > + { > > > > + *zero_width_bf_seen = 1; > > > > + continue; > > > > + } > > So, from what you wrote, :0 in the ppc* psABIs the intent is that :0 is not > ignored, right? > In that case I don't really understand the above (the continue in > particular). Because the continue means it is ignored for C++ and not > ignored for C, so basically you return to the 4.5-11 ABI incompatibility > between C and C++. > C++ :0 will have DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD set, C :0 will not... To be more precise, I'd expect what most targets want to do for the actual ABI decisions not to use DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD at all. I.e. do: if (TREE_CODE (field) != FIELD_DECL) continue; if (DECL_BIT_FIELD (field) && integer_zerop (DECL_SIZE (field))) { // :0 // in some psABIs, ignore it, i.e. continue; // in others psABIs, take them into account, i.e. do nothing. } and use DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD only for the -Wpsabi purposes. The only exception would be for targets that decide to keep GCC 4.5-11 compatibility with the C incompatible with C++. Jakub
Hi Jakub, On 9/22/21 11:33 AM, Jakub Jelinek wrote: > On Wed, Sep 22, 2021 at 05:02:15PM +0200, Jakub Jelinek via Gcc-patches wrote: >>>>> @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, >>>>> return -1; >>>>> count = rs6000_aggregate_candidate (TREE_TYPE (type), modep, >>>>> - empty_base_seen); >>>>> + empty_base_seen, >>>>> + zero_width_bf_seen); >>>>> if (count == -1 >>>>> || !index >>>>> || !TYPE_MAX_VALUE (index) >>>>> @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, >>>>> if (TREE_CODE (field) != FIELD_DECL) >>>>> continue; >>>>> + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field)) >>>>> + { >>>>> + *zero_width_bf_seen = 1; >>>>> + continue; >>>>> + } >> So, from what you wrote, :0 in the ppc* psABIs the intent is that :0 is not >> ignored, right? >> In that case I don't really understand the above (the continue in >> particular). Because the continue means it is ignored for C++ and not >> ignored for C, so basically you return to the 4.5-11 ABI incompatibility >> between C and C++. >> C++ :0 will have DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD set, C :0 will not... > To be more precise, I'd expect what most targets want to do for the > actual ABI decisions not to use DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD at all. > I.e. do: > if (TREE_CODE (field) != FIELD_DECL) > continue; > if (DECL_BIT_FIELD (field) && integer_zerop (DECL_SIZE (field))) > { > // :0 > // in some psABIs, ignore it, i.e. continue; > // in others psABIs, take them into account, i.e. do nothing. > } > and use DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD only for the -Wpsabi purposes. > > The only exception would be for targets that decide to keep GCC 4.5-11 > compatibility with the C incompatible with C++. I think you're misunderstanding what I'm trying to do with this patch. I am not changing the code generation at all (your patch did that). All I'm doing is detecting when the old code generation and new code generation will differ, and emitting a diagnostic in that case. The way I do that is to allow rs6000_aggregate_candidate to *think* that something is a homogeneous aggregate even when zero-width bitfields are present (hence the continue), but record the fact that we saw one. This gives the same answer as we gave before your patch. Then, in rs6000_discover_homogeneous_aggregate, once we think we have a homogeneous aggregate, we check whether we actually had a zero-width bitfield present. If so, then we diagnose the change in code generation and return false, indicating that we didn't actually find a homogeneous aggregate. Other than the diagnostic, this matches the behavior after your patch. I've verified that we didn't change code generation for C code with zero-width bitfields as a result of either your patch or mine. Before and after, a C struct containing a zero-width bitfield causes us to avoid generating code for a homogeneous aggregate, just as we do for C++ after your patch. I hope this helps clear things up, and I apologize for not giving a better description of my intent. Thanks! Bill > > Jakub >
Hi! On Tue, Sep 21, 2021 at 05:35:56PM -0500, Bill Schmidt wrote: > Previously zero-width bit fields were removed from structs, so that > otherwise > homogeneous aggregates were treated as such and passed in FPRs and VSRs. > This was incorrect behavior per the ELFv2 ABI. Now that these fields are no > longer being removed, we generate the correct parameter passing code. Alert > the unwary user in the rare cases where this behavior changes. > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no > regressions. > Is this okay for trunk? This can obviously not change anything for other ABIs, so it doesn't need testing anywhere else. Good :-) > @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types > altivec_overloaded_builtins[] = { > > static int > rs6000_aggregate_candidate (const_tree type, machine_mode *modep, > - int *empty_base_seen) > + int *empty_base_seen, int *zero_width_bf_seen) The new function parameter should be described in the function comment. > + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field)) > + { > + *zero_width_bf_seen = 1; > + continue; > + } Please add a comment here, saying what it is for? I'd do it inside the braces. > sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep, > - empty_base_seen); > + empty_base_seen, > + zero_width_bf_seen); So it is important that this function only ever sets *zero_width_bf_seen to 1, never resets it; please document that with it as well? > + inform (input_location, > + "parameter passing for an argument containing " > + "zero-width bit fields but that is otherwise a > " > + "homogeneous aggregate changed in GCC 12.1"); Just "GCC 12" please. You might want to indicate that older compilers did the wrong thing here. And maybe say this is only for ELFv2 somehow? In some way that doesn't make it more confusing than not saying it :-) > +double > +foo (a_thing a) // { dg-message "parameter passing for an argument > containing zero-width bit fields but that is otherwise a homogeneous > aggregate changed in GCC 12.1" } I think you used "format=flawed" again? Okay for trunk with such comment updates. Thanks! Segher
Hi Segher, Thanks for the review! This is what I committed. 2021-09-23 Bill Schmidt <wschmidt@linux.ibm.com> gcc/ PR target/102024 * config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Detect zero-width bit fields and return indicator. (rs6000_discover_homogeneous_aggregate): Diagnose when the presence of a zero-width bit field changes parameter passing in GCC 12. gcc/testsuite/ PR target/102024 * g++.target/powerpc/pr102024.C: New. --- gcc/config/rs6000/rs6000-call.c | 64 +++++++++++++++++++-- gcc/testsuite/g++.target/powerpc/pr102024.C | 23 ++++++++ 2 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.target/powerpc/pr102024.C diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c index 7d485480225..2eceb2c71c0 100644 --- a/gcc/config/rs6000/rs6000-call.c +++ b/gcc/config/rs6000/rs6000-call.c @@ -6223,11 +6223,19 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = { or vector type. If a non-floating point or vector type is found, or if a floating point or vector type that doesn't match a non-VOIDmode *MODEP is found, then return -1, otherwise return the count in the - sub-tree. */ + sub-tree. + + There have been some ABI snafus along the way with C++. Modify + EMPTY_BASE_SEEN to a nonzero value iff a C++ empty base class makes + an appearance; separate flag bits indicate whether or not such a + field is marked "no unique address". Modify ZERO_WIDTH_BF_SEEN + to 1 iff a C++ zero-length bitfield makes an appearance, but + in this case otherwise treat this as still being a homogeneous + aggregate. */ static int rs6000_aggregate_candidate (const_tree type, machine_mode *modep, - int *empty_base_seen) + int *empty_base_seen, int *zero_width_bf_seen) { machine_mode mode; HOST_WIDE_INT size; @@ -6298,7 +6306,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, return -1; count = rs6000_aggregate_candidate (TREE_TYPE (type), modep, - empty_base_seen); + empty_base_seen, + zero_width_bf_seen); if (count == -1 || !index || !TYPE_MAX_VALUE (index) @@ -6336,6 +6345,26 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, if (TREE_CODE (field) != FIELD_DECL) continue; + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field)) + { + /* GCC 11 and earlier generated incorrect code in a rare + corner case for C++. When a RECORD_TYPE looks like a + homogeneous aggregate, except that it also contains + one or more zero-width bit fields, these earlier + compilers would incorrectly pass the fields in FPRs + or VSRs. This occurred because the front end wrongly + removed these bitfields from the RECORD_TYPE. In + GCC 12 and later, the front end flaw was corrected. + We want to diagnose this case. To do this, we pretend + that we don't see the zero-width bit fields (hence + the continue statement here), but pass back a flag + indicating what happened. The caller then diagnoses + the issue and rejects the RECORD_TYPE as a homogeneous + aggregate. */ + *zero_width_bf_seen = 1; + continue; + } + if (DECL_FIELD_ABI_IGNORED (field)) { if (lookup_attribute ("no_unique_address", @@ -6347,7 +6376,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, } sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep, - empty_base_seen); + empty_base_seen, + zero_width_bf_seen); if (sub_count < 0) return -1; count += sub_count; @@ -6381,7 +6411,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, continue; sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep, - empty_base_seen); + empty_base_seen, + zero_width_bf_seen); if (sub_count < 0) return -1; count = count > sub_count ? count : sub_count; @@ -6423,8 +6454,10 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type, { machine_mode field_mode = VOIDmode; int empty_base_seen = 0; + int zero_width_bf_seen = 0; int field_count = rs6000_aggregate_candidate (type, &field_mode, - &empty_base_seen); + &empty_base_seen, + &zero_width_bf_seen); if (field_count > 0) { @@ -6460,6 +6493,25 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type, last_reported_type_uid = uid; } } + if (zero_width_bf_seen && warn_psabi) + { + static unsigned last_reported_type_uid; + unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); + if (uid != last_reported_type_uid) + { + inform (input_location, + "ELFv2 parameter passing for an argument " + "containing zero-width bit fields but that is " + "otherwise a homogeneous aggregate was " + "corrected in GCC 12"); + last_reported_type_uid = uid; + } + if (elt_mode) + *elt_mode = mode; + if (n_elts) + *n_elts = 1; + return false; + } return true; } } diff --git a/gcc/testsuite/g++.target/powerpc/pr102024.C b/gcc/testsuite/g++.target/powerpc/pr102024.C new file mode 100644 index 00000000000..769585052b5 --- /dev/null +++ b/gcc/testsuite/g++.target/powerpc/pr102024.C @@ -0,0 +1,23 @@ +// PR target/102024 +// { dg-do compile { target powerpc_elfv2 } } +// { dg-options "-O2" } + +// Test that a zero-width bit field in an otherwise homogeneous aggregate +// generates a psabi warning and passes arguments in GPRs. + +// { dg-final { scan-assembler-times {\mstd\M} 4 } } + +struct a_thing +{ + double x; + double y; + double z; + int : 0; + double w; +}; + +double +foo (a_thing a) // { dg-message "ELFv2 parameter passing for an argument containing zero-width bit fields but that is otherwise a homogeneous aggregate was corrected in GCC 12" } +{ + return a.x * a.y + a.z - a.w; +}
diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c index 7d485480225..c02b202b0cd 100644 --- a/gcc/config/rs6000/rs6000-call.c +++ b/gcc/config/rs6000/rs6000-call.c @@ -6227,7 +6227,7 @@ const struct altivec_builtin_types altivec_overloaded_builtins[] = { static int rs6000_aggregate_candidate (const_tree type, machine_mode *modep, - int *empty_base_seen) + int *empty_base_seen, int *zero_width_bf_seen) { machine_mode mode; HOST_WIDE_INT size; @@ -6298,7 +6298,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, return -1; count = rs6000_aggregate_candidate (TREE_TYPE (type), modep, - empty_base_seen); + empty_base_seen, + zero_width_bf_seen); if (count == -1 || !index || !TYPE_MAX_VALUE (index) @@ -6336,6 +6337,12 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, if (TREE_CODE (field) != FIELD_DECL) continue; + if (DECL_FIELD_CXX_ZERO_WIDTH_BIT_FIELD (field)) + { + *zero_width_bf_seen = 1; + continue; + } + if (DECL_FIELD_ABI_IGNORED (field)) { if (lookup_attribute ("no_unique_address", @@ -6347,7 +6354,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, } sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep, - empty_base_seen); + empty_base_seen, + zero_width_bf_seen); if (sub_count < 0) return -1; count += sub_count; @@ -6381,7 +6389,8 @@ rs6000_aggregate_candidate (const_tree type, machine_mode *modep, continue; sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep, - empty_base_seen); + empty_base_seen, + zero_width_bf_seen); if (sub_count < 0) return -1; count = count > sub_count ? count : sub_count; @@ -6423,8 +6432,10 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type, { machine_mode field_mode = VOIDmode; int empty_base_seen = 0; + int zero_width_bf_seen = 0; int field_count = rs6000_aggregate_candidate (type, &field_mode, - &empty_base_seen); + &empty_base_seen, + &zero_width_bf_seen); if (field_count > 0) { @@ -6460,6 +6471,24 @@ rs6000_discover_homogeneous_aggregate (machine_mode mode, const_tree type, last_reported_type_uid = uid; } } + if (zero_width_bf_seen && warn_psabi) + { + static unsigned last_reported_type_uid; + unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type)); + if (uid != last_reported_type_uid) + { + inform (input_location, + "parameter passing for an argument containing " + "zero-width bit fields but that is otherwise a " + "homogeneous aggregate changed in GCC 12.1"); + last_reported_type_uid = uid; + } + if (elt_mode) + *elt_mode = mode; + if (n_elts) + *n_elts = 1; + return false; + } return true; } } diff --git a/gcc/testsuite/g++.target/powerpc/pr102024.C b/gcc/testsuite/g++.target/powerpc/pr102024.C new file mode 100644 index 00000000000..29c9678acfd --- /dev/null +++ b/gcc/testsuite/g++.target/powerpc/pr102024.C @@ -0,0 +1,23 @@ +// PR target/102024 +// { dg-do compile { target powerpc_elfv2 } } +// { dg-options "-O2" } + +// Test that a zero-width bit field in an otherwise homogeneous aggregate +// generates a psabi warning and passes arguments in GPRs. + +// { dg-final { scan-assembler-times {\mstd\M} 4 } } + +struct a_thing +{ + double x; + double y; + double z; + int : 0; + double w; +}; + +double +foo (a_thing a) // { dg-message "parameter passing for an argument containing zero-width bit fields but that is otherwise a homogeneous aggregate changed in GCC 12.1" } +{ + return a.x * a.y + a.z - a.w; +}