Message ID | 20220321091328.2792264-1-guojiufu@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 2F9043857C71 for <patchwork@sourceware.org>; Mon, 21 Mar 2022 09:14:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 2F9043857C71 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1647854044; bh=DmyuBehcVRqwl9tnI8GzAxpNabsRVVaiIdWVBQ/Hqvg=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=kSIsfS5nXsAP6VBTITmndDkqf4P/otr/b14vy95uxZgFO7FyxDsajx0IIx/ce0iXv LCHjzohlI20cPweEWg6AmAF73o5ZT+GgC26hlS3g3q+nfjQv1CuzcKQoOt/+iTDoog aDecO//v9mEGGehW4MmHGugoj3mtFUF9Kte4JQ/I= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id F37733858C83 for <gcc-patches@gcc.gnu.org>; Mon, 21 Mar 2022 09:13:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F37733858C83 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 22L6MnEv008532; Mon, 21 Mar 2022 09:13:34 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3exm1tu8yd-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 21 Mar 2022 09:13:34 +0000 Received: from m0098417.ppops.net (m0098417.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 22L96wxL030226; Mon, 21 Mar 2022 09:13:33 GMT Received: from ppma04ams.nl.ibm.com (63.31.33a9.ip4.static.sl-reverse.com [169.51.49.99]) by mx0a-001b2d01.pphosted.com with ESMTP id 3exm1tu8xv-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 21 Mar 2022 09:13:33 +0000 Received: from pps.filterd (ppma04ams.nl.ibm.com [127.0.0.1]) by ppma04ams.nl.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 22L98nSj029861; Mon, 21 Mar 2022 09:13:32 GMT Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by ppma04ams.nl.ibm.com with ESMTP id 3ew6t8uect-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 21 Mar 2022 09:13:32 +0000 Received: from d06av21.portsmouth.uk.ibm.com (d06av21.portsmouth.uk.ibm.com [9.149.105.232]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 22L9DTN217367402 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 21 Mar 2022 09:13:29 GMT Received: from d06av21.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A2F4A52052; Mon, 21 Mar 2022 09:13:29 +0000 (GMT) Received: from genoa.aus.stglabs.ibm.com (unknown [9.40.192.157]) by d06av21.portsmouth.uk.ibm.com (Postfix) with ESMTP id DB8BB52050; Mon, 21 Mar 2022 09:13:28 +0000 (GMT) To: gcc-patches@gcc.gnu.org Subject: [PATCH]rs6000: avoid peeking eof after __vector keyword Date: Mon, 21 Mar 2022 17:13:28 +0800 Message-Id: <20220321091328.2792264-1-guojiufu@linux.ibm.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: gVh7AHVTLi6cUHdazlFAjY4FYKisWpPl X-Proofpoint-ORIG-GUID: c_y1_uYWYZkkYppzrRE5KKrQVU3QBObU X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.850,Hydra:6.0.425,FMLib:17.11.64.514 definitions=2022-03-21_04,2022-03-15_01,2022-02-23_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 mlxscore=0 clxscore=1015 phishscore=0 bulkscore=0 adultscore=0 lowpriorityscore=0 malwarescore=0 priorityscore=1501 spamscore=0 mlxlogscore=999 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000 definitions=main-2203210059 X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: Jiufu Guo via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jiufu Guo <guojiufu@linux.ibm.com> Cc: dje.gcc@gmail.com, segher@kernel.crashing.org 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: avoid peeking eof after __vector keyword
|
|
Commit Message
Jiufu Guo
March 21, 2022, 9:13 a.m. UTC
Hi! There is a rare corner case: where __vector is followed only with ";" and near the end of the file. Like the case in PR101168: using vdbl = __vector double; #define BREAK 1 For this case, "__vector double" is not followed by a PP_NAME, it is followed by CPP_SEMICOLON and then EOF. In this case, there is no more tokens in the file. Then, do not need to continue to parse the file. This patch pass bootstrap and regtest on ppc64 and ppc64le. BR, Jiufu PR preprocessor/101168 gcc/ChangeLog: * config/rs6000/rs6000-c.cc (rs6000_macro_to_expand): Avoid empty identifier. gcc/testsuite/ChangeLog: * g++.target/powerpc/pr101168.C: New test. --- gcc/config/rs6000/rs6000-c.cc | 4 +++- gcc/testsuite/g++.target/powerpc/pr101168.C | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.target/powerpc/pr101168.C
Comments
On Mon, Mar 21, 2022 at 5:13 AM Jiufu Guo <guojiufu@linux.ibm.com> wrote: > > Hi! > > There is a rare corner case: where __vector is followed only with ";" > and near the end of the file. > > Like the case in PR101168: > using vdbl = __vector double; > #define BREAK 1 > > For this case, "__vector double" is not followed by a PP_NAME, it is > followed by CPP_SEMICOLON and then EOF. In this case, there is no > more tokens in the file. Then, do not need to continue to parse the > file. > > This patch pass bootstrap and regtest on ppc64 and ppc64le. This is okay. Maybe a tweak to the comment, see below. Thanks, David > > > BR, > Jiufu > > > PR preprocessor/101168 > > gcc/ChangeLog: > > * config/rs6000/rs6000-c.cc (rs6000_macro_to_expand): > Avoid empty identifier. > > gcc/testsuite/ChangeLog: > > * g++.target/powerpc/pr101168.C: New test. > > > --- > gcc/config/rs6000/rs6000-c.cc | 4 +++- > gcc/testsuite/g++.target/powerpc/pr101168.C | 6 ++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.target/powerpc/pr101168.C > > diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc > index 3b62b499df2..f8cc7bad812 100644 > --- a/gcc/config/rs6000/rs6000-c.cc > +++ b/gcc/config/rs6000/rs6000-c.cc > @@ -282,7 +282,9 @@ rs6000_macro_to_expand (cpp_reader *pfile, const cpp_token *tok) > expand_bool_pixel = __pixel_keyword; > else if (ident == C_CPP_HASHNODE (__bool_keyword)) > expand_bool_pixel = __bool_keyword; > - else > + > + /* If it needs to check tokens continue. */ Maybe /* If there are more tokens to check. */ ? > + else if (ident) > { > /* Try two tokens down, too. */ > do > diff --git a/gcc/testsuite/g++.target/powerpc/pr101168.C b/gcc/testsuite/g++.target/powerpc/pr101168.C > new file mode 100644 > index 00000000000..284e77fdc88 > --- /dev/null > +++ b/gcc/testsuite/g++.target/powerpc/pr101168.C > @@ -0,0 +1,6 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target powerpc_altivec_ok } */ > +/* { dg-options "-maltivec" } */ > + > +using vdbl = __vector double; > +#define BREAK 1 > -- > 2.25.1 >
On Mon, Mar 21, 2022 at 02:14:08PM -0400, David Edelsohn wrote: > On Mon, Mar 21, 2022 at 5:13 AM Jiufu Guo <guojiufu@linux.ibm.com> wrote: > > There is a rare corner case: where __vector is followed only with ";" > > and near the end of the file. > This is okay. Maybe a tweak to the comment, see below. This whole function could use some restructuring / rewriting to make clearer what it actually does. See the function comment: /* Called to decide whether a conditional macro should be expanded. Since we have exactly one such macro (i.e, 'vector'), we do not need to examine the 'tok' parameter. */ ... followed by 17 uses of "tok". Yes, some of those overwrite the function argument, but that doesn't make it any better! :-P Some factoring would help, too, perhaps. Segher
Hi! Segher Boessenkool <segher@kernel.crashing.org> writes: > On Mon, Mar 21, 2022 at 02:14:08PM -0400, David Edelsohn wrote: >> On Mon, Mar 21, 2022 at 5:13 AM Jiufu Guo <guojiufu@linux.ibm.com> wrote: >> > There is a rare corner case: where __vector is followed only with ";" >> > and near the end of the file. > >> This is okay. Maybe a tweak to the comment, see below. > > This whole function could use some restructuring / rewriting to make > clearer what it actually does. See the function comment: > > /* Called to decide whether a conditional macro should be expanded. > Since we have exactly one such macro (i.e, 'vector'), we do not > need to examine the 'tok' parameter. */ > > ... followed by 17 uses of "tok". Yes, some of those overwrite the > function argument, but that doesn't make it any better! :-P > > Some factoring would help, too, perhaps. Thanks for your review! I am also confused about it when I check this function for the first time. In the function, 'tok' is used directly at the beginning, and then it is overwritten by cpp_peek_token. From the history of this function, the first version of this function contains this 'inconsistency' between comments and implementations. :-P With check related code, it seems this function is used to predicate if a conditional macro should be expanded by peeking two or more tokens. The context-sensitive macros are vector/bool/pixel. Correponding keywords __vector/__bool/__pixel are unconditional. Based on those related codes, the behavior of function rs6000_macro_to_expand would be checking if the 'vector' token is followed by bool/__bool or pixel/__pixel. To do this the 'tok' has to be 'examined'. From this understanding, we may just update the comment. While the patch does not cover this. BR, Jiufu > > > Segher
Hi! On Tue, Mar 22, 2022 at 01:50:39PM +0800, Jiufu Guo wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Mon, Mar 21, 2022 at 02:14:08PM -0400, David Edelsohn wrote: > >> On Mon, Mar 21, 2022 at 5:13 AM Jiufu Guo <guojiufu@linux.ibm.com> wrote: > >> > There is a rare corner case: where __vector is followed only with ";" > >> > and near the end of the file. > > > >> This is okay. Maybe a tweak to the comment, see below. > > > > This whole function could use some restructuring / rewriting to make > > clearer what it actually does. See the function comment: > > > > /* Called to decide whether a conditional macro should be expanded. > > Since we have exactly one such macro (i.e, 'vector'), we do not > > need to examine the 'tok' parameter. */ > > > > ... followed by 17 uses of "tok". Yes, some of those overwrite the > > function argument, but that doesn't make it any better! :-P > > > > Some factoring would help, too, perhaps. > > Thanks for your review! > > I am also confused about it when I check this function for the first > time. In the function, 'tok' is used directly at the beginning, and > then it is overwritten by cpp_peek_token. > >From the history of this function, the first version of this function > contains this 'inconsistency' between comments and implementations. :-P > > With check related code, it seems this function is used to predicate > if a conditional macro should be expanded by peeking two or more > tokens. Yes, and the function comment should say that, that's what it's for :-) > The context-sensitive macros are vector/bool/pixel. Correponding > keywords __vector/__bool/__pixel are unconditional. > Based on those related codes, the behavior of function > rs6000_macro_to_expand would be checking if the 'vector' token is > followed by bool/__bool or pixel/__pixel. To do this the 'tok' has to > be 'examined'. > > >From this understanding, we may just update the comment. > While the patch does not cover this. Yes, and it doesn't have to, probably it's best not to change the code much in stage 4 anyway. It is really hard to fix bugs in it, or to review the resulting patches, without documentation what it is supposed to do (especially if the code isn't clear, is inconsistent, and even contradicts the little documentation that there is). Oh well. Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > Hi! > > On Tue, Mar 22, 2022 at 01:50:39PM +0800, Jiufu Guo wrote: >> Segher Boessenkool <segher@kernel.crashing.org> writes: >> > On Mon, Mar 21, 2022 at 02:14:08PM -0400, David Edelsohn wrote: >> >> On Mon, Mar 21, 2022 at 5:13 AM Jiufu Guo <guojiufu@linux.ibm.com> wrote: >> >> > There is a rare corner case: where __vector is followed only with ";" >> >> > and near the end of the file. >> > >> >> This is okay. Maybe a tweak to the comment, see below. >> > >> > This whole function could use some restructuring / rewriting to make >> > clearer what it actually does. See the function comment: >> > >> > /* Called to decide whether a conditional macro should be expanded. >> > Since we have exactly one such macro (i.e, 'vector'), we do not >> > need to examine the 'tok' parameter. */ >> > >> > ... followed by 17 uses of "tok". Yes, some of those overwrite the >> > function argument, but that doesn't make it any better! :-P >> > >> > Some factoring would help, too, perhaps. >> >> Thanks for your review! >> >> I am also confused about it when I check this function for the first >> time. In the function, 'tok' is used directly at the beginning, and >> then it is overwritten by cpp_peek_token. >> >From the history of this function, the first version of this function >> contains this 'inconsistency' between comments and implementations. :-P >> >> With check related code, it seems this function is used to predicate >> if a conditional macro should be expanded by peeking two or more >> tokens. > > Yes, and the function comment should say that, that's what it's for :-) > >> The context-sensitive macros are vector/bool/pixel. Correponding >> keywords __vector/__bool/__pixel are unconditional. >> Based on those related codes, the behavior of function >> rs6000_macro_to_expand would be checking if the 'vector' token is >> followed by bool/__bool or pixel/__pixel. To do this the 'tok' has to >> be 'examined'. >> >> >From this understanding, we may just update the comment. >> While the patch does not cover this. > > Yes, and it doesn't have to, probably it's best not to change the code > much in stage 4 anyway. It is really hard to fix bugs in it, or to > review the resulting patches, without documentation what it is supposed > to do (especially if the code isn't clear, is inconsistent, and even > contradicts the little documentation that there is). Thanks for your comments! Understand. :) I would commit the patch and submit patch to update the comments in stage 1. BR, Jiufu > > Oh well. > > > Segher
diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc index 3b62b499df2..f8cc7bad812 100644 --- a/gcc/config/rs6000/rs6000-c.cc +++ b/gcc/config/rs6000/rs6000-c.cc @@ -282,7 +282,9 @@ rs6000_macro_to_expand (cpp_reader *pfile, const cpp_token *tok) expand_bool_pixel = __pixel_keyword; else if (ident == C_CPP_HASHNODE (__bool_keyword)) expand_bool_pixel = __bool_keyword; - else + + /* If it needs to check tokens continue. */ + else if (ident) { /* Try two tokens down, too. */ do diff --git a/gcc/testsuite/g++.target/powerpc/pr101168.C b/gcc/testsuite/g++.target/powerpc/pr101168.C new file mode 100644 index 00000000000..284e77fdc88 --- /dev/null +++ b/gcc/testsuite/g++.target/powerpc/pr101168.C @@ -0,0 +1,6 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target powerpc_altivec_ok } */ +/* { dg-options "-maltivec" } */ + +using vdbl = __vector double; +#define BREAK 1