From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 85358 invoked by alias); 30 May 2017 11:47:11 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 85300 invoked by uid 89); 30 May 2017 11:47:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: fencepost.gnu.org Received: from fencepost.gnu.org (HELO fencepost.gnu.org) (208.118.235.10) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 30 May 2017 11:47:06 +0000 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37188) by fencepost.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1dFfcC-0005aN-NU for gcc-patches@gnu.org; Tue, 30 May 2017 07:47:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dFfcB-0003PG-6y for gcc-patches@gnu.org; Tue, 30 May 2017 07:47:08 -0400 Received: from mail-oi0-x234.google.com ([2607:f8b0:4003:c06::234]:32973) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dFfcB-0003PA-27 for gcc-patches@gnu.org; Tue, 30 May 2017 07:47:07 -0400 Received: by mail-oi0-x234.google.com with SMTP id w10so107848426oif.0 for ; Tue, 30 May 2017 04:47:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=d5Qg3TvBMksJ4jluP/msrF3mgKdyD189chXCMLX+130=; b=EsgJ6K+Q7Z2rdZhLY5gE/bMMRtxsnPiJQA2v/HiFop+zwFVnAOv7z2ot6sQx2EhyW5 D77+hxifNL/a6BgEQmXowHw2FoxsNVemvCP0HZrmuNgB4sGUUdY9iyuu2vMfHuDaP3ea Ld6xJACT+D5mMR8EflABFBSw/5wt3hNSvQOzJIutoj1CpiZ/z6H+z2+H/v88U1tWRSfn xHGgMPCAGMFsqwBW/8Xhh1L+tzETc6cG3C5zYWnYPEOvBIPPs8acicZhkmrCCozjoLJw Ak+/JDLc73Pf+y+xUgYL9NMbVSFW5uXmIp50TpxVUj/YdqWNnFlYEsjrQQ/0qcJI/bRE kyBw== X-Gm-Message-State: AODbwcAyO4pQ3wJ9ylO1tVwC/cQ6X0IRupdMES/QGFnVhbcJ7J8MFmiF obsrC6skQ7Y/OJGB+KAQnKiOHk1T3w== X-Received: by 10.202.95.5 with SMTP id t5mr7796569oib.19.1496144826321; Tue, 30 May 2017 04:47:06 -0700 (PDT) MIME-Version: 1.0 Received: by 10.157.51.83 with HTTP; Tue, 30 May 2017 04:47:05 -0700 (PDT) In-Reply-To: References: <9e3b3ee6-0c67-f974-c38e-7f9f25249914@gmail.com> <377838f2-a96b-3147-20ba-d14b1a4bea40@gmail.com> From: Richard Biener Date: Tue, 30 May 2017 11:50:00 -0000 Message-ID: Subject: Re: [PATCH 1/5][GIMPLE FE] PR testsuite/80580. Handle missing labels in goto statements To: Mikhail Maltsev Cc: gcc-patches , Prathamesh Kulkarni Content-Type: text/plain; charset="UTF-8" X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4003:c06::234 X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg02259.txt.bz2 On Tue, May 30, 2017 at 1:46 PM, Richard Biener wrote: > On Mon, May 29, 2017 at 6:38 AM, Mikhail Maltsev wrote: >> Hi. Sorry for a long delay. >> >> On 02.05.2017 17:16, Richard Biener wrote: >>> Certainly an improvement. I suppose we can do better error recovery >>> for cases like >>> >>> if (1) >>> goto >>> else >>> goto bar; >>> >>> but I guess this is better than nothing. >> I think it's worth spending a bit more time to get this right. >> >>> >>> And yes, we use c_parser_error -- input_location should be ok but here >>> we just peek which may upset the parser. Maybe it works better >>> when consuming the token before issueing the error? Thus >>> >>> Index: gcc/c/gimple-parser.c >>> =================================================================== >>> --- gcc/c/gimple-parser.c (revision 247498) >>> +++ gcc/c/gimple-parser.c (working copy) >>> @@ -1315,8 +1315,8 @@ c_parser_gimple_if_stmt (c_parser *parse >>> loc = c_parser_peek_token (parser)->location; >>> c_parser_consume_token (parser); >>> label = c_parser_peek_token (parser)->value; >>> - t_label = lookup_label_for_goto (loc, label); >>> c_parser_consume_token (parser); >>> + t_label = lookup_label_for_goto (loc, label); >>> if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>")) >>> return; >>> } >>> >> I was more focused on cases with missing labels (i.e. 'label' is NULL), rather >> than cases with syntactically correct if-statements referencing undefined >> labels. And, frankly speaking, I'm not sure that swapping >> 'c_parser_consume_token' with 'lookup_label_for_goto' will help, because >> 'lookup_label_for_goto' already gets a 'loc' parameter. > > Ah, indeed. > >> BTW, unfortunately GIMPLE FE does not handle undefined labels properly. I.e., >> this test case >> >> __GIMPLE() void foo() >> { >> bb_0: >> if (0) >> goto bb_0; >> else >> goto bb_1; >> } >> >> causes an ICE somewhere in build_gimple_cfg/cleanup_dead_labels. But this is a >> separate issue, of course. > > Yes. I think ICEing for invalid GIMPLE (as opposed for syntactic > errors) is OK for now. > >> I attached a slightly modified patch, which incorporates your changes and also uses >> >> if (! c_parser_next_token_is (parser, CPP_NAME)) >> ... >> >> instead of >> >> label = c_parser_peek_token (parser)->value; >> ... >> if (!label) >> ... >> >> for better readability. This version correctly handles missing labels and >> semicolons in both branches of the 'if' statement. >> >> The only major problem, which I want to fix is error recovery in the following >> example: >> >> __GIMPLE() void foo() >> { >> if (1) >> goto lbl; >> else >> goto ; /* { dg-error "expected label" } */ >> } >> >> __GIMPLE() void bar() >> { >> if (1) >> goto lbl; >> else >> goto >> } /* { dg-error "expected label" } */ >> >> In this case GCC correctly diagnoses both errors. But if I swap these two >> functions so that 'bar' comes before 'foo', the error in 'foo' is not diagnosed. >> I did not dive into details, but my speculation is that the parser enters some >> strange state and skips 'foo' entirely (-fdump-tree-gimple contains only 'bar'). >> If I add another function after 'foo', it is handled correctly. >> Any ideas, why this could happen and how to improve error recovery in this case? > > Huh. I suppose this is due to us testing c_parser_error () to skip > tokens in some places and > not clearing it after (successfully) ending parsing of a function. > > Not sure where clearing of parser->error happens usually, it somewhat > looks like it has > to be done manually somewhere up in the callstack (I suppose once we managed to > "recover"). Most c_parser_skip* routines clear error for example. Oh, and the patch you posted is ok. Thanks, Richard. > Richard. > >> >> -- >> Regards, >> Mikhail Maltsev