On Thu, 13 Aug 2020, Martin Sebor via Gcc-patches wrote:
> > * Maybe cdk_pointer is followed by cdk_attrs before cdk_id. In this case
> > the code won't return.
>
> I think I see the problem you're pointing out (I just don't see how
> to trigger it or test that it doesn't happen). If the tweak in
> the attached update doesn't fix it a test case would be helpful.
I think you need a while loop there, not just an if, to account for the
case of multiple consecutive cdk_attrs. At least the GNU attribute syntax
direct-declarator:
[...]
( gnu-attributes[opt] declarator )
should produce multiple consecutive cdk_attrs for each level of
parentheses with attributes inside.
> > * Maybe the code is correct to continue because we're in the case of an
> > array of pointers (cdk_array follows). But as I understand it, the intent
> > is to set up an "arg spec" that describes only the (multidimensional)
> > array that is the parameter itself - not any array pointed to. And it
> > looks to me like, in the case of an array of pointers to arrays, both sets
> > of array bounds would end up in the spec constructed.
>
> Ideally, I'd like to check even pointers to arrays and so they should
> be recorded somewhere. The middle end code doesn't do any checking
> of those yet for out-of-bounds accesses. It wasn't a goal for
> the first iteration so I've tweaked the code to avoid recording them.
Could you expand the comment on get_parm_array_spec to specify exactly
what you think the function should be putting in the returned attribute,
in what order, in cases where there are array declarators (constant,
empty, [*] and VLA) intermixed with other kinds of declarators and the
type from the type specifiers may or may not be an array type itself?
That will provide a basis for subsequent rounds of review of whether the
function is actually behaving as expected.
As far as I can see, the logic
+ if (TREE_CODE (nelts) == INTEGER_CST)
+ {
+ /* Skip all constant bounds except the most significant one.
+ The interior ones are included in the array type. */
+ if (next && (next->kind == cdk_array || next->kind == cdk_pointer))
+ continue;
will skip constant bounds in an array that's the target of a pointer
declarator, but not any other kind of bounds. Is that what you intend -
that all the other kind of bounds in pointed-to arrays will be recorded in
this string?
> > Then, the code
> >
> > + if (pd->kind == cdk_id)
> > + {
> > + /* Extract the upper bound from a parameter of an array type. */
> >
> > also seems misplaced. If the type specifiers for the parameter are a
> > typedef for an array type, that array type should be processed *before*
> > the declarator to get the correct semantics (as if the bounds from those
> > type specifiers were given in the declarator), not at the end which gets
> > that type out of order with respect to array declarators. (Processing
> > before the declarator also means clearing the results of that processing
> > if a pointer declarator is encountered at any point, because in that case
> > the array type in the type specifiers is irrelevant.)
>
> I'm not sure I follow you here. Can you show me what you mean on
> a piece of code? This test case (which IIUC does what you described)
> works as expected:
>
> $ cat q.c && gcc -O2 -S -Wall q.c
> typedef int A[7][9];
>
> void f (A[3][5]);
So this is equivalent to A[3][5][7][9]. The c_declarator structures have
the one for the [3] (the top-level bound) inside the one for the [5].
The [5] bound is skipped by the "Skip all constant bounds except the most
significant one." logic. When the [3] bound is reached, the "break;" at
the end of that processing means the "Extract the upper bound from a
parameter of an array type." never gets executed. Try replacing the [3]
bound by a VLA bound. As I read the code, it will end up generating a
spec string that records first the VLA, then the [7], when it should be
first the 9 (skipped), then the 7 (skipped), then the 5 (skipped), then
the VLA. Or if it's "void f (A *[variable][5]);", it will do the same
thing (VLA, then 7, although both the 7 and the 9 are part of the
pointed-to type).
--
Joseph S. Myers
jos...@codesourcery.com
