From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83596 invoked by alias); 30 May 2017 11:46:47 -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 83578 invoked by uid 89); 30 May 2017 11:46:46 -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=Hx-spam-relays-external:2607, H*RU:2607, H*RU:f8b0, Hx-spam-relays-external:f8b0 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:46:44 +0000 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37038) by fencepost.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1dFfbq-0005Xe-Pi for gcc-patches@gnu.org; Tue, 30 May 2017 07:46:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dFfbp-0003K5-5B for gcc-patches@gnu.org; Tue, 30 May 2017 07:46:46 -0400 Received: from mail-oi0-x22e.google.com ([2607:f8b0:4003:c06::22e]:35997) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dFfbp-0003JJ-0f for gcc-patches@gnu.org; Tue, 30 May 2017 07:46:45 -0400 Received: by mail-oi0-x22e.google.com with SMTP id h4so107607423oib.3 for ; Tue, 30 May 2017 04:46:42 -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=Knk4XK3LLYMtjH6u0Ad11hFHjM2WgxF4H9V5h3hzyf4=; b=iikjgp6odquKt2JFd2TgXs3ilO9U4Uf4XHz/x61bkLahiWK1cDvki/kT7SqBRP8ojH KVvWEmfN2uz2MmHM6aRuuWKo1KiFxH3cIF1SVlWRHO23iG9WakLzwRDgDGF3bhabEUgv StFqiLjkf6q7b4JTczVMV9aYyPm1GMOTqW87JuVTXZZje/Bcu1ZGYRvt7+yhQewc+txF Ez4AnxTWPPId4T/yjWRAGyJ2MgsuwbhelZZIeKkdLS4Na4/MaluTWhvl3gaAnmPI2dmj pqD2ZAew3EpZhr6sThczTeqWx3otYa1RbBGwTm8NmFIiEL6llTiHySf6cg0JB9QFX6qY vKDA== X-Gm-Message-State: AODbwcAmdHztgODW2pMopFD4K2rN1yHQ6C9nLIgdyfjRA673mRXbGnYD b5rp1WOHu1Nx8EBifPIpQ8Wm3Ot4KA== X-Received: by 10.157.2.98 with SMTP id 89mr11262121otb.105.1496144801029; Tue, 30 May 2017 04:46:41 -0700 (PDT) MIME-Version: 1.0 Received: by 10.157.51.83 with HTTP; Tue, 30 May 2017 04:46:40 -0700 (PDT) In-Reply-To: <377838f2-a96b-3147-20ba-d14b1a4bea40@gmail.com> References: <9e3b3ee6-0c67-f974-c38e-7f9f25249914@gmail.com> <377838f2-a96b-3147-20ba-d14b1a4bea40@gmail.com> From: Richard Biener Date: Tue, 30 May 2017 11:47: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::22e X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg02258.txt.bz2 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. Richard. > > -- > Regards, > Mikhail Maltsev