On 2/1/21 9:27 AM, Jakub Jelinek wrote:
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.
You might want to look at the commit first before making accusations.
I committed the subset that Richard approved in the place you suggested.
That's also what I posted in my last reply:
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564475.html
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.
None of this was committed.
Martin
