From patchwork Thu Feb 6 22:40:45 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Janus Weil X-Patchwork-Id: 317563 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 2D0542C0084 for ; Fri, 7 Feb 2014 09:41:57 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=JUn+PjUVNbiaAbi7Dv mwL1+OzNd0m+Dky5FcMJxyKArdjPgeZ+HVcLE2uuEn/aotVFx2fuCsk1atKkKH4v wsEyeCGzaaUDTM9HrzEKeaDTOeCOue/32HjlT91BzLadkKJ8/egQEeKCmKNCDwZC ib67cpbmFbKJfp/pyfLey/S4k= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=do7Ag7oomCEFaqDkg2bPz6BT Okc=; b=ATSzEEfnb7qwRAI77l+y+a2cvtK/moeQSweDAFyPFs2sXgH4rthUdVxn aKLD3iDZ2x5Czr31l/Pjfmee3Y4WKA0ZYLNgoTVR8Y/kLeIZJ3+GoGkkR8rn3Yuk VzAb/AsPpduOoev//2fv76GKjf4kpvd9Dt0wtRk/BSK7Th9C8Dk= Received: (qmail 6923 invoked by alias); 6 Feb 2014 22:41:51 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 6906 invoked by uid 89); 6 Feb 2014 22:41:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-qc0-f174.google.com Received: from mail-qc0-f174.google.com (HELO mail-qc0-f174.google.com) (209.85.216.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 06 Feb 2014 22:41:49 +0000 Received: by mail-qc0-f174.google.com with SMTP id x13so4481265qcv.33 for ; Thu, 06 Feb 2014 14:41:47 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.224.223.134 with SMTP id ik6mr3544060qab.90.1391726445921; Thu, 06 Feb 2014 14:40:45 -0800 (PST) Received: by 10.96.156.38 with HTTP; Thu, 6 Feb 2014 14:40:45 -0800 (PST) In-Reply-To: <52F3EDE4.8010603@sfr.fr> References: <52F3EDE4.8010603@sfr.fr> Date: Thu, 6 Feb 2014 23:40:45 +0100 Message-ID: Subject: Re: [Patch, Fortran] PR 58470: [4.9 Regression] [OOP] ICE on invalid with FINAL procedure and type extension From: Janus Weil To: Mikael Morin Cc: gfortran , gcc-patches Hi Mikael, thanks for your comments ... >> attached is a small patch which fixes an ICE-on-invalid regression >> with finalization. In the PR, Dominique objected to the patch, but I >> think it's the correct thing to do after all. The line that I'm >> removing was added in a patch authored by Tobias and myself. I suspect >> it was added to work around some other problem in the finalization >> implementation, and there is no evidence it's actually needed. >> >> The patch regtests cleanly on x86_64-unknown-linux-gnu. Ok for trunk? >> > Wait a bit; let's try to understand the problem. > > Normally I would say calling gfc_is_finalizable here in > resolve_fl_derived0 is harmless because gfc_resolve_finalizers has been > called before in resolve_fl_derived. > BUT: > resolve_fl_derived0 recurses on its parent type, while > gfc_resolve_finalizers doesn't; and in this case we end up recursing on > type "cfml" whose finalizers haven't been resolved yet. Yes, that's more or less what happens. And the real problem is that gfc_is_finalzable already generates the finalization wrapper (via gfc_find_derived_vtab -> generate_finalizaton_wrapper) before we have checked that the finalizer is actually valid (which is what gfc_resolve_finalizers does). Once we get into gfc_resolve_finalizers, it is fooled to believe that the finalizer has already been resolved and therefore skips the checks and produces no error message. > Now whether your patch is the right thing to do... I'm a bit skeptical > about removing the one use of gfc_is_finalizable in resolve.c. Well, all others occurrences of 'gfc_is_finalizable' are in trans*, so this is the only one that comes too early. Also the fact that its return value is not used here tells you that we are not actually interested if the type is finalizable at this point. The call is bogus and should be removed, I think. > On the > other hand it is regression free... Well, finalization is new in 4.9, and Dominique has argued that the test suite may not have sufficient coverage yet. The absence of regressions may not be enough to conclude that the patch is correct. But after all I think that the patch should not hurt. After giving it some second thoughts, the only alternative I could see is this: It also gets rid of the ICE, but I haven't regtested it yet. Does this look better to you than the original patch? (It might give duplicate error messages in some cases?) Cheers, Janus Index: gcc/fortran/resolve.c =================================================================== --- gcc/fortran/resolve.c (revision 207485) +++ gcc/fortran/resolve.c (working copy) @@ -11224,13 +11224,6 @@ gfc_resolve_finalizers (gfc_symbol* derived) gfc_finalizer* i; int my_rank; - /* Skip this finalizer if we already resolved it. */ - if (list->proc_tree) - { - prev_link = &(list->next); - continue; - } - /* Check this exists and is a SUBROUTINE. */ if (!list->proc_sym->attr.subroutine) {