On Wed, 11 Aug 2021, Richard Biener wrote:
> On Tue, 10 Aug 2021, Eric Botcazou wrote:
>
> > > The question is whether we instead want to amend build3 to
> > > set TREE_THIS_VOLATILE automatically when the FIELD_DECL has
> > > it set. At least for the Fortran FE cases the gimplifier
> > > fails to see some volatile references and thus can generate
> > > wrong code which is a latent issue.
> >
> > What do we do for other similar flags, e.g. TREE_READONLY?
>
> build3 currently does no special processing for the FIELD_DECL operand,
> it just sets TREE_THIS_VOLATILE from operand zero for tcc_references.
>
> The C and C++ frontends have repeated patterns like
>
> ref = build3 (COMPONENT_REF, subtype, datum, subdatum,
> NULL_TREE);
> SET_EXPR_LOCATION (ref, loc);
> if (TREE_READONLY (subdatum)
> || (use_datum_quals && TREE_READONLY (datum)))
> TREE_READONLY (ref) = 1;
> if (TREE_THIS_VOLATILE (subdatum)
> || (use_datum_quals && TREE_THIS_VOLATILE (datum)))
> TREE_THIS_VOLATILE (ref) = 1;
>
> Leaving out TREE_READONLY shouldn't have any correctness issue. It's
> just that when adjusting the SSA operand scanner to correctly interpret
> GENERIC that this uncovers pre-existing issues in the Fortran frontend
> (one manifests in a testsuite FAIL - otherwise I wouldn't have noticed).
>
> I'm fine with just plugging the Fortran FE holes as we discover them
> but I did not check other frontends and testsuite coverage is weak.
>
> Now - I wonder if there's a reason a frontend might _not_ want to
> set TREE_THIS_VOLATILE on a COMPONENT_REF when the FIELD_DECL has
> TREE_THIS_VOLATILE set.
>
> I guess I'll do one more experiment and add verification that
> TREE_THIS_VOLATILE on COMPONENT_REFs and FIELD_DECLs is consistent
> and see where that trips.
It trips for
struct X { volatile int i; };
void foo ()
{
struct X x = (struct X){ .i = 0 };
}
where the gimplifier in gimplify_init_ctor_eval does
gcc_assert (TREE_CODE (purpose) == FIELD_DECL);
cref = build3 (COMPONENT_REF, TREE_TYPE (purpose),
unshare_expr (object), purpose, NULL_TREE);
producing
x.i = 0;
that is not volatile qualified. This manifests itself during the
build of libasan. I'm not sure whether the gimplifiers doing is
correct or not. Changing build3 would alter the behavior here.
Then there's a case where the COMPONENT_REF is TREE_THIS_VOLATILE
but neither the FIELD_DECL nor the base reference is. This
trips during libtsan build and again is from gimplification/folding,
this time gimplify_modify_expr_rhs doing
case INDIRECT_REF:
{
/* If we have code like
*(const A*)(A*)&x
where the type of "x" is a (possibly cv-qualified variant
of "A"), treat the entire expression as identical to "x".
This kind of code arises in C++ when an object is bound
to a const reference, and if "x" is a TARGET_EXPR we want
to take advantage of the optimization below. */
bool volatile_p = TREE_THIS_VOLATILE (*from_p);
tree t = gimple_fold_indirect_ref_rhs (TREE_OPERAND (*from_p,
0));
if (t)
{
if (TREE_THIS_VOLATILE (t) != volatile_p)
{
if (DECL_P (t))
t = build_simple_mem_ref_loc (EXPR_LOCATION
(*from_p),
build_fold_addr_expr
(t));
if (REFERENCE_CLASS_P (t))
TREE_THIS_VOLATILE (t) = volatile_p;
I suppose that's OK, it's folding volatile
*(void (*__sanitizer_sighandler_ptr) (int) *) &act->D.5368.handler
to act->D.5368.handler which wouldn't be volatile.
The opposite could happen, too, of course - casting away volatileness
for an access but letting that slip through verification would make
it moot. So ...
With those cases fixed bootstrap runs through and testing reveals
no additional issues apart from the already known
gfortran.dg/volatile11.f90
So I'm leaning towards leaving build3 alone and fixing up frontends
as issues pop up.
Ricahrd.
