On Mon, Feb 01, 2021 at 09:11:20AM -0700, Martin Sebor via Gcc-patches wrote:
> > > > Because free_lang_data only frees anything when LTO is enabled and
> > > > we want these trees cleared regardless to keep them from getting
> > > > clobbered during gimplification, this change also modifies the pass
> > > > to do the clearing even when the pass is otherwise inactive.
> > >
> > > if (TREE_CODE (bound) == NOP_EXPR)
> > > + bound = TREE_OPERAND (bound, 0);
> > > +
> > > + if (TREE_CODE (bound) == CONVERT_EXPR)
> > > + {
> > > + tree op0 = TREE_OPERAND (bound, 0);
> > > + tree bndtyp = TREE_TYPE (bound);
> > > + tree op0typ = TREE_TYPE (op0);
> > > + if (TYPE_PRECISION (bndtyp) == TYPE_PRECISION (op0typ))
> > > + bound = op0;
> > > + }
> > > +
> > > + if (TREE_CODE (bound) == NON_LVALUE_EXPR)
> > > + bound = TREE_OPERAND (bound, 0);
> > >
> > > all of the above can be just
> > >
> > > STRIP_NOPS (bound);
> > >
> > > which also handles nesting of the above in any order.
> >
> > No, it can't be just STRIP_NOPS.
> >
> > The goal is to strip the meaningless (to the user) cast to sizetype
> > from the array type. For example:
> >
> > void f (int n, int[n]);
> > void f (int n, int[n + 1]);
> >
> > I want the type in the warning to reflect the source:
> >
> > warning: argument 2 of type ‘int[n + 1]’ declared with mismatched
> > bound ‘n + 1’ [-Wvla-parameter]
> >
> > and not:
> >
> > warning: ... ‘int[(sizetype)(n + 1)]’ ...
> >
> > >
> > > + if (TREE_CODE (bound) == PLUS_EXPR
> > > + && integer_all_onesp (TREE_OPERAND (bound, 1)))
> > > + {
> > > + bound = TREE_OPERAND (bound, 0);
> > > + if (TREE_CODE (bound) == NOP_EXPR)
> > > + bound = TREE_OPERAND (bound, 0);
> > > + }
> > >
> > > so it either does or does not strip a -1 but has no indication on
> > > whether it did that? That looks fragile and broken.
> >
> > Indication to what? The caller? The function is only used to recover
> > a meaningful VLA bound for warnings and its callers aren't interested
> > in whether the -1 was stripped or not.
> >
> > >
> > > Anyway, the split out of this function seems unrelated to the
> > > original problem and should be submitted separately.
> >
> > It was a remnant of the previous patch where it was used to format
> > the string representation of the VLA bounds and called from three
> > sites. I've removed the function from this revision (and restored
> > the one site in the pretty printer that open-codes the same thing).
> >
> > >
> > > + for (vblist = TREE_VALUE (vblist); vblist; vblist =
> > > TREE_CHAIN (vblist))
> > > + {
> > > + tree *pvbnd = &TREE_VALUE (vblist);
> > > + if (!*pvbnd || DECL_P (*pvbnd))
> > > + continue;
> > >
> > > so this doesn't let constant bounds prevail but only symbolical
> > > ones? Not
> > > that I care but I'd have expected || CONSTANT_CLASS_P (*pvbnd)
> >
> > There must be some confusion here. There are no constant VLA bounds.
> > The essential purpose of this patch is to remove expressions from
> > the attributes, such as PLUS_EXPR, that denote nontrivial VLA bounds.
> > The test above retains decls that might refer to function parameters
> > or global variables so that they can be mentioned in middle end
> > warnings.
> >
> > Attached is yet another revision of this fix that moves the call
> > to attr_access:free_lang_data() to c_parse_final_cleanups() as
> > Jakub suggested.
>
> With no further comments I have committed the final patch in
> g:0718336a528.
This is unacceptable, you chose to ignore Richard's comments,
nobody has approved the patch and you've committed it anyway.
The code of course should be using STRIP_NOPS, and if the callers
don't care if you sometimes strip away + -1 from it or not, they are just
broken. Either the expression stands for the largest valid index into the
array, or it stands for the number of array elements. If the former, you
don't want to strip away + -1 when it appears, if the latter, you do want to
strip it away but if you don't find it, you need to add + 1 yourself, the +
-1 could disappear from earlier folding.
Jakub
