From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x131.google.com (mail-lf1-x131.google.com [IPv6:2a00:1450:4864:20::131]) by sourceware.org (Postfix) with ESMTPS id 8F1B8394D8E7 for ; Tue, 23 May 2023 08:18:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8F1B8394D8E7 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lf1-x131.google.com with SMTP id 2adb3069b0e04-4f3b5881734so3980094e87.0 for ; Tue, 23 May 2023 01:18:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684829928; x=1687421928; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=mt/DP8VETO0t/fVCpOnIxpx8HtHbmQi3Ca0hCwiOmvw=; b=DjFENlcxScgtQWD0KfsKxva5/aGGiWWxxLATvTevzlYxQYwNjaLxjFYMxU83xU1b1R a46kP9inSKJJ/y5JzXc56fTyEklW/JECWnki7J6R/AOGFrxIgH8fUDd8liu43G10VFu8 A+m+IK+beG4IuLVAnShK7+Dv055rTzigqIkX/5SFhbeKKsTWUhq8pcTt9WWowMkT629g ZE4DdRM8qvLepxwW4NZJTCDSAa3uBlqvZrqFy+3/5TO52vuH2N7wagJJ+70QqgRp5uAW GCrkDObvEX+OQAbjkdDEEIkdOFX9GuIS2OpuDlNTGJmIrhmpn2bCJGv2Xn7xuNnxLx4m Vbpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684829928; x=1687421928; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=mt/DP8VETO0t/fVCpOnIxpx8HtHbmQi3Ca0hCwiOmvw=; b=boZPXaXyCb6Lz4uLBY1HO8ibQ+d4bi9GXjj18ly0SaAJZ2Urf/Ij92yC6kmnG6LSsT PpyutzzM8Pp0O6NScdXJY/oNhAfeSk1KYWPXDFDeME2GqffV6IApFK7HtycPuLmICe0m JsAHHvTk/oJKKJymcN7ryeXu7unQlfZ8V1YUlcPNPcFTkEnP6qnVLoCOp9p3aVoBIQjW wGvfyxS6W/tDgzkpxNlWBOV+vIWD96Chm+V2cKVauhr4dcTqsAk7+u4MFikazxjkCWVr deIt1UDa/gCXoWPqtbLCX8kH7gg8NBYkQaCaMTH9DoRgmx3/fUc9rPZE7F26f09DKWoQ Xx4w== X-Gm-Message-State: AC+VfDzBk7AmNS8I4jKuN3E4Fm6EvPjgWTxEhi0GcRw6Rg/Uogo59GCi /0b/wYnAd6GYd98rez9+FMTWDKIqlsGGs2gBdAg= X-Google-Smtp-Source: ACHHUZ6oI2+yZwR22r8YuZoKcGCfdNukjwss5dM4fwosjUZRkrrTlBBL26ZKXXTk2BCByokzcLpByAin169kMkrjXv8= X-Received: by 2002:a2e:3c0d:0:b0:2ac:826e:c5a with SMTP id j13-20020a2e3c0d000000b002ac826e0c5amr5353307lja.1.1684829927597; Tue, 23 May 2023 01:18:47 -0700 (PDT) MIME-Version: 1.0 References: <1e82fc2dcce84019104d693420daf601c03dcbf8.camel@tugraz.at> In-Reply-To: <1e82fc2dcce84019104d693420daf601c03dcbf8.camel@tugraz.at> From: Richard Biener Date: Tue, 23 May 2023 10:18:34 +0200 Message-ID: Subject: Re: [C PATCH v3] Fix ICEs related to VM types in C 2/2 To: Martin Uecker Cc: gcc-patches@gcc.gnu.org, Joseph Myers , Richard Biener 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,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, May 23, 2023 at 8:24=E2=80=AFAM Martin Uecker wr= ote: > > Am Dienstag, dem 23.05.2023 um 08:13 +0200 schrieb Richard Biener: > > On Mon, May 22, 2023 at 7:24=E2=80=AFPM Martin Uecker via Gcc-patches > > wrote: > > > > > > > > > > > > This version contains the middle-end changes for PR109450 > > > and test cases as before. The main middle-end change is that > > > we use gimplify_type_sizes also for parameters and remove > > > the special code that also walked into pointers (which is > > > incorrect). > > > > > > In addition, in the C FE this patch now also adds DECL_EXPR > > > for vm-types which are pointed-to by parameters declared > > > as arrays. The new function created contains the exact > > > code previously used only for regular pointers, and is > > > now also called for parameters declared as arrays. > > > > > > > > > Martin > > > > > > > > > > > > > > > > > > > > > > > > Fix ICEs related to VM types in C 2/2 [PR109450] > > > > > > Size expressions were sometimes lost and not gimplified correctly= , > > > leading to ICEs and incorrect evaluation order. Fix this by 1) n= ot > > > recursing pointers when gimplifying parameters in the middle-end > > > (the code is merged with gimplify_type_sizes), which is incorrect > > > because it might access variables declared later for incomplete > > > structs, and 2) adding a decl expr for variably-modified arrays > > > that are pointed to by parameters declared as arrays. > > > > > > PR c/109450 > > > > > > gcc/ > > > * c/c-decl.cc (add_decl_expr): New function. > > > (grokdeclarator): Add decl expr for size expression in > > > types pointed to by parameters declared as arrays. > > > * function.cc (gimplify_parm_type): Remove function. > > > (gimplify_parameters): Call gimplify_parm_sizes. > > > * gimplify.cc (gimplify_type_sizes): Make function static= . > > > (gimplify_parm_sizes): New function. > > > > > > gcc/testsuite/ > > > * gcc.dg/pr109450-1.c: New test. > > > * gcc.dg/pr109450-2.c: New test. > > > * gcc.dg/vla-26.c: New test. > > > > > > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc > > > index 494d3cf1747..c35347734b2 100644 > > > --- a/gcc/c/c-decl.cc > > > +++ b/gcc/c/c-decl.cc > > > @@ -6490,6 +6490,55 @@ smallest_type_quals_location (const location_t= *locations, > > > return loc; > > > } > > > > > > + > > > +/* We attach an artificial TYPE_DECL to pointed-to type > > > + and arrange for it to be included in a DECL_EXPR. This > > > + forces the sizes evaluation at a safe point and ensures it > > > + is not deferred until e.g. within a deeper conditional context. > > > + > > > + PARM contexts have no enclosing statement list that > > > + can hold the DECL_EXPR, so we need to use a BIND_EXPR > > > + instead, and add it to the list of expressions that > > > + need to be evaluated. > > > + > > > + TYPENAME contexts do have an enclosing statement list, > > > + but it would be incorrect to use it, as the size should > > > + only be evaluated if the containing expression is > > > + evaluated. We might also be in the middle of an > > > + expression with side effects on the pointed-to type size > > > + "arguments" prior to the pointer declaration point and > > > + the fake TYPE_DECL in the enclosing context would force > > > + the size evaluation prior to the side effects. We therefore > > > + use BIND_EXPRs in TYPENAME contexts too. */ > > > +static void > > > +add_decl_expr(location_t loc, enum decl_context decl_context, tree t= ype, tree *expr) > > > +{ > > > + tree bind =3D NULL_TREE; > > > + if (decl_context =3D=3D TYPENAME || decl_context =3D=3D PARM || de= cl_context =3D=3D FIELD) > > > + { > > > + bind =3D build3 (BIND_EXPR, void_type_node, NULL_TREE, NULL_TR= EE, NULL_TREE); > > > + TREE_SIDE_EFFECTS (bind) =3D 1; > > > + BIND_EXPR_BODY (bind) =3D push_stmt_list (); > > > + push_scope (); > > > + } > > > + > > > + tree decl =3D build_decl (loc, TYPE_DECL, NULL_TREE, type); > > > + pushdecl (decl); > > > + DECL_ARTIFICIAL (decl) =3D 1; > > > + add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DECL_EXPR, decl= )); > > > + TYPE_NAME (type) =3D decl; > > > + > > > + if (bind) > > > + { > > > + pop_scope (); > > > + BIND_EXPR_BODY (bind) =3D pop_stmt_list (BIND_EXPR_BODY (bind)= ); > > > + if (*expr) > > > + *expr =3D build2 (COMPOUND_EXPR, void_type_node, *expr, bind)= ; > > > + else > > > + *expr =3D bind; > > > + } > > > +} > > > + > > > /* Given declspecs and a declarator, > > > determine the name and type of the object declared > > > and construct a ..._DECL node for it. > > > @@ -7474,58 +7523,9 @@ grokdeclarator (const struct c_declarator *dec= larator, > > > > > > This is expected to happen automatically when the poin= ted-to > > > type has a name/declaration of it's own, but special a= ttention > > > - is required if the type is anonymous. > > > - > > > - We attach an artificial TYPE_DECL to such pointed-to t= ype > > > - and arrange for it to be included in a DECL_EXPR. Thi= s > > > - forces the sizes evaluation at a safe point and ensure= s it > > > - is not deferred until e.g. within a deeper conditional= context. > > > - > > > - PARM contexts have no enclosing statement list that > > > - can hold the DECL_EXPR, so we need to use a BIND_EXPR > > > - instead, and add it to the list of expressions that > > > - need to be evaluated. > > > - > > > - TYPENAME contexts do have an enclosing statement list, > > > - but it would be incorrect to use it, as the size shoul= d > > > - only be evaluated if the containing expression is > > > - evaluated. We might also be in the middle of an > > > - expression with side effects on the pointed-to type si= ze > > > - "arguments" prior to the pointer declaration point and > > > - the fake TYPE_DECL in the enclosing context would forc= e > > > - the size evaluation prior to the side effects. We the= refore > > > - use BIND_EXPRs in TYPENAME contexts too. */ > > > - if (!TYPE_NAME (type) > > > - && c_type_variably_modified_p (type)) > > > - { > > > - tree bind =3D NULL_TREE; > > > - if (decl_context =3D=3D TYPENAME || decl_context =3D= =3D PARM > > > - || decl_context =3D=3D FIELD) > > > - { > > > - bind =3D build3 (BIND_EXPR, void_type_node, NULL_= TREE, > > > - NULL_TREE, NULL_TREE); > > > - TREE_SIDE_EFFECTS (bind) =3D 1; > > > - BIND_EXPR_BODY (bind) =3D push_stmt_list (); > > > - push_scope (); > > > - } > > > - tree decl =3D build_decl (loc, TYPE_DECL, NULL_TREE, = type); > > > - pushdecl (decl); > > > - DECL_ARTIFICIAL (decl) =3D 1; > > > - add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DE= CL_EXPR, decl)); > > > - TYPE_NAME (type) =3D decl; > > > - > > > - if (bind) > > > - { > > > - pop_scope (); > > > - BIND_EXPR_BODY (bind) > > > - =3D pop_stmt_list (BIND_EXPR_BODY (bind)); > > > - if (*expr) > > > - *expr =3D build2 (COMPOUND_EXPR, void_type_node= , *expr, > > > - bind); > > > - else > > > - *expr =3D bind; > > > - } > > > - } > > > + is required if the type is anonymous. */ > > > + if (!TYPE_NAME (type) && c_type_variably_modified_p (type= )) > > > + add_decl_expr (loc, decl_context, type, expr); > > > > > > type =3D c_build_pointer_type (type); > > > > > > @@ -7787,6 +7787,11 @@ grokdeclarator (const struct c_declarator *dec= larator, > > > if (type_quals) > > > type =3D c_build_qualified_type (type, type_quals, orig= _qual_type, > > > orig_qual_indirect); > > > + > > > + /* The pointed-to type may need a decl expr (see above). = */ > > > + if (!TYPE_NAME (type) && c_type_variably_modified_p (type= )) > > > + add_decl_expr (loc, decl_context, type, expr); > > > + > > > type =3D c_build_pointer_type (type); > > > type_quals =3D array_ptr_quals; > > > if (type_quals) > > > diff --git a/gcc/function.cc b/gcc/function.cc > > > index f0ae641512d..5699b9d495d 100644 > > > --- a/gcc/function.cc > > > +++ b/gcc/function.cc > > > @@ -3872,30 +3872,6 @@ assign_parms (tree fndecl) > > > } > > > } > > > > > > -/* A subroutine of gimplify_parameters, invoked via walk_tree. > > > - For all seen types, gimplify their sizes. */ > > > - > > > -static tree > > > -gimplify_parm_type (tree *tp, int *walk_subtrees, void *data) > > > -{ > > > - tree t =3D *tp; > > > - > > > - *walk_subtrees =3D 0; > > > - if (TYPE_P (t)) > > > - { > > > - if (POINTER_TYPE_P (t)) > > > - *walk_subtrees =3D 1; > > > - else if (TYPE_SIZE (t) && !TREE_CONSTANT (TYPE_SIZE (t)) > > > - && !TYPE_SIZES_GIMPLIFIED (t)) > > > - { > > > - gimplify_type_sizes (t, (gimple_seq *) data); > > > - *walk_subtrees =3D 1; > > > - } > > > - } > > > - > > > - return NULL; > > > -} > > > - > > > /* Gimplify the parameter list for current_function_decl. This invo= lves > > > evaluating SAVE_EXPRs of variable sized parameters and generating= code > > > to implement callee-copies reference parameters. Returns a seque= nce of > > > @@ -3931,14 +3907,7 @@ gimplify_parameters (gimple_seq *cleanup) > > > SAVE_EXPRs (amongst others) onto a pending sizes list. This > > > turned out to be less than manageable in the gimple world. > > > Now we have to hunt them down ourselves. */ > > > - walk_tree_without_duplicates (&data.arg.type, > > > - gimplify_parm_type, &stmts); > > > - > > > - if (TREE_CODE (DECL_SIZE_UNIT (parm)) !=3D INTEGER_CST) > > > - { > > > - gimplify_one_sizepos (&DECL_SIZE (parm), &stmts); > > > - gimplify_one_sizepos (&DECL_SIZE_UNIT (parm), &stmts); > > > - } > > > + gimplify_parm_sizes (parm, &stmts); > > > > > > if (data.arg.pass_by_reference) > > > { > > > diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc > > > index d0d16a24820..fd990c328eb 100644 > > > --- a/gcc/gimplify.cc > > > +++ b/gcc/gimplify.cc > > > @@ -242,6 +242,7 @@ static struct gimplify_omp_ctx *gimplify_omp_ctxp= ; > > > static bool in_omp_construct; > > > > > > /* Forward declaration. */ > > > +static void gimplify_type_sizes (tree type, gimple_seq *list_p); > > > static enum gimplify_status gimplify_compound_expr (tree *, gimple_s= eq *, bool); > > > static hash_map *oacc_declare_returns; > > > static enum gimplify_status gimplify_expr (tree *, gimple_seq *, gim= ple_seq *, > > > @@ -17425,7 +17426,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_= p, gimple_seq *post_p, > > > /* Look through TYPE for variable-sized objects and gimplify each su= ch > > > size that we find. Add to LIST_P any statements generated. */ > > > > > > -void > > > +static void > > > gimplify_type_sizes (tree type, gimple_seq *list_p) > > > { > > > if (type =3D=3D NULL || type =3D=3D error_mark_node) > > > @@ -17533,6 +17534,21 @@ gimplify_type_sizes (tree type, gimple_seq *= list_p) > > > } > > > } > > > > > > +/* Gimplify sizes in parameter declarations. */ > > > + > > > +void > > > +gimplify_parm_sizes (tree parm, gimple_seq *list_p) > > > > Can you instead inline this at the single caller in gimplify_parameters= ? > > It looks like both gimplify_type_sizes and gimplify_one_sizepos are > > already exported. So just add the missing gimplify_type_sizes () call > > at the single call site. > > > > The middle-end changes are OK with that change. > > Thanks Richard! > > I did this in this way because this was the only outside use of > gimplify_type_sizes. Then one can unexport gimplify_type_size. > In fact, it is declared static (see above) and removed from the > header (see below) in the patch. I thought this is better logical > encapsulation. (BTW: gimplify_one_sizepos then has only > one remaining use in the Ada FE, so maybe we make a copy there > and also unexport it?). > > What do you think? I think any API streamlining should be done separately and as long as gimplify_one_sizepos is still exported it's odd to focus on gimplify_type_sizes, looking at the Ada FE use it doesn't look like that can be removed. Richard. > Martin > > > Thanks, > > Richard. > > > > > +{ > > > + gimplify_type_sizes (TREE_TYPE (parm), list_p); > > > + > > > + if (TREE_CODE (DECL_SIZE_UNIT (parm)) !=3D INTEGER_CST) > > > + { > > > + gimplify_one_sizepos (&DECL_SIZE (parm), list_p); > > > + gimplify_one_sizepos (&DECL_SIZE_UNIT (parm), list_p); > > > + } > > > +} > > > + > > > + > > > /* A subroutine of gimplify_type_sizes to make sure that *EXPR_P, > > > a size or position, has had all of its SAVE_EXPRs evaluated. > > > We add any required statements to *STMT_P. */ > > > diff --git a/gcc/gimplify.h b/gcc/gimplify.h > > > index f4a3eea2606..17ea0580647 100644 > > > --- a/gcc/gimplify.h > > > +++ b/gcc/gimplify.h > > > @@ -78,7 +78,7 @@ extern enum gimplify_status gimplify_expr (tree *, = gimple_seq *, gimple_seq *, > > > > > > int omp_construct_selector_matches (enum tree_code *, int, int *); > > > > > > -extern void gimplify_type_sizes (tree, gimple_seq *); > > > +extern void gimplify_parm_sizes (tree, gimple_seq *); > > > extern void gimplify_one_sizepos (tree *, gimple_seq *); > > > extern gbind *gimplify_body (tree, bool); > > > extern enum gimplify_status gimplify_arg (tree *, gimple_seq *, loca= tion_t, > > > diff --git a/gcc/testsuite/gcc.dg/pr109450-1.c b/gcc/testsuite/gcc.dg= /pr109450-1.c > > > new file mode 100644 > > > index 00000000000..aec127f2afc > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.dg/pr109450-1.c > > > @@ -0,0 +1,21 @@ > > > +/* PR c/109450 > > > + * { dg-do run } > > > + * { dg-options "-std=3Dgnu99" } > > > + * */ > > > + > > > +int bar(int n, struct foo* x) /* { dg-warning "not be visible" } */ > > > +{ > > > + int a =3D n; > > > + struct foo { char buf[n++]; }* p =3D x; > > > + return a; > > > +} > > > + > > > +int main() > > > +{ > > > + if (1 !=3D bar(1, 0)) > > > + __builtin_abort(); > > > +} > > > + > > > + > > > + > > > + > > > diff --git a/gcc/testsuite/gcc.dg/pr109450-2.c b/gcc/testsuite/gcc.dg= /pr109450-2.c > > > new file mode 100644 > > > index 00000000000..06799f6df23 > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.dg/pr109450-2.c > > > @@ -0,0 +1,18 @@ > > > +/* PR c/109450 > > > + * { dg-do run } > > > + * { dg-options "-std=3Dgnu99" } > > > + * */ > > > + > > > +int bar(int n, struct foo *x) /* { dg-warning "not be visible" } */ > > > +{ > > > + int a =3D n; > > > + struct foo { char buf[a++]; }* p =3D x; > > > + return n =3D=3D a; > > > +} > > > + > > > +int main() > > > +{ > > > + if (bar(1, 0)) > > > + __builtin_abort(); > > > +} > > > + > > > diff --git a/gcc/testsuite/gcc.dg/vla-26.c b/gcc/testsuite/gcc.dg/vla= -26.c > > > new file mode 100644 > > > index 00000000000..5d2fa3e280a > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.dg/vla-26.c > > > @@ -0,0 +1,15 @@ > > > +/* { dg-do compile } */ > > > +/* { dg-options "-std=3Dc99 -O2" } */ > > > + > > > +void ed(int n, float s[3][n]) > > > +{ > > > + for (int i =3D 0; i < n; i++) > > > + s[1][i]; > > > +} > > > + > > > +void e(int n, float s[3][n]) > > > +{ > > > + ed(n, s); > > > +} > > > + > > > + > > > > > > > >