Re: [PATCH] warning about const multidimensional array as function parameter
Hi,
attached is a revised and extended patch. Changes with respect
to the previous patch are:
- warn if qualifiers are lost for pointers to multi-dimensional arrays
- warn if qualifiers are lost when converting to void*
- warnings for _Atomic are preserved
- qualifiers are not lost in conditional expressions
- new -Wdiscarded-array-qualifiers option (on by default)
- document as extension
- tests for initialization/assignment/arguments/return/conditionals/
comparison/subtration w and w/o '-pedantic' and including tests
for multi-dimensional arrays
- tests for _Atomic
Note that there is now a semantic (and not only diagnostic) change.
Without this patch
const int a[1];
int b[1];
(x ? a : b)
would return a 'void*' and a warning about pointer type mismatch.
With this patch the conditional has type 'const int (*)[1]'.
-- Martin
2014-10-28 Martin Uecker uec...@eecs.berkeley.edu
* doc/invoke.texi: Document -Wdiscarded-array-qualifiers
* doc/extend.texi: Document new behavior for pointers to arrays with
qualifies
c/
* c-typeck.c: New behavior for pointers to arrays with qualifiers
c-family/
* c.opt (Wdiscarded-array-qualifiers): New option
testsuite/
* gcc.dg/Wwrite-strings-1.c: Change dg-warning
* gcc.dg/array-quals-1.c: Use -Wno-discarded-array-qualifiers
* gcc.dg/array-quals-2.c: Change dg-options, dg-warning
* gcc.dg/pointer-array-atomic.c: New test
* gcc.dg/pointer-array-quals.c: New test
Index: gcc/c/c-typeck.c
===
--- gcc/c/c-typeck.c(Revision 216816)
+++ gcc/c/c-typeck.c(Arbeitskopie)
@@ -673,12 +673,13 @@
mv2 = TYPE_MAIN_VARIANT (pointed_to_2);
target = composite_type (mv1, mv2);
+ /* Strip array types to get correct qualifier for pointers to arrays */
+ quals1 = TYPE_QUALS_NO_ADDR_SPACE (strip_array_types (pointed_to_1));
+ quals2 = TYPE_QUALS_NO_ADDR_SPACE (strip_array_types (pointed_to_2));
+
/* For function types do not merge const qualifiers, but drop them
if used inconsistently. The middle-end uses these to mark const
and noreturn functions. */
- quals1 = TYPE_QUALS_NO_ADDR_SPACE (pointed_to_1);
- quals2 = TYPE_QUALS_NO_ADDR_SPACE (pointed_to_2);
-
if (TREE_CODE (pointed_to_1) == FUNCTION_TYPE)
target_quals = (quals1 quals2);
else
@@ -1224,6 +1225,7 @@
comp_target_types (location_t location, tree ttl, tree ttr)
{
int val;
+ int val_ped;
tree mvl = TREE_TYPE (ttl);
tree mvr = TREE_TYPE (ttr);
addr_space_t asl = TYPE_ADDR_SPACE (mvl);
@@ -1235,19 +1237,32 @@
if (!addr_space_superset (asl, asr, as_common))
return 0;
- /* Do not lose qualifiers on element types of array types that are
- pointer targets by taking their TYPE_MAIN_VARIANT. */
- if (TREE_CODE (mvl) != ARRAY_TYPE)
-mvl = (TYPE_ATOMIC (mvl)
- ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
- : TYPE_MAIN_VARIANT (mvl));
- if (TREE_CODE (mvr) != ARRAY_TYPE)
-mvr = (TYPE_ATOMIC (mvr)
- ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
- : TYPE_MAIN_VARIANT (mvr));
+ /* For -pedantic record result of comptypes on arrays before loosing
+ qualifiers on the element type below. */
+ val_ped = 1;
+
+ if (TREE_CODE (mvl) == ARRAY_TYPE
+ TREE_CODE (mvr) == ARRAY_TYPE)
+val_ped = comptypes (mvl, mvr);
+
+ /* Qualifiers on element types of array types that are
+ pointer targets are lost by taking their TYPE_MAIN_VARIANT. */
+
+ mvl = (TYPE_ATOMIC (strip_array_types (mvl))
+? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
+: TYPE_MAIN_VARIANT (mvl));
+
+ mvr = (TYPE_ATOMIC (strip_array_types (mvr))
+? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
+: TYPE_MAIN_VARIANT (mvr));
+
enum_and_int_p = false;
val = comptypes_check_enum_int (mvl, mvr, enum_and_int_p);
+ if (val == 1 val_ped != 1)
+pedwarn (location, OPT_Wpedantic, pointers to arrays with different
qualifiers
+ are incompatible in ISO C);
+
if (val == 2)
pedwarn (location, OPT_Wpedantic, types are not quite compatible);
@@ -6090,7 +6105,31 @@
== c_common_signed_type (mvr))
TYPE_ATOMIC (mvl) == TYPE_ATOMIC (mvr)))
{
- if (pedantic
+ /* Warn about conversions for pointers to arrays with different
+ qualifiers on the element type. Otherwise we only warn about
+ these as being incompatible pointers with -pedantic. */
+ if (OPT_Wdiscarded_array_qualifiers
+ ((TREE_CODE (ttr) == ARRAY_TYPE)
+ || TREE_CODE (ttl) == ARRAY_TYPE))
+{
+ ttr = strip_array_types(ttr);
+ ttl = strip_array_types(ttl);
+
+ if (TYPE_QUALS_NO_ADDR_SPACE_NO_ATOMIC (ttr)
+
Re: [PATCH] warning about const multidimensional array as function parameter
On Tue, 28 Oct 2014, Martin Uecker wrote:
attached is a revised and extended patch. Changes with respect
to the previous patch are:
Thanks for the revised patch. I've moved this to gcc-patches as the more
appropriate mailing list for discussion of specific patches as opposed to
more general questions. It would also be a good idea to get started on
the paperwork
http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future
if you haven't already.
Note that there is now a semantic (and not only diagnostic) change.
Without this patch
const int a[1];
int b[1];
(x ? a : b)
would return a 'void*' and a warning about pointer type mismatch.
With this patch the conditional has type 'const int (*)[1]'.
I believe that is safe (in that that conditional expression isn't valid in
ISO C). What wouldn't be safe is making a conditional expression between
void * and const int (*)[] have type const void * instead of void *.
* c-typeck.c: New behavior for pointers to arrays with qualifiers
Note that the ChangeLog entry should name the functions being changed and
what changed in each function (it's also helpful to diff with svn diff -x
-up so that the function names are visible in the diff).
@@ -6090,7 +6105,31 @@
== c_common_signed_type (mvr))
TYPE_ATOMIC (mvl) == TYPE_ATOMIC (mvr)))
{
- if (pedantic
+ /* Warn about conversions for pointers to arrays with different
+ qualifiers on the element type. Otherwise we only warn about
+ these as being incompatible pointers with -pedantic. */
+ if (OPT_Wdiscarded_array_qualifiers
+ ((TREE_CODE (ttr) == ARRAY_TYPE)
+ || TREE_CODE (ttl) == ARRAY_TYPE))
+{
+ ttr = strip_array_types(ttr);
Note there should be a space before the open parenthesis.
+ ttl = strip_array_types(ttl);
+
+ if (TYPE_QUALS_NO_ADDR_SPACE_NO_ATOMIC (ttr)
+~TYPE_QUALS_NO_ADDR_SPACE_NO_ATOMIC (ttl))
+ WARN_FOR_QUALIFIERS (location, expr_loc,
WARN_FOR_QUALIFIERS uses pedwarn. That means this is not safe, because
pedwarns become errors with -pedantic-errors, but this includes cases that
are valid in ISO C and so must not become errors with -pedantic-errors
(such as converting const int (*)[] to void *).
So you need a variant of WARN_FOR_QUALIFIERS that uses warning_at instead
of pedwarn. But then you *also* need to be careful not to lose errors
with -pedantic-errors for cases where they are required by ISO C but not
with the C++ handling of qualifiers (such as converting const void * to
const int (*)[]) - so you can't have an if / else chain where an earlier
case gives a plain warning and stops a later case from running that would
give a pedwarn required by ISO C.
I think the correct logic might be something like:
* If the existing check for discarding qualifiers applies, then: recheck
the qualifiers after strip_array_types; give the existing
WARN_FOR_QUALIFIERS diagnostic if either qualifiers are still being
discarded with the C++-style interpretation, or -pedantic.
* As the next case after that existing check, see if qualifiers are being
discarded with the C++-style interpretation even though they weren't with
the C standard interpretation, and if so then give diagnostics using the
new macro that uses warning_at instead of pedwarn.
(And otherwise the code would fall through to the existing cases relating
to mismatch in signedness between two pointers to integers.)
--
Joseph S. Myers
jos...@codesourcery.com
Re: [PATCH] warning about const multidimensional array as function parameter
On Sat, 25 Oct 2014, Martin Uecker wrote:
Strictly speaking the C standard considers such pointers to be
incompatible. This seems to be an unintentional consequence
of how qualifiers are always attached to the element type.
(I am trying to get the standard revised too.) The new
behaviour should also be more compatible with C++.
What is the exact difference in wording in the C++ standard that results
in this difference in semantics? If we implement the C++ semantics in
some cases for C, we should make sure to be as compatible is possible with
C++. (Within the constraints of not introducing extra warnings by
default, I suppose - a conversion from const int (*)[] to void * is valid
for C, even if not for C++. Of course -Wc++-compat could warn for such
cases valid for C but not for C++, but that would be a separate issue.)
Note that there are several cases affected (assignment / initialization /
function call / return; conditional expressions; pointer subtraction;
pointer comparisons (ordered and unordered), at least) so we should make
sure of what the standard wording covers, and make sure that all relevant
cases are covered in the testsuite (both for getting the correct -pedantic
diagnostic, and not getting the diagnostic in the default case if that's
what's intended). These cases may also apply to both single- and
multi-dimensional arrays. And where a composite type is formed in a
conditional expression, there's the matter of making sure it has all the
qualifiers from either side of the expression (I think the existing code
will get that right, so it's just a matter of covering it in the
testsuite).
The acceptance of this as an extension should also be documented in
extend.texi.
- /* Do not lose qualifiers on element types of array types that are
- pointer targets by taking their TYPE_MAIN_VARIANT. */
- if (TREE_CODE (mvl) != ARRAY_TYPE)
-mvl = (TYPE_ATOMIC (mvl)
-? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
-: TYPE_MAIN_VARIANT (mvl));
- if (TREE_CODE (mvr) != ARRAY_TYPE)
-mvr = (TYPE_ATOMIC (mvr)
-? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
-: TYPE_MAIN_VARIANT (mvr));
+ /* For -pedantic record result of comptypes on arrays before loosing
+ qualifiers on the element type below. */
+ val2 = 1;
+
+ if (TREE_CODE (mvl) == ARRAY_TYPE
+ TREE_CODE (mvr) == ARRAY_TYPE)
+val2 = comptypes (mvl, mvr);
+
+ /* Qualifiers on element types of array types that are
+ pointer targets are lost by taking their TYPE_MAIN_VARIANT. */
+
+ mvl = (TYPE_ATOMIC (mvl)
+ ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
+ : TYPE_MAIN_VARIANT (mvl));
+
+ mvr = (TYPE_ATOMIC (mvr)
+ ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
+ : TYPE_MAIN_VARIANT (mvr));
Won't this remove _Atomic even on arrays (since it's the element, possibly
after multiple levels of indirection, that satisfies TYPE_ATOMIC, rather
than the array itself)? As in C standard terms _Atomic types aren't
qualified (it's syntactically a qualifier, but not generally included in
the term qualified type), I think diagnostics *should* still be given by
default for e.g. passing int (*)[] where _Atomic int (*)[] is expected.
(Part of the logic for the incompatibility is that it's allowed for
_Atomic int to be larger than plain int, for example.)
@@ -5961,7 +5974,7 @@ convert_for_assignment (location_t locat
int target_cmp = 0; /* Cache comp_target_types () result. */
addr_space_t asl;
addr_space_t asr;
-
+
if (TREE_CODE (mvl) != ARRAY_TYPE)
mvl = (TYPE_ATOMIC (mvl)
? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl),
Please avoid spurious diff hunks (especially where the change is in the
wrong direction - adding trailing whitespace, as here).
@@ -6083,6 +6096,16 @@ convert_for_assignment (location_t locat
== c_common_signed_type (mvr))
TYPE_ATOMIC (mvl) == TYPE_ATOMIC (mvr)))
{
+ /* For arrays consider element type instead. This is required
because
+ we warn about conversions for pointers to arrays with different
+ qualifiers on the element type only for -pedantic. */
+ if (TREE_CODE (ttr) == ARRAY_TYPE
+ TREE_CODE (ttl) == ARRAY_TYPE)
+{
+ ttr = TREE_TYPE (ttr);
+ ttl = TREE_TYPE (ttl);
+}
This doesn't look to me as if it will work for multidimensional arrays.
Presumably you still want diagnostics for passing const int (*)[1][1][1]
where int (*)[1][1][1] is expected, but not vice versa.
Index: gcc/testsuite/gcc.dg/qual-component-1.c
===
--- gcc/testsuite/gcc.dg/qual-component-1.c (Revision 216690)
+++
Re: [PATCH] warning about const multidimensional array as function parameter
On 27 October 2014 13:10, Joseph S. Myers wrote: On Sat, 25 Oct 2014, Martin Uecker wrote: Strictly speaking the C standard considers such pointers to be incompatible. This seems to be an unintentional consequence of how qualifiers are always attached to the element type. (I am trying to get the standard revised too.) The new behaviour should also be more compatible with C++. What is the exact difference in wording in the C++ standard that results in this difference in semantics? See 4.4 [conv.qual] in https://isocpp.org/files/papers/N3797.pdf
Re: [PATCH] warning about const multidimensional array as function parameter
Jonathan Wakely jwakely@gmail.com:
On 27 October 2014 13:10, Joseph S. Myers wrote:
On Sat, 25 Oct 2014, Martin Uecker wrote:
Strictly speaking the C standard considers such pointers to be
incompatible. This seems to be an unintentional consequence
of how qualifiers are always attached to the element type.
(I am trying to get the standard revised too.) The new
behaviour should also be more compatible with C++.
What is the exact difference in wording in the C++ standard that results
in this difference in semantics?
See 4.4 [conv.qual] in https://isocpp.org/files/papers/N3797.pdf
Note that this doesn't talk explicitly about arrays
and that C++ keeps the notion that qualifiers are
always attached to the element type:
---
3.9.3(2) ... Any cv-qualifiers applied to an array type
affect the array element type, not the array type (8.3.4).
---
and
---
3.9.3(5) ...
Cv-qualifiers applied to an array type attach to the
underlying element type, so the notation “cv T,” where
T is an array type, refers to an array whose elements
are so-qualified. Such array types can be said to be
more (or less) cv-qualified than other types based on
the cv-qualification of the underlying element types.
---
I *believe* (but I don't know the C++ standard very well)
that all the magic is in the wording can be said to be more
(or less) cv-qualified which makes the conversion rules work
for arrays with constant element type as if the array itself
had the qualifier.
---
4.4 Qualification conversions
4.4(1) A prvalue of type “pointer to cv1 T” can be converted
to a prvalue of type “pointer to cv2 T” if “cv2 T” is more
cv-qualified than “cv1 T”.
---
There is another issue in C which has the same underlying reason
(brought up by Tim Rentsch in comp.std.c) as shown in the following
example (this is legal C and compiles without a warning (gcc) but is
illegal in C++):
#include string.h
static const int x[5] = { 0 };
void test(void)
{
memset(x, 0, sizeof(x));
}
I did not try to address this in the patch because it would make legal
code have a warning, but one could think about it.
Martin
PS: Joseph, thank you for reviewing the patch.
Re: [PATCH] warning about const multidimensional array as function parameter
On Mon, 27 Oct 2014, Martin Uecker wrote:
3.9.3(5) ...
Cv-qualifiers applied to an array type attach to the
underlying element type, so the notation cv T, where
T is an array type, refers to an array whose elements
are so-qualified. Such array types can be said to be
more (or less) cv-qualified than other types based on
the cv-qualification of the underlying element types.
I think that is what's relevant. (The wording you quote seems to be the
C++11 (and C++98/C++03) wording; N3797 has An array type whose elements
are cv-qualified is also considered to have the same cv-qualifications as
its elements.; the final C++14 standard doesn't yet seem to be
available.)
There is another issue in C which has the same underlying reason
(brought up by Tim Rentsch in comp.std.c) as shown in the following
example (this is legal C and compiles without a warning (gcc) but is
illegal in C++):
#include string.h
static const int x[5] = { 0 };
void test(void)
{
memset(x, 0, sizeof(x));
}
I did not try to address this in the patch because it would make legal
code have a warning, but one could think about it.
That code clearly can't have a pedwarn (as it's valid C, it mustn't have
an error with -pedantic-errors). And it would clearly be fine for it to
be diagnosed with -Wc++-compat. Warnings (not pedwarns) by default, with
some option to disable them, could be considered if it's unlikely people
will intentionally do this in C.
--
Joseph S. Myers
jos...@codesourcery.com
