I've committed a patch to the Go frontend that fixes this problem.
Thanks for the analysis.
Ian
On Thu, May 20, 2021 at 1:59 AM guojiufu <guoji...@linux.ibm.com> wrote:
>
> On 2021-05-18 14:58, Richard Biener wrote:
> > On Mon, 17 May 2021, Ian Lance Taylor wrote:
> >
> >> On Mon, May 17, 2021 at 1:17 AM Richard Biener via Gcc-patches
> >> <gcc-patches@gcc.gnu.org> wrote:
> >> >
> >> > On Fri, May 14, 2021 at 11:19 AM guojiufu via Gcc-patches
> >> > <gcc-patches@gcc.gnu.org> wrote:
> >> > >
> >> > > On 2021-05-14 15:39, guojiufu via Gcc-patches wrote:
> >> > > > On 2021-05-14 15:15, Richard Biener wrote:
> >> > > >> On May 14, 2021 4:52:56 AM GMT+02:00, Jiufu Guo
> >> > > >> <guoji...@linux.ibm.com> wrote:
> >> > > >>> As discussed in the PR, Richard mentioned the method to
> >> > > >>> figure out which VAR was not set TREE_ADDRESSABLE, and
> >> > > >>> then cause this failure. It is address_expression which
> >> > > >>> build addr_expr (build_fold_addr_expr_loc), but not set
> >> > > >>> TREE_ADDRESSABLE.
> >> > > >>>
> >> > > >>> I drafted this patch with reference the comments from Richard
> >> > > >>> in this PR, while I'm not quite sure if more thing need to do.
> >> > > >>> So, please have review, thanks!
> >> > > >>>
> >> > > >>> Bootstrap and regtest pass on ppc64le. Is this ok for trunk?
> >> > > >>
> >> > > >> I suggest to use mark_addresssable unless we're sure expr is always
> >> > > >> an
> >> > > >> entity where TREE_ADDRESSABLE has the desired meaning.
> >> > >
> >> > > Thanks, Richard!
> >> > > You point out the root concern, I'm not sure ;)
> >> > >
> >> > > With looking at code "mark_addresssable" and code around
> >> > > tree-ssa.c:1013,
> >> > > VAR_P, PARM_DECL, and RESULT_DECL are checked before accessing
> >> > > TREE_ADDRESSABLE.
> >> > > So, just wondering if these entities need to be marked as
> >> > > TREE_ADDRESSABLE?
> >> > >
> >> > > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc
> >> > > index 5d9dbb5d068..85d324a92cc 100644
> >> > > --- a/gcc/go/go-gcc.cc
> >> > > +++ b/gcc/go/go-gcc.cc
> >> > > @@ -1680,6 +1680,11 @@ Gcc_backend::address_expression(Bexpression*
> >> > > bexpr, Location location)
> >> > > if (expr == error_mark_node)
> >> > > return this->error_expression();
> >> > >
> >> > > + if ((VAR_P(expr)
> >> > > + || TREE_CODE(expr) == PARM_DECL
> >> > > + || TREE_CODE(expr) == RESULT_DECL)
> >> > > + TREE_ADDRESSABLE (expr) = 1;
> >> > > +
> >> >
> >> > The root concern is that mark_addressable does
> >> >
> >> > while (handled_component_p (x))
> >> > x = TREE_OPERAND (x, 0);
> >> >
> >> > and I do not know the constraints on 'expr' as passed to
> >> > Gcc_backend::address_expression.
> >> >
> >> > I think we need input from Ian here. Most FEs have their own
> >> > *_mark_addressable
> >> > function where they also emit diagnostics (guess this is handled in
> >> > the actual Go frontend).
> >> > Since Gcc_backend does lowering to GENERIC using a middle-end is
> >> > probably OK.
> >>
> >> I doubt I understand all the issues here.
> >>
> >> In general the Go frontend only takes the addresses of VAR_DECLs or
> >> PARM_DECLs. It doesn't bother to set TREE_ADDRESSABLE for global
> >> variables for which TREE_STATIC or DECL_EXTERNAL is true. For local
> >> variables it sets TREE_ADDRESSABLE based on the is_address_taken
> >> parameter to Gcc_backend::local_variable, and similarly for PARM_DECLs
> >> and Gcc_backend::parameter_variable.
> >>
> >> The name in the bug report is for a string initializer, which should
> >> be TREE_STATIC == 1 and TREE_PUBLIC == 0. Perhaps the fix is simply
> >> to set TREE_ADDRESSABLE in Gcc_backend::immutable_struct and
> >> Gcc_backend::implicit_variable. I can't see how it would hurt to set
> >> TREE_ADDRESSABLE unnecessarily for a TREE_STATIC variable.
> >>
> >> But, again, I doubt I understand all the issues here.
> >
> > GENERIC requires TREE_ADDRESSABLE to be set on all address-taken
> > VAR_DECLs, PARM_DECLs and RESULT_DECLs - the gimplifier is the
> > first to require this for correctness. Setting TREE_ADDRESSABLE
> > when the address is not taken is harmless and at most results in
> > missed optimizations (on most entities we are able to clear the
> > flag later).
> >
> > We're currently quite forgiving with this though (still the
> > gimplifier can generate wrong-code). The trigger of the current
> > failure removed one "forgiveness", I do plan to remove a few more.
> >
> > guojiufu's patch works for me but as said I'm not sure if there's
> > a better place to set TREE_ADDRESSABLE for entities that have
> > their address taken - definitely catching the places where
> > you build an ADDR_EXPR are the most obvious ones.
> >
> > Richard.
>
> I tested below patch As Ian said, bootstrap pass.
>
> ----------------------------------------
> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc
> index 5d9dbb5d068..529f657598a 100644
> --- a/gcc/go/go-gcc.cc
> +++ b/gcc/go/go-gcc.cc
> @@ -2943,6 +2943,7 @@ Gcc_backend::implicit_variable(const std::string&
> name,
> TREE_STATIC(decl) = 1;
> TREE_USED(decl) = 1;
> DECL_ARTIFICIAL(decl) = 1;
> + TREE_ADDRESSABLE(decl) = 1;
> if (is_common)
> {
> DECL_COMMON(decl) = 1;
> @@ -3053,6 +3054,7 @@ Gcc_backend::immutable_struct(const std::string&
> name,
> TREE_READONLY(decl) = 1;
> TREE_CONSTANT(decl) = 1;
> DECL_ARTIFICIAL(decl) = 1;
> + TREE_ADDRESSABLE(decl) = 1;
> if (!is_hidden)
> TREE_PUBLIC(decl) = 1;
> if (! asm_name.empty())
> ----------------------------------------
> While, I hacked a patch for OPERATOR_AND. I'm thinking it may be
> acceptable.
>
> BR.
> Jiufu Guo.
> ----------------------------------------
> Bootstrap failure go [PR100537]
>
> In general the Go frontend only takes the addresses of VAR_DECLs or
> PARM_DECLs. It doesn't bother to set TREE_ADDRESSABLE for global
> variables for which TREE_STATIC or DECL_EXTERNAL is true.
>
> This patch sets TREE_ADDRESSABLE for those symbols which is under
> OPERATOR_AND (&). Since, I feel, we may able to treat
> "Unary_expression:OPERATOR_AND" as address_taken operation.
>
> Bootstra pass on ppc64le. Is this ok for trunk?
>
>
> gcc/ChangeLog:
> 2021-05-20 Richard Biener <rguent...@suse.de>
> Jiufu Guo <guoji...@linux.ibm.com>
>
> PR go/100537
> * gimple-expr.c (mark_addressable): Check cfun.
>
> gcc/go/ChangeLog:
> 2021-05-20 Richard Biener <rguent...@suse.de>
> Jiufu Guo <guoji...@linux.ibm.com>
>
> PR go/100537
> * go-gcc.cc (class Gcc_backend): New mark_addresstaken function.
> (Gcc_backend::mark_addresstaken): New function.
> * gofrontend/backend.h (class Backend): New mark_addresstaken
> function.
> * gofrontend/expressions.cc
> (Unary_expression::do_get_backend): Call mark_addresstaken.
> ---
> gcc/gimple-expr.c | 1 +
> gcc/go/go-gcc.cc | 13 +++++++++++++
> gcc/go/gofrontend/backend.h | 4 ++++
> gcc/go/gofrontend/expressions.cc | 1 +
> 4 files changed, 19 insertions(+)
>
> diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c
> index b8c732b632a..f682841391b 100644
> --- a/gcc/gimple-expr.c
> +++ b/gcc/gimple-expr.c
> @@ -915,6 +915,7 @@ mark_addressable (tree x)
> if (TREE_CODE (x) == VAR_DECL
> && !DECL_EXTERNAL (x)
> && !TREE_STATIC (x)
> + && cfun != NULL
> && cfun->gimple_df != NULL
> && cfun->gimple_df->decls_to_pointers != NULL)
> {
> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc
> index 5d9dbb5d068..fdbc72a2c75 100644
> --- a/gcc/go/go-gcc.cc
> +++ b/gcc/go/go-gcc.cc
> @@ -319,6 +319,9 @@ class Gcc_backend : public Backend
> Bexpression*
> address_expression(Bexpression*, Location);
>
> + void
> + mark_addresstaken(Bexpression*);
> +
> Bexpression*
> struct_field_expression(Bexpression*, size_t, Location);
>
> @@ -1684,6 +1687,16 @@ Gcc_backend::address_expression(Bexpression*
> bexpr, Location location)
> return this->make_expression(ret);
> }
>
> +void
> +Gcc_backend::mark_addresstaken(Bexpression* bexpr)
> +{
> + tree expr = bexpr->get_tree();
> + if (expr == error_mark_node)
> + return;
> +
> + mark_addressable(expr);
> +}
> +
> // Return an expression for the field at INDEX in BSTRUCT.
>
> Bexpression*
> diff --git a/gcc/go/gofrontend/backend.h b/gcc/go/gofrontend/backend.h
> index c5b5d8f2054..45284ed2f0a 100644
> --- a/gcc/go/gofrontend/backend.h
> +++ b/gcc/go/gofrontend/backend.h
> @@ -315,6 +315,10 @@ class Backend
> virtual Bexpression*
> address_expression(Bexpression*, Location) = 0;
>
> + // Mark an expression that takes the address of an expression.
> + virtual void
> + mark_addresstaken(Bexpression*) = 0;
> +
> // Return an expression for the field at INDEX in BSTRUCT.
> virtual Bexpression*
> struct_field_expression(Bexpression* bstruct, size_t index, Location)
> = 0;
> diff --git a/gcc/go/gofrontend/expressions.cc
> b/gcc/go/gofrontend/expressions.cc
> index 5409d269ebf..4e52c8eafba 100644
> --- a/gcc/go/gofrontend/expressions.cc
> +++ b/gcc/go/gofrontend/expressions.cc
> @@ -5327,6 +5327,7 @@
> Unary_expression::do_get_backend(Translate_context* context)
> }
>
> go_assert(!this->create_temp_ ||
> this->expr_->is_multi_eval_safe());
> + gogo->backend()->mark_addresstaken(bexpr);
> ret = gogo->backend()->address_expression(bexpr, loc);
> break;
>
> --
> 2.17.1
>
>
