[[PATCH] 1/3] aarch64: Sanitize access to cfun in aarch64_declare_function_name()

[Christoph Müllner]

On Thu, Mar 4, 2021 at 1:19 AM Andrew Pinski <pinskia@gmail.com> wrote:

> On Wed, Mar 3, 2021 at 4:02 PM Christoph Müllner <cmuellner@gcc.gnu.org>
> wrote:
> >
> > [CCing Andrew Pinski, who had the same question]
> >
> > On 2/15/21 1:12 PM, Richard Earnshaw wrote:
> > > On 09/12/2020 17:21, Christoph Müllner wrote:
> > >> From: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> > >>
> > >> It is possible to call aarch64_declare_function_name() and
> > >> have cfun not set. Let's sanitize the access to this variable.
> > >>
> > >> gcc/
> > >>
> > >>     * config/aarch64/aarch64.c (aarch64_declare_function_name):
> > >>     Santize access to cfun.
> > >> ---
> > >>  gcc/config/aarch64/aarch64.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/gcc/config/aarch64/aarch64.c
> b/gcc/config/aarch64/aarch64.c
> > >> index 67ffba02d3e..264ccb8beb2 100644
> > >> --- a/gcc/config/aarch64/aarch64.c
> > >> +++ b/gcc/config/aarch64/aarch64.c
> > >> @@ -19317,7 +19317,8 @@ aarch64_declare_function_name (FILE *stream,
> const char* name,
> > >>    ASM_OUTPUT_TYPE_DIRECTIVE (stream, name, "function");
> > >>    ASM_OUTPUT_LABEL (stream, name);
> > >>
> > >> -  cfun->machine->label_is_assembled = true;
> > >> +  if (cfun)
> > >> +    cfun->machine->label_is_assembled = true;
> > >>  }
> > >>
> > >>  /* Implement PRINT_PATCHABLE_FUNCTION_ENTRY.  Check if the patch
> area is after
> > >>
> > >
> > > Do you have a simple testcase that demonstrates this?  If so, we likely
> > > have a regression here that will not only need fixing, but back-porting
> > > as well.
> >
> > I was testing this on master back then in December and it this patch
> > had an influence on the tests (make check), but I can't recall which
> ones.
> > I rebased the patches today and cannot reproduce the issues anymore with
> "make check".
> > I.e. this patch does not influence any tests as of today's master branch
> (anymore).
> >
> > However, if I also apply patch 3/3 of this series, then this patch is
> needed.
> > More or less all new tests of patch 3/3 trigger this test otherwise.
>
> That still does not describe why cfun is null in the case of patch 3/3?
> From looking into that patch I noticed you call set_cfun wtih null in
> output_indirect_thunk_function shouldn't you push and pop the cfun
> instead?
>

I got very much inspired by the function with the same name in
gcc/config/i386/i386.c. Therefore I assumed, set_cfun(NULL) is fine.
I guess, that the code does so, because it is called via the macro
TARGET_ASM_CODE_END, and setting cfun to NULL is considered
as prevention of mistakes.

Thanks,
Christoph


>
> Thanks,
> Andrew Pinski
>
> >
> > Thanks,
> > Christoph
>