> 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? thanks. Qing > > Jason
