From: Richard Biener <rguenther@suse.de>
To: Qing Zhao <qing.zhao@oracle.com>
Cc: Jakub Jelinek <jakub@redhat.com>,
Richard Sandiford <richard.sandiford@arm.com>,
Nick Alcock via Gcc-patches <gcc-patches@gcc.gnu.org>,
Kees Cook <keescook@chromium.org>
Subject: Re: [patch][version 6] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
Date: Mon, 16 Aug 2021 09:12:59 +0200 (CEST) [thread overview]
Message-ID: <nycvar.YFH.7.76.2108160911510.11781@zhemvz.fhfr.qr> (raw)
In-Reply-To: <28A70D41-58C6-49FF-BDA9-661F36A2D795@oracle.com>
On Wed, 11 Aug 2021, Qing Zhao wrote:
> Hi,
>
> I finally decided to take another approach to resolve this issue, it resolved all the potential issues with the “address taken” auto variable.
>
> The basic idea is to avoid generating the temporary variable in the beginning.
> As you mentioned, "The reason is that alt_reloc is memory (because it is address taken) and that GIMPLE says that register typed stores
> need to use a is_gimple_val RHS which the call is not.”
> In order to avoid generating the temporary variable for “address taken” auto variable, I updated the utility routine “is_gimple_val” as following:
>
> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
> index a2563a45c37d..d5ef1aef8cea 100644
> --- a/gcc/gimple-expr.c
> +++ b/gcc/gimple-expr.c
> @@ -787,8 +787,20 @@ is_gimple_reg (tree t)
> return !DECL_NOT_GIMPLE_REG_P (t);
> }
>
> +/* Return true if T is a call to .DEFERRED_INIT internal function. */
> +static bool
> +is_deferred_init_call (tree t)
> +{
> + if (TREE_CODE (t) == CALL_EXPR
> + && CALL_EXPR_IFN (t) == IFN_DEFERRED_INIT)
> + return true;
> + return false;
> +}
> +
>
> -/* Return true if T is a GIMPLE rvalue, i.e. an identifier or a constant. */
> +/* Return true if T is a GIMPLE rvalue, i.e. an identifier or a constant,
> + or a call to .DEFERRED_INIT internal function because the call to
> + .DEFERRED_INIT will eventually be expanded as a constant. */
>
> bool
> is_gimple_val (tree t)
> @@ -799,7 +811,8 @@ is_gimple_val (tree t)
> && !is_gimple_reg (t))
> return false;
>
> - return (is_gimple_variable (t) || is_gimple_min_invariant (t));
> + return (is_gimple_variable (t) || is_gimple_min_invariant (t)
> + || is_deferred_init_call (t));
> }
>
> With this change, the temporary variable will not be created for “address taken” auto variable, and uninitialized analysis does not need any change.
> Everything works well.
>
> And I believe that treating “call to .DEFERRED_INIT” as “is_gimple_val” is reasonable since this call actually is a constant.
>
> Let me know if you have any objection on this solution.
Yeah, I object to this solution.
Richard.
> thanks.
>
> Qing
>
> > On Aug 11, 2021, at 3:30 PM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> >
> > Hi,
> >
> > I met another issue for “address taken” auto variable, see below for details:
> >
> > **** the testing case: (gcc/testsuite/gcc.dg/uninit-16.c)
> >
> > int foo, bar;
> >
> > static
> > void decode_reloc(int reloc, int *is_alt)
> > {
> > if (reloc >= 20)
> > *is_alt = 1;
> > else if (reloc >= 10)
> > *is_alt = 0;
> > }
> >
> > void testfunc()
> > {
> > int alt_reloc;
> >
> > decode_reloc(foo, &alt_reloc);
> >
> > if (alt_reloc) /* { dg-warning "may be used uninitialized" } */
> > bar = 42;
> > }
> >
> > ****When compiled with -ftrivial-auto-var-init=zero -O2 -Wuninitialized -fdump-tree-all:
> >
> > .*************gimple dump:
> >
> > void testfunc ()
> > {
> > int alt_reloc;
> >
> > try
> > {
> > _1 = .DEFERRED_INIT (4, 2, 0);
> > alt_reloc = _1;
> > foo.0_2 = foo;
> > decode_reloc (foo.0_2, &alt_reloc);
> > alt_reloc.1_3 = alt_reloc;
> > if (alt_reloc.1_3 != 0) goto <D.1952>; else goto <D.1953>;
> > <D.1952>:
> > bar = 42;
> > <D.1953>:
> > }
> > finally
> > {
> > alt_reloc = {CLOBBER};
> > }
> > }
> >
> > **************fre1 dump:
> >
> > void testfunc ()
> > {
> > int alt_reloc;
> > int _1;
> > int foo.0_2;
> >
> > <bb 2> :
> > _1 = .DEFERRED_INIT (4, 2, 0);
> > foo.0_2 = foo;
> > if (foo.0_2 > 19)
> > goto <bb 3>; [50.00%]
> > else
> > goto <bb 4>; [50.00%]
> >
> > <bb 3> :
> > goto <bb 7>; [100.00%]
> >
> > <bb 4> :
> > if (foo.0_2 > 9)
> > goto <bb 5>; [50.00%]
> > else
> > goto <bb 6>; [50.00%]
> >
> > <bb 5> :
> > goto <bb 8>; [100.00%]
> >
> > <bb 6> :
> > if (_1 != 0)
> > goto <bb 7>; [INV]
> > else
> > goto <bb 8>; [INV]
> >
> > <bb 7> :
> > bar = 42;
> >
> > <bb 8> :
> > return;
> >
> > }
> >
> > From the above IR file after “FRE”, we can see that the major issue with this IR is:
> >
> > The address taken auto variable “alt_reloc” has been completely replaced by the temporary variable “_1” in all
> > the uses of the original “alt_reloc”.
> >
> > The major problem with such IR is, during uninitialized analysis phase, the original use of “alt_reloc” disappeared completely.
> > So, the warning cannot be reported.
> >
> >
> > My questions:
> >
> > 1. Is it possible to get the original “alt_reloc” through the temporary variable “_1” with some available information recorded in the IR?
> > 2. If not, then we have to record the relationship between “alt_reloc” and “_1” when the original “alt_reloc” is replaced by “_1” and get such relationship during
> > Uninitialized analysis phase. Is this doable?
> > 3. Looks like that for “address taken” auto variable, if we have to introduce a new temporary variable and split the call to .DEFERRED_INIT into two:
> >
> > temp = .DEFERRED_INIT (4, 2, 0);
> > alt_reloc = temp;
> >
> > More issues might possible.
> >
> > Any comments and suggestions on this issue?
> >
> > Qing
> >
> > j
> >> On Aug 11, 2021, at 11:55 AM, Richard Biener <rguenther@suse.de> wrote:
> >>
> >> On August 11, 2021 6:22:00 PM GMT+02:00, Qing Zhao <qing.zhao@oracle.com> wrote:
> >>>
> >>>
> >>>> On Aug 11, 2021, at 10:53 AM, Richard Biener <rguenther@suse.de> wrote:
> >>>>
> >>>> On August 11, 2021 5:30:40 PM GMT+02:00, Qing Zhao <qing.zhao@oracle.com> wrote:
> >>>>> I modified the routine “gimple_add_init_for_auto_var” as the following:
> >>>>> ====
> >>>>> /* Generate initialization to automatic variable DECL based on INIT_TYPE.
> >>>>> Build a call to internal const function DEFERRED_INIT:
> >>>>> 1st argument: SIZE of the DECL;
> >>>>> 2nd argument: INIT_TYPE;
> >>>>> 3rd argument: IS_VLA, 0 NO, 1 YES;
> >>>>>
> >>>>> as DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA). */
> >>>>> static void
> >>>>> gimple_add_init_for_auto_var (tree decl,
> >>>>> enum auto_init_type init_type,
> >>>>> bool is_vla,
> >>>>> gimple_seq *seq_p)
> >>>>> {
> >>>>> gcc_assert (VAR_P (decl) && !DECL_EXTERNAL (decl) && !TREE_STATIC (decl));
> >>>>> gcc_assert (init_type > AUTO_INIT_UNINITIALIZED);
> >>>>> tree decl_size = TYPE_SIZE_UNIT (TREE_TYPE (decl));
> >>>>>
> >>>>> tree init_type_node
> >>>>> = build_int_cst (integer_type_node, (int) init_type);
> >>>>> tree is_vla_node
> >>>>> = build_int_cst (integer_type_node, (int) is_vla);
> >>>>>
> >>>>> tree call = build_call_expr_internal_loc (UNKNOWN_LOCATION, IFN_DEFERRED_INIT,
> >>>>> TREE_TYPE (decl), 3,
> >>>>> decl_size, init_type_node,
> >>>>> is_vla_node);
> >>>>>
> >>>>> /* If this DECL is a VLA, a temporary address variable for it has been
> >>>>> created, the replacement for DECL is recorded in DECL_VALUE_EXPR (decl),
> >>>>> we should use it as the LHS of the call. */
> >>>>>
> >>>>> tree lhs_call
> >>>>> = is_vla ? DECL_VALUE_EXPR (decl) : decl;
> >>>>> gimplify_assign (lhs_call, call, seq_p);
> >>>>> }
> >>>>>
> >>>>> With this change, the current issue is resolved, the gimple dump now is:
> >>>>>
> >>>>> (*arr.1) = .DEFERRED_INIT (D.1952, 2, 1);
> >>>>>
> >>>>> However, there is another new issue:
> >>>>>
> >>>>> For the following testing case:
> >>>>>
> >>>>> ======
> >>>>> [opc@qinzhao-ol8u3-x86 gcc]$ cat t.c
> >>>>> int bar;
> >>>>>
> >>>>> extern void decode_reloc(int *);
> >>>>>
> >>>>> void testfunc()
> >>>>> {
> >>>>> int alt_reloc;
> >>>>>
> >>>>> decode_reloc(&alt_reloc);
> >>>>>
> >>>>> if (alt_reloc) /* { dg-warning "may be used uninitialized" } */
> >>>>> bar = 42;
> >>>>> }
> >>>>> =====
> >>>>>
> >>>>> In the above, the auto var “alt_reloc” is address taken, then the gimple dump for it when compiled with -ftrivial-auto-var-init=zero is:
> >>>>>
> >>>>> void testfunc ()
> >>>>> {
> >>>>> int alt_reloc;
> >>>>>
> >>>>> try
> >>>>> {
> >>>>> _1 = .DEFERRED_INIT (4, 2, 0);
> >>>>> alt_reloc = _1;
> >>>>> decode_reloc (&alt_reloc);
> >>>>> alt_reloc.0_2 = alt_reloc;
> >>>>> if (alt_reloc.0_2 != 0) goto <D.1949>; else goto <D.1950>;
> >>>>> <D.1949>:
> >>>>> bar = 42;
> >>>>> <D.1950>:
> >>>>> }
> >>>>> finally
> >>>>> {
> >>>>> alt_reloc = {CLOBBER};
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> I.e, instead of the expected IR:
> >>>>>
> >>>>> alt_reloc = .DEFERRED_INIT (4, 2, 0);
> >>>>>
> >>>>> We got the following:
> >>>>>
> >>>>> _1 = .DEFERRED_INIT (4, 2, 0);
> >>>>> alt_reloc = _1;
> >>>>>
> >>>>> I guess the temp “_1” is created because “alt_reloc” is address taken.
> >>>>
> >>>> Yes and no. The reason is that alt_reloc is memory (because it is address taken) and that GIMPLE says that register typed stores need to use a is_gimple_val RHS which the call is not.
> >>>
> >>> Okay.
> >>>>
> >>>>> My questions:
> >>>>>
> >>>>> Shall we accept such IR for .DEFERRED_INIT purpose when the auto var is address taken?
> >>>>
> >>>> I think so. Note it doesn't necessarily need address taking but any other reason that prevents SSA rewriting the variable suffices.
> >>>
> >>> You mean, in addition to “address taken”, there are other situations that will introduce such IR:
> >>>
> >>> temp = .DEFERRED_INIT();
> >>> auto_var = temp;
> >>>
> >>> So, such IR is unavoidable and we have to handle it?
> >>
> >> Yes.
> >>
> >>> If we have to handle it, what’ the best way to do it?
> >>>
> >>> The solution in my mind is:
> >>> 1. During uninitialized analysis phase, following the data flow to connect .DEFERRED_INIT to “auto_var”, and then decide that “auto_var” is uninitialized.
> >>
> >> Yes. Basically if there's an artificial variable auto initialized you have to look at its uses.
> >>
> >>> 2. During RTL expansion, following the data flow to connect .DEFERRED_INIT to “auto_var”, and then delete “temp”, and then expand .DEFERRED_INIT to auto_var.
> >>
> >> That shouldn't be necessary. You'd initialize a temporary register which is then copied to the real variable. That's good enough and should be optimized by the RTL pipeline.
> >>
> >>> Let me know your comments and suggestions on this.
> >>>
> >>>
> >>>>
> >>>> The only other option is to force. DEFERED_INIT making the LHS address taken which I think could be achieved by passing it the address as argument instead of having a LHS. But let's not go down this route - it will have quite bad behavior on alias analysis and optimization.
> >>>
> >>> Okay.
> >>>
> >>> Qing
> >>>>
> >>>>> If so, “uninitialized analysis” phase need to be further adjusted to specially handle such IR.
> >>>>>
> >>>>> If not, what should we do when the auto var is address taken?
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
next prev parent reply other threads:[~2021-08-16 7:13 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-27 3:26 Qing Zhao
2021-07-28 20:21 ` Kees Cook
2021-07-28 21:53 ` Qing Zhao
2021-08-09 14:09 ` Richard Biener
2021-08-09 16:38 ` Qing Zhao
2021-08-09 17:14 ` Richard Biener
2021-08-10 7:36 ` Richard Biener
2021-08-10 13:39 ` Qing Zhao
2021-08-10 14:16 ` Richard Biener
2021-08-10 15:02 ` Qing Zhao
2021-08-10 15:22 ` Richard Biener
2021-08-10 15:55 ` Qing Zhao
2021-08-10 20:16 ` Qing Zhao
2021-08-10 22:26 ` Qing Zhao
2021-08-11 7:02 ` Richard Biener
2021-08-11 13:33 ` Qing Zhao
2021-08-11 13:37 ` Richard Biener
2021-08-11 13:54 ` Qing Zhao
2021-08-11 13:58 ` Richard Biener
2021-08-11 14:00 ` Qing Zhao
2021-08-11 15:30 ` Qing Zhao
2021-08-11 15:53 ` Richard Biener
2021-08-11 16:22 ` Qing Zhao
2021-08-11 16:55 ` Richard Biener
2021-08-11 16:57 ` Qing Zhao
2021-08-11 20:30 ` Qing Zhao
2021-08-11 22:03 ` Qing Zhao
2021-08-16 7:12 ` Richard Biener [this message]
2021-08-16 14:48 ` Qing Zhao
2021-08-16 15:08 ` Richard Biener
2021-08-16 15:39 ` Qing Zhao
2021-08-16 7:11 ` Richard Biener
2021-08-16 16:48 ` Qing Zhao
2021-08-17 15:04 ` Qing Zhao
2021-08-17 20:40 ` Qing Zhao
2021-08-18 7:19 ` Richard Biener
2021-08-18 14:39 ` Qing Zhao
2021-08-11 9:02 ` Richard Sandiford
2021-08-11 13:44 ` Qing Zhao
2021-08-11 16:15 ` Richard Sandiford
2021-08-11 16:29 ` Qing Zhao
2021-08-12 19:24 ` Qing Zhao
2021-08-12 22:45 ` Qing Zhao
2021-08-16 7:40 ` Richard Biener
2021-08-16 15:45 ` Qing Zhao
2021-08-17 8:29 ` Richard Biener
2021-08-17 14:50 ` Qing Zhao
2021-08-17 16:08 ` Qing Zhao
2021-08-18 7:15 ` Richard Biener
2021-08-18 16:02 ` Qing Zhao
2021-08-19 9:00 ` Richard Biener
2021-08-19 13:54 ` Qing Zhao
2021-08-20 14:52 ` Qing Zhao
2021-08-23 13:55 ` Richard Biener
2021-09-02 17:24 ` Qing Zhao
2021-08-16 19:49 ` Qing Zhao
2021-08-17 8:43 ` Richard Biener
2021-08-17 14:03 ` Qing Zhao
2021-08-17 14:45 ` Richard Biener
2021-08-17 14:53 ` Qing Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.YFH.7.76.2108160911510.11781@zhemvz.fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=keescook@chromium.org \
--cc=qing.zhao@oracle.com \
--cc=richard.sandiford@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).