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. Richard.
