From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id A56E83838000 for ; Mon, 26 Apr 2021 17:47:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A56E83838000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3E1BA31B; Mon, 26 Apr 2021 10:47:17 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 84AD73F73B; Mon, 26 Apr 2021 10:47:16 -0700 (PDT) From: Richard Sandiford To: Qing Zhao Mail-Followup-To: Qing Zhao , Richard Biener , Kees Cook , Gcc-patches Qing Zhao via , richard.sandiford@arm.com Cc: Richard Biener , Kees Cook , Gcc-patches Qing Zhao via Subject: Re: [patch for gcc12 stage1][version 2] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc References: <0CE28536-176B-44E1-BDC6-3942739B829C@oracle.com> <0DDDEAB4-AA58-4F98-9526-8F63AEAC3D3D@oracle.com> Date: Mon, 26 Apr 2021 18:47:15 +0100 In-Reply-To: <0DDDEAB4-AA58-4F98-9526-8F63AEAC3D3D@oracle.com> (Qing Zhao's message of "Mon, 26 Apr 2021 10:09:15 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.6 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP 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 17:47:19 -0000 Qing Zhao writes: >>> @@ -1831,6 +2000,17 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *se= q_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. > > VLA is handled in another place already, it should be initialized with ca= lls to memset/memcpy. 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. >>> + children are to be processed. TOP_OFFSET is the offset of the pro= cessed >>> + subtree which has to be subtracted from offsets of individual acces= ses to >>> + get corresponding offsets for AGG. GSI is a statement iterator use= d 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 origi= nal 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? > > For call to DEFFERED_INIT, yes, this is guaranteed. OK, in that case=E2=80=A6 >> 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. =E2=80=A6I think we need the tree-cfg.c check too. Having the check there ensures that the invariant is maintained throughout gimple. >>> + 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 (acc= ess), >>> + drhs, gsi_stmt (*gsi)); >>> + gsi_insert_before (gsi, ds, GSI_SAME_STMT); >>=20 >> Would be good to fix the FIXME :-) > > 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-i= nit >> should affect debug-only constructs too. > > Where can I get more details on Debug-only constructs ? 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. AIUI that's what the code above is handling. >>> @@ -4863,6 +4863,29 @@ find_func_aliases_for_builtin_call (struct funct= ion *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_chec= ks) >>> + 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. > > So, you mean the above change will not have any impact at all? Stepping back a bit first: why did you add the code? What case it is trying to handle? Perhaps I misunderstood the motivation. In the above reply I meant more that the code seems unnecessarily pessimistic. It looks like it's based on: /* 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. 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; } But if the user writes: intptr_t x =3D 0; *(int *)x =3D 42; 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. But even with the new option, I don't think: intptr_t x; *(int *)x =3D 42; 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: intptr_t x; and intptr_t x =3D 0; equivalent in all respects. And if we were trying to make them equivalent, we'd need to do much more than this. The same applies to the pattern case. If =E2=80=9Cx=E2=80=9D is initialise= d 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 (especia= lly since we're hoping that =E2=80=9C*x=E2=80=9D will trap). 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. >> 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...; > > Okay. >>=20 >> I'm not sure if this makes sense for NULLPTR_TYPE. > > Is there a good testing case in gcc test suite about NULLPTR_TYPE that I = can take a look? I'm guessing you'll need to construct a new one to exercise this code path. >>> + 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? > > 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 responsi= bility to allocate=20 > Memory for it. Therefore, compiler doesn=E2=80=99t need to initialize i= t. Ah, OK. This sounds like the behaviour for flexible array members rather than VLA members. E.g. GCC allows: void bar (int *); int foo (int a) { // VLA in struct struct { int x[a]; } foo; bar (foo.x); } In this case the foo.x really does have =E2=80=9Ca=E2=80=9D elements that w= ould need to be initialised. I think the case you're talking about is: void bar (int *); int foo (int a) { // struct ending in a flexible array struct { int prefix; int x[]; } foo; bar (foo.x); } where foo.x has zero elements. >>> @@ -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? > > 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. It's documented in doc/extend.texi. Thanks, Richard