Re: [PATCH v2] x86: Issue error for return/argument only with function body

[Uros Bizjak <ubizjak@gmail.com>]

On Fri, Mar 19, 2021 at 1:11 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Mar 19, 2021 at 1:39 AM Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > On Thu, Mar 18, 2021 at 11:44 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > If we never generate function body, we shouldn't issue errors for return
> > > nor argument.  Add init_cumulative_args_called to i386 machine_function
> > > to avoid issuing errors for return and argument without function body.
> > >
> > > gcc/
> > >
> > >         PR target/99652
> > >         * config/i386/i386.c (init_cumulative_args): Set
> > >         init_cumulative_args_called to true.
> > >         (construct_container): Issue error for return and argument only
> > >         if init_cumulative_args_called is true.
> > >         * config/i386/i386.h (machine_function): Add
> > >         init_cumulative_args_called.
> > >
> > > gcc/testsuite/
> > >
> > >         PR target/99652
> > >         * gcc.dg/torture/pr99652-1.c: New test.
> > >         * gcc.dg/torture/pr99652-2.c: Likewise.
> > >         * gcc.target/i386/pr57655.c: Adjusted.
> > >         * gcc.target/i386/pr59794-6.c: Likewise.
> > >         * gcc.target/i386/pr70738-1.c: Likewise.
> > >         * gcc.target/i386/pr96744-1.c: Likewise.
> > > ---
> > >  gcc/config/i386/i386.c                    | 26 ++++++++++++++---------
> > >  gcc/config/i386/i386.h                    |  3 +++
> > >  gcc/testsuite/gcc.dg/torture/pr99652-1.c  |  8 +++++++
> > >  gcc/testsuite/gcc.dg/torture/pr99652-2.c  |  8 +++++++
> > >  gcc/testsuite/gcc.target/i386/pr57655.c   |  4 ++--
> > >  gcc/testsuite/gcc.target/i386/pr59794-6.c |  4 ++--
> > >  gcc/testsuite/gcc.target/i386/pr70738-1.c |  4 ++--
> > >  gcc/testsuite/gcc.target/i386/pr96744-1.c |  4 ++--
> > >  8 files changed, 43 insertions(+), 18 deletions(-)
> > >  create mode 100644 gcc/testsuite/gcc.dg/torture/pr99652-1.c
> > >  create mode 100644 gcc/testsuite/gcc.dg/torture/pr99652-2.c
> > >
> > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> > > index 540d4f44517..4a0b8c73bef 100644
> > > --- a/gcc/config/i386/i386.c
> > > +++ b/gcc/config/i386/i386.c
> > > @@ -1705,6 +1705,8 @@ init_cumulative_args (CUMULATIVE_ARGS *cum,  /* Argument info to initialize */
> > >    struct cgraph_node *local_info_node = NULL;
> > >    struct cgraph_node *target = NULL;
> > >
> > > +  cfun->machine->init_cumulative_args_called = true;
> > > +
> > >    memset (cum, 0, sizeof (*cum));
> > >
> > >    if (fndecl)
> > > @@ -2534,18 +2536,21 @@ construct_container (machine_mode mode, machine_mode orig_mode,
> > >       some less clueful developer tries to use floating-point anyway.  */
> > >    if (needed_sseregs && !TARGET_SSE)
> > >      {
> > > -      if (in_return)
> > > +      if (cfun->machine->init_cumulative_args_called)
> >
> > Please make this an early return, with appropriate comment.
>
> Done.
>
> > >         {
> > > -         if (!issued_sse_ret_error)
> > > +         if (in_return)
> > >             {
> > > -             error ("SSE register return with SSE disabled");
> > > -             issued_sse_ret_error = true;
> > > +             if (!issued_sse_ret_error)
> > > +               {
> > > +                 error ("SSE register return with SSE disabled");
> > > +                 issued_sse_ret_error = true;
> > > +               }
> > > +           }
> > > +         else if (!issued_sse_arg_error)
> > > +           {
> > > +             error ("SSE register argument with SSE disabled");
> > > +             issued_sse_arg_error = true;
> > >             }
> > > -       }
> > > -      else if (!issued_sse_arg_error)
> > > -       {
> > > -         error ("SSE register argument with SSE disabled");
> > > -         issued_sse_arg_error = true;
> > >         }
> > >        return NULL;
> > >      }
> > > @@ -2558,7 +2563,8 @@ construct_container (machine_mode mode, machine_mode orig_mode,
> > >           || regclass[i] == X86_64_X87UP_CLASS
> > >           || regclass[i] == X86_64_COMPLEX_X87_CLASS)
> > >         {
> > > -         if (!issued_x87_ret_error)
> > > +         if (cfun->machine->init_cumulative_args_called
> > > +             && !issued_x87_ret_error)
> >
> > Also, early return.
>
> Done.
>
> > >             {
> > >               error ("x87 register return with x87 disabled");
> > >               issued_x87_ret_error = true;
> > > diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
> > > index 48749104b24..ad908c010b3 100644
> > > --- a/gcc/config/i386/i386.h
> > > +++ b/gcc/config/i386/i386.h
> > > @@ -2945,6 +2945,9 @@ struct GTY(()) machine_function {
> > >       function.  */
> > >    BOOL_BITFIELD has_explicit_vzeroupper : 1;
> > >
> > > +  /* If true if init_cumulative_args has been called.  */
> > > +  BOOL_BITFIELD init_cumulative_args_called: 1;
> >
> > I think that we should follow aarch64 lead and name it silent_p. The
> > comment also suits our needs.
> >
> >   bool silent_p;        /* True if we should act silently, rather than
> >                    raise an error for invalid calls.  */
> >
>
> Done.
>
> Here is the v2 patch.  OK for master?

   BOOL_BITFIELD has_explicit_vzeroupper : 1;

+  /* If true if we should act silently, rather than raise an error for

True if we should ...

+     invalid calls.  */
+  BOOL_BITFIELD silent_p : 1;
+

OK with the above comment fix.

Thanks,
Uros.