From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 880F13858406 for ; Fri, 1 Oct 2021 15:33:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 880F13858406 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-145-otZqPjyOM9qVOMP9ti4WIQ-1; Fri, 01 Oct 2021 11:33:31 -0400 X-MC-Unique: otZqPjyOM9qVOMP9ti4WIQ-1 Received: by mail-qt1-f200.google.com with SMTP id 100-20020aed30ed000000b002a6b3dc6465so14941273qtf.13 for ; Fri, 01 Oct 2021 08:33:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=sk2TXrBuwpzj6FurW7k+J1gGe6u32z/Y/+Ip7o6cUig=; b=BqXNtrI/Mvdvw0skfxXxao6jC5YEhXa/NG5DyZozqvZ562DnI7aTkqTj6C1HQphDMw F/c3Cwmf9PljtZhdcnhGF7rp/3/Oc8DzpxXBmBP4P5wi0dYqv7PvY+3xIQhnKuXkI9CL wcEqOw0CBWD7otu+QLfFQtBAGllfZByYG9AWrrMaZlbKU5Bs50BxSB51qbtUv3yn3udQ Bn3ZCPhbn3VWOssrRqOBbdVW1nweBtXqMEkgbwJ6tANK39M6PqUYu+6pLJR1gnF8Lwqt KiukTUA8NccZjJgi+jvolzKqcg5X6FXDMtxnyaN3WA0SQ6TY+2hLgM3Pn1SmPbyxBH29 5Oow== X-Gm-Message-State: AOAM532/GTe43RQC5/mLj45ks5TA4Shx0oAtSCWvBzIbw9NnTisJZ/wZ xObBvp9MW/mKmREjY15DM3ONRPaPMdIHHvDRtG8LCDqXkKgZKNCowXwmyJIt2yl/IXEs9pNNO+c z7JobBulDZIkb0nlOxA== X-Received: by 2002:ad4:4629:: with SMTP id x9mr9578274qvv.58.1633102411249; Fri, 01 Oct 2021 08:33:31 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz7ts7c81SNH24NNcUSYpjksI0F4tXPFwatEGhnHeNYCB/U6HyUOsuPv/ctsXt0gOGJTlUUnA== X-Received: by 2002:ad4:4629:: with SMTP id x9mr9578246qvv.58.1633102410939; Fri, 01 Oct 2021 08:33:30 -0700 (PDT) Received: from [192.168.1.149] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id 90sm3559683qte.89.2021.10.01.08.33.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 01 Oct 2021 08:33:30 -0700 (PDT) Message-ID: Date: Fri, 1 Oct 2021 11:33:29 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.2 Subject: Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init To: Qing Zhao Cc: Richard Biener , gcc-patches Nick Alcock via References: <788c04d7-6e87-4eff-decb-a13abf0b4058@redhat.com> <5q583245-3qq5-76p7-o1p4-312496os4140@fhfr.qr> <710E24B6-B845-49F0-B426-741905C48EA2@oracle.com> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Oct 2021 15:33:38 -0000 On 10/1/21 10:54, Qing Zhao wrote: > > >> On Sep 30, 2021, at 2:31 PM, Jason Merrill wrote: >> >> On 9/30/21 11:42, Qing Zhao wrote: >>>> On Sep 30, 2021, at 1:54 AM, Richard Biener wrote: >>>> >>>> On Thu, 30 Sep 2021, Jason Merrill wrote: >>>> >>>>> On 9/29/21 17:30, Qing Zhao wrote: >>>>>> Hi, >>>>>> >>>>>> PR102359 (ICE gimplification failed since r12-3433-ga25e0b5e6ac8a77a) >>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359 >>>>>> >>>>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY >>>>>> variable “this” in the following routine: (t.cpp.005t.original) >>>>>> >>>>>> ======= >>>>>> >>>>>> ;; Function A::foo():: (null) >>>>>> ;; enabled by -tree-original >>>>>> >>>>>> { >>>>>> const struct A * const this [value-expr: &__closure->__this]; >>>>>> const struct A * const this [value-expr: &__closure->__this]; >>>>>> return = (double) ((const struct A *) this)->a; >>>>>> } >>>>>> ======= >>>>>> >>>>>> However, in the above routine, “this” is NOT marked as READONLY, but its >>>>>> value-expr "&__closure->__this” is marked as READONLY. >>>>>> >>>>>> There are two major issues: >>>>>> >>>>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is >>>>>> marked as READONLY; >>>>>> 2. In the C++ FE, “this” should be marked as READONLY. >>>>>> >>>>>> The idea solution will be: >>>>>> >>>>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl); >>>>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true; >>>>>> >>>>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not? >>>>>> >>>>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in >>>>>> middle end: >>>>>> >>>>>> Let me know your comments or suggestions on this. >>>>>> >>>>>> Thanks a lot for the help. >>>>> >>>>> I'd think is_var_need_auto_init should be false for any variable with >>>>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming >>>>> objects that are initialized elsewhere. >>>> >>>> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to >>>> auto-init VLAs, otherwise I tend to agree - would we handle those >>>> when we see a DECL_EXPR then? >>> The current implementation is: >>> gimplify_decl_expr: >>> For each DECL_EXPR “decl” >>> If (VAR_P (decl) && !DECL_EXTERNAL (decl)) >>> { >>> if (is_vla (decl)) >>> gimplify_vla_decl (decl, …); /* existing handling: create a VALUE_EXPR for this vla decl*/ >>> … >>> if (has_explicit_init (decl)) >>> { >>> …; /* existing handling. */ >>> } >>> else if (is_var_need_auto_init (decl)) /*. New code. */ >>> { >>> gimple_add_init_for_auto_var (….); /* new code. */ >>> ... >>> } >>> } >>> Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization. >>> if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either. We will miss adding initializations for these decls. >>> So, I think that the current implementation is correct. >>> And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too. >>> Let me know your opinion on this. >> >> The problem with this test is not whether the 'this' proxy is marked READONLY, the problem is that you're trying to initialize lambda capture proxies at all; the lambda capture objects were already initialized when forming the closure object. So this test currently aborts with -ftrivial-auto-var-init=zero because you "initialize" the i capture field to 0 after it was previously initialized to 42: >> >> int main() >> { >> int i = 42; >> auto l = [=]() mutable { return i; }; >> if (l() != i) >> __builtin_abort (); >> } >> >> I believe the same issue applies to the proxy variables in coroutines that work much like lambdas. > So, how should the middle end determine that a variable is “proxy variable”? In the front end, is_capture_proxy will identify a lambda capture proxy variable. But that won't be true for the similar proxies used by coroutines. > Have all “proxy variables” been initialized by C++ FE already? Yes. >> You can't just assume that a VAR_DECL with DECL_VALUE_EXPR is uninitialized. > > So, all the VAR_DECLs with DECL_VALUE_EXPR (except the ones created by “gimplify_decl_expr”) are initialized by FE already? In general I'd expect them to refer to previously created objects which may or may not have been initialized, but if they haven't been, the place to deal with that is at their previous creation. >> Since there's already VLA handling in gimplify_decl_expr, you could remember whether you added DECL_VALUE_EXPR in that function, and only then do the initialization. > > Yes, if we can guarantee that all the VAR_DECLs with DECL_VALUE_EXPR created from FEs have been initialized already by FE, we can fix this issue as this way. Or more generally, check whether the argument to gimplify_decl_expr has DECL_VALUE_EXPR when we enter the function, and don't do the initialization in that case. Jason