From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aserp2130.oracle.com (aserp2130.oracle.com [141.146.126.79]) by sourceware.org (Postfix) with ESMTPS id 01ECC384A029 for ; Mon, 26 Apr 2021 20:02:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 01ECC384A029 Received: from pps.filterd (aserp2130.oracle.com [127.0.0.1]) by aserp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 13QJsfJK119600; Mon, 26 Apr 2021 20:02:54 GMT Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by aserp2130.oracle.com with ESMTP id 385afpuafe-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 26 Apr 2021 20:02:54 +0000 Received: from pps.filterd (userp3020.oracle.com [127.0.0.1]) by userp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 13QJuKjl082525; Mon, 26 Apr 2021 20:02:53 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userp3020.oracle.com with ESMTP id 384w3s1u7h-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 26 Apr 2021 20:02:53 +0000 Received: from abhmp0015.oracle.com (abhmp0015.oracle.com [141.146.116.21]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 13QK2q82020716; Mon, 26 Apr 2021 20:02:52 GMT Received: from dhcp-10-154-186-143.vpn.oracle.com (/10.154.186.143) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 26 Apr 2021 20:02:52 +0000 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: Re: [patch for gcc12 stage1][version 2] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc From: Qing Zhao In-Reply-To: Date: Mon, 26 Apr 2021 15:02:51 -0500 Cc: Richard Biener , Kees Cook , Gcc-patches Qing Zhao via Content-Transfer-Encoding: quoted-printable Message-Id: References: <0CE28536-176B-44E1-BDC6-3942739B829C@oracle.com> <0DDDEAB4-AA58-4F98-9526-8F63AEAC3D3D@oracle.com> To: Richard Sandiford X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9966 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 malwarescore=0 mlxscore=0 mlxlogscore=999 bulkscore=0 phishscore=0 spamscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104060000 definitions=main-2104260152 X-Proofpoint-ORIG-GUID: OBJDMyXqVOT4CWfDUQpis_m-5I3AgwKv X-Proofpoint-GUID: OBJDMyXqVOT4CWfDUQpis_m-5I3AgwKv X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9966 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 phishscore=0 clxscore=1015 suspectscore=0 lowpriorityscore=0 mlxlogscore=999 mlxscore=0 adultscore=0 malwarescore=0 impostorscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104060000 definitions=main-2104260152 X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_PASS, SPF_PASS, TXREP, UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Mon, 26 Apr 2021 20:03:02 -0000 > On Apr 26, 2021, at 12:47 PM, Richard Sandiford = wrote: >=20 > Qing Zhao writes: >>>> @@ -1831,6 +2000,17 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq = *seq_p) >>>> as they may contain a label address. */ >>>> walk_tree (&init, force_labels_r, NULL, NULL); >>>> } >>>> + /* When there is no explicit initializer, if the user = requested, >>>> + We should insert an artifical initializer for this automatic >>>> + variable for non vla variables. */ >>>=20 >>> I think we should explain why we can skip VLAs here. >>=20 >> VLA is handled in another place already, it should be initialized = with calls to memset/memcpy. >=20 > Yeah, what I meant here was that the comment should explain the > difference between the handling of VLAs and non-VLAs. It's fairly > obvious when reading the patch, but it won't be as obvious once the > patch is applied. Okay, I see, will update the comments. >=20 >>>> + children are to be processed. TOP_OFFSET is the offset of the = processed >>>> + subtree which has to be subtracted from offsets of individual = accesses to >>>> + get corresponding offsets for AGG. GSI is a statement iterator = used to place >>>> + the new statements. */ >>>> +static void >>>> +generate_subtree_deferred_init (struct access *access, tree agg, >>>> + enum auto_init_type init_type, >>>> + HOST_WIDE_INT top_offset, >>>> + gimple_stmt_iterator *gsi, >>>> + location_t loc) >>>> +{ >>>> + do >>>> + { >>>> + if (access->grp_to_be_replaced) >>>> + { >>>> + tree repl =3D get_access_replacement (access); >>>> + tree init_type_node >>>> + =3D build_int_cst (integer_type_node, (int) init_type); >>>> + gimple *call =3D gimple_build_call_internal = (IFN_DEFERRED_INIT, 2, >>>> + repl, = init_type_node); >>>> + gimple_call_set_lhs (call, repl); >>>=20 >>> AFAICT =E2=80=9Caccess=E2=80=9D is specifically for the lhs of the = original call. >>> So there seems to be an implicit assumption here that the lhs of the >>> original call is the same as the first argument of the original = call. >>> Is that guaranteed/required? >>=20 >> For call to DEFFERED_INIT, yes, this is guaranteed. >=20 > OK, in that case=E2=80=A6 >=20 >>> If so, I think it's something that >>> tree-cfg.c should check. It might also be worth having an assertion >>> in sra_modify_deferred_init. >> I can definitely add an assertion to make sure this. >=20 > =E2=80=A6I think we need the tree-cfg.c check too. Having the check = there > ensures that the invariant is maintained throughout gimple. Okay, will add check in tree-cfg.c too. >=20 >>>> + gimple_set_location (call, loc); >>>> + >>>> + sra_stats.subtree_deferred_init++; >>>> + } >>>> + else if (access->grp_to_be_debug_replaced) >>>> + { >>>> + /* FIXME, this part might have some issue. */ >>>> + tree drhs =3D build_debug_ref_for_model (loc, agg, >>>> + access->offset - = top_offset, >>>> + access); >>>> + gdebug *ds =3D gimple_build_debug_bind (get_access_replacement = (access), >>>> + drhs, gsi_stmt (*gsi)); >>>> + gsi_insert_before (gsi, ds, GSI_SAME_STMT); >>>=20 >>> Would be good to fix the FIXME :-) >>=20 >> This is the part I am not very sure, so I added the FIXME in order to = get more review and suggestion >> to make sure it. -:) >>>=20 >>> I guess the thing we need to decide here is whether = -ftrivial-auto-var-init >>> should affect debug-only constructs too. >>=20 >> Where can I get more details on Debug-only constructs ? >=20 > What I meant by =E2=80=9Cdebug-only construct=E2=80=9D is a piece of = source-level data > (in this case a field of an aggregate) that has been optimised out of > the executable code but still exists in debug stmts. Okay, I see now. Then, we should handle this case too, I think. > AIUI that's what > the code above is handling. >=20 >>>> @@ -4863,6 +4863,29 @@ find_func_aliases_for_builtin_call (struct = function *fn, gcall *t) >>>> return false; >>>> } >>>>=20 >>>> +static void >>>> +find_func_aliases_for_deferred_init (gcall *t) >>>> +{ >>>> + tree lhsop =3D gimple_call_lhs (t); >>>> + enum auto_init_type init_type >>>> + =3D (enum auto_init_type) TREE_INT_CST_LOW (gimple_call_arg = (t, 1)); >>>> + auto_vec lhsc; >>>> + auto_vec rhsc; >>>> + struct constraint_expr temp; >>>> + >>>> + get_constraint_for (lhsop, &lhsc); >>>> + if (init_type =3D=3D AUTO_INIT_ZERO && = flag_delete_null_pointer_checks) >>>> + temp.var =3D nothing_id; >>>> + else >>>> + temp.var =3D nonlocal_id; >>>> + temp.type =3D ADDRESSOF; >>>> + temp.offset =3D 0; >>>> + rhsc.safe_push (temp); >>>> + >>>> + process_all_all_constraints (lhsc, rhsc); >>>> + return; >>>> +} >>>> + >>>=20 >>> What's the reasoning behind doing it like this? AFAICT the result >>> of the call doesn't validly alias anything, regardless of the init = type. >>> Even if there happens to be a valid decl at the address given by the >>> chosen fill pattern, there's no expectation that accesses to that >>> decl will be coherent wrt accesses via the result of the call. >>=20 >> So, you mean the above change will not have any impact at all? >=20 > Stepping back a bit first: why did you add the code? What case > it is trying to handle? Perhaps I misunderstood the motivation. >=20 > In the above reply I meant more that the code seems unnecessarily > pessimistic. It looks like it's based on: >=20 > /* x =3D integer is all glommed to a single variable, which doesn't > point to anything by itself. That is, of course, unless it is an > integer constant being treated as a pointer, in which case, we > will return that this is really the addressof anything. This > happens below, since it will fall into the default case. The only > case we know something about an integer treated like a pointer is > when it is the NULL pointer, and then we just say it points to > NULL. >=20 > Do not do that if -fno-delete-null-pointer-checks though, because > in that case *NULL does not fail, so it _should_ alias *anything. > It is not worth adding a new option or renaming the existing one, > since this case is relatively obscure. */ > if ((TREE_CODE (t) =3D=3D INTEGER_CST > && integer_zerop (t)) > /* The only valid CONSTRUCTORs in gimple with pointer typed > elements are zero-initializer. But in IPA mode we also > process global initializers, so verify at least. */ > || (TREE_CODE (t) =3D=3D CONSTRUCTOR > && CONSTRUCTOR_NELTS (t) =3D=3D 0)) > { > if (flag_delete_null_pointer_checks) > temp.var =3D nothing_id; > else > temp.var =3D nonlocal_id; > temp.type =3D ADDRESSOF; > temp.offset =3D 0; > results->safe_push (temp); > return; > } >=20 > But if the user writes: >=20 > intptr_t x =3D 0; > *(int *)x =3D 42; >=20 > and compiles with -fno-delete-null-pointer-checks, then we have to > assume that the code is valid and is accessing a global decl that the > user knows is at address 0. So in that case we need the &nonlocal_id > constraint. >=20 > But even with the new option, I don't think: >=20 > intptr_t x; > *(int *)x =3D 42; >=20 > is well-defined in the same way. Maybe there's an argument that using > -fno-delete-null-pointer-checks is such a niche case that we might as > well treat it as though it were well-defined. That'd feel to me like > a half-measure though. It comes back to Richard Smith's argument > that we shouldn't try to create a new dialect of C/C++. We're > explicitly not trying to make: >=20 > intptr_t x; >=20 > and >=20 > intptr_t x =3D 0; >=20 > equivalent in all respects. And if we were trying to make them > equivalent, we'd need to do much more than this. >=20 > The same applies to the pattern case. If =E2=80=9Cx=E2=80=9D is = initialised to a pattern > that happens to point to a real decl, we don't have to preserve the > order of accesses to the decl wrt accesses to =E2=80=9C*x=E2=80=9D = (especially since > we're hoping that =E2=80=9C*x=E2=80=9D will trap). >=20 > I think for aliasing purposes, the .DEFERRED_INIT return value is = still > analogous to an undefined SSA name, even though we will later generate > code to initialise it. Okay, I see now. Will delete this part from the code. >=20 >>> In fact it might be simpler to have something like: >>>=20 >>> if (POINTER_TYPE_P (type) && TYPE_PRECISION (type) < 64) >>> return build_int_cstu (type, 0xAA); >>>=20 >>> if (INTEGRAL_TYPE_P (type) || POINTER_TYPE_P (type)) >>> ...the wi::from_buffer thing above...; >>=20 >> Okay. >>>=20 >>> I'm not sure if this makes sense for NULLPTR_TYPE. >>=20 >> Is there a good testing case in gcc test suite about NULLPTR_TYPE = that I can take a look? >=20 > I'm guessing you'll need to construct a new one to exercise this code = path. Will do this. >=20 >>>> + case RECORD_TYPE: >>>> + { >>>> + tree field; >>>> + tree field_value; >>>> + vec *v =3D NULL; >>>> + for (field=3D TYPE_FIELDS (type); field; field =3D DECL_CHAIN = (field)) >>>> + { >>>> + if (TREE_CODE (field) !=3D FIELD_DECL) >>>> + continue; >>>> + /* if the field is a variable length array, it should be the = last >>>> + field of the record, and no need to initialize. */ >>>=20 >>> Why doesn't it need to be initialized though? >>=20 >> My understanding is, the compiler will not allocate memory for the = latest field of the record if its a VLA, it=E2=80=99s the user=E2=80=99s = responsibility to allocate=20 >> Memory for it. Therefore, compiler doesn=E2=80=99t need to = initialize it. >=20 > Ah, OK. This sounds like the behaviour for flexible array members = rather > than VLA members. Oh, Yes. My bad to use a wrong concept to confuse you. > E.g. GCC allows: >=20 > void bar (int *); > int > foo (int a) > { > // VLA in struct > struct { int x[a]; } foo; > bar (foo.x); > } >=20 > In this case the foo.x really does have =E2=80=9Ca=E2=80=9D elements = that would > need to be initialised. >=20 > I think the case you're talking about is: >=20 > void bar (int *); > int > foo (int a) > { > // struct ending in a flexible array > struct { int prefix; int x[]; } foo; > bar (foo.x); > } >=20 > where foo.x has zero elements. Yes.=20 Then, that part of the code currently is okay? >=20 >>>> @@ -11950,6 +12088,72 @@ lower_bound_in_type (tree outer, tree = inner) >>>> } >>>> } >>>>=20 >>>> +/* Returns true when the given TYPE has padding inside it. >>>> + return false otherwise. */ >>>> +bool >>>> +type_has_padding (tree type) >>>=20 >>> Would it be possible to reuse __builtin_clear_padding here? >>=20 >> Not sure, where can I get more details on __builtin_clear_padding? I = can study a little bit more on this to make sure this. >=20 > It's documented in doc/extend.texi. Okay, will check on that. Thanks a lot. Qing >=20 > Thanks, > Richard