> On Oct 4, 2021, at 1:44 AM, Richard Biener <rguent...@suse.de> wrote: > > On Fri, 1 Oct 2021, Qing Zhao wrote: > >> >> >>> On Oct 1, 2021, at 10:33 AM, Jason Merrill <ja...@redhat.com> wrote: >>> >>> On 10/1/21 10:54, Qing Zhao wrote: >>>>> On Sep 30, 2021, at 2:31 PM, Jason Merrill <ja...@redhat.com> wrote: >>>>> >>>>> On 9/30/21 11:42, Qing Zhao wrote: >>>>>>> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguent...@suse.de> 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()::<lambda()> (null) >>>>>>>>> ;; enabled by -tree-original >>>>>>>>> >>>>>>>>> { >>>>>>>>> const struct A * const this [value-expr: &__closure->__this]; >>>>>>>>> const struct A * const this [value-expr: &__closure->__this]; >>>>>>>>> return <retval> = (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. >> >> Does this mean that in middle end, especially in gimplification phase, there >> is Not a simple way to determine whether a variable is a proxy variable? >>> >>>> 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. >> >> Still a little confuse..., do you mean, even for VAL_DECLS with >> DECL_VALUE_EXPR that were created by FE, we cannot guarantee they have been >> initialized? >> >> What did you mean by “the place to deal with that is at there 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. >> >> Yes, we can do that. >> >> However, the major thing I need to make sure is: >> >> can we guarantee that for All the VAL_DECLS with DECL_VALUE_EXPR created >> by FE are initialized already? > > I think so.
Okay. Will fix this bug based on this. Thanks. Qing > > Richard.
