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.
One more finding:
Gcc_backend::implicit_variable -> build_decl is called
for "<var_decl 0x200000a285e0 go..C479>" at
Unary_expression::do_get_backend (expressions.cc:5322).
And, this code (as below) from "expressions.cc:5322"
Unary_expression::do_get_backend (expressions.cc:5322):
gogo->backend()->implicit_variable(var_name, "", btype, true, true,
false, 0);
where var_name is go..C479
This code is under **"case OPERATOR_AND:"** of a switch statement.
Unary_expression with OPERATOR_AND is "&" expression, I guess,
it may look as taking address.
And as the log mentioned: "PHI <err$__object_76(58), &go..C479(59)>".
In this phi, &go..C479(59) would be the Unary_expression with
OPERATOR_AND
on "go..C479".
So, I guess, we may able to treat "Unary_expression with OPERATOR_AND"
as address_taken operation. Then it would be necessary to mark
addressable.
address_expression: "ret = gogo->backend()->address_expression(bexpr,
loc);"
(expressions.cc:5330) is already called under "Unary_expression with
OPERATOR_AND".
Does this make sense? If so, we may set "TREE_ADDRESSABLE" just before
expressions.cc:5330?
Hope this finding is helpful.
BR.
Jiufu Guo.
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.
