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/qu
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 :
> 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
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
> 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
Re: PR63633: May middle-end come up width hard regs for insn expanders?
Am 10/24/2014 08:29 PM, schrieb Jakub Jelinek: On Fri, Oct 24, 2014 at 08:19:57PM +0200, Georg-Johann Lay wrote: Yes, that's the straight forward approach which works so far. Bit tedious, but well... In one case expmed generated different code, though: divmodhi instead of mulhi_highpart for HI division by const_int. Cheating with costs did not help. However for now I am mostly after correct, ICE-less code. What I am concerned about is: 1) May it happen that a value lives in a hard-reg across the expander? The expander has no means to detect that situation and interfere, e.g. hard-reg = source_value // middle-end expand-code // back-end sink_value = hard-reg // middle-end where "expand-code" is one defind_expand / define_insn that clobbers / changes (parts of) hard-reg but does *not* get hard-reg as operand. This is wrong code obviously. It can happen, but if it happens, that would mean user code bug, like using register asm with an register that is unsuitable for use as global or local register var on the target, or it could be backend bug (expansion of some pattern clobbering register that has other fixed uses). You shouldn't ICE on it, but what happens is undefined. Now I have a test case where exact that happens: - An expander needs to clobber a hard register. - That hard reg is live at that time and *not* used as operand to the expander - That hard reg is neither fixed nor a global register. - That hard reg is neither used for argument passing nor for frame pointer or stack pointer and not for any exotic use like static chain etc. For the cases where the hard register is operand to the expander I have a patch; is it possible to get it in 4.9.2? It fixes both ICEs and also (but not all) wrong code situations. AFAIKT the remaining wrong code situations are as explained above. The hard register is assigned a const_int too early so that it gets clobbered by the mentioned expander. I still think that's a bug in the middle ends... Johann Before RA, the use of hard regs should be limited (pretty much just fixed regs where really necessary, global and local register variables (user needs to use with care), function arguments and return values (short lived around the call patterns). Jakub
Re: PR63633: May middle-end come up width hard regs for insn expanders?
On 10/27/14 13:33, Georg-Johann Lay wrote: Am 10/24/2014 08:29 PM, schrieb Jakub Jelinek: On Fri, Oct 24, 2014 at 08:19:57PM +0200, Georg-Johann Lay wrote: Yes, that's the straight forward approach which works so far. Bit tedious, but well... In one case expmed generated different code, though: divmodhi instead of mulhi_highpart for HI division by const_int. Cheating with costs did not help. However for now I am mostly after correct, ICE-less code. What I am concerned about is: 1) May it happen that a value lives in a hard-reg across the expander? The expander has no means to detect that situation and interfere, e.g. hard-reg = source_value // middle-end expand-code // back-end sink_value = hard-reg // middle-end where "expand-code" is one defind_expand / define_insn that clobbers / changes (parts of) hard-reg but does *not* get hard-reg as operand. This is wrong code obviously. It can happen, but if it happens, that would mean user code bug, like using register asm with an register that is unsuitable for use as global or local register var on the target, or it could be backend bug (expansion of some pattern clobbering register that has other fixed uses). You shouldn't ICE on it, but what happens is undefined. Now I have a test case where exact that happens: - An expander needs to clobber a hard register. - That hard reg is live at that time and *not* used as operand to the expander - That hard reg is neither fixed nor a global register. - That hard reg is neither used for argument passing nor for frame pointer or stack pointer and not for any exotic use like static chain etc. So can you give more details about the live hard reg? How did a value get into that hard reg, how did it get used later? That's where I'd focus my efforts since that probably shouldn't be happening except in very special circumstances. jeff
Rearrangement mirror servers
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Dear GCC Mirror Admin, I have rearranged our mirror setup which means we offer additional mirror servers mirroring GCC. Could you please make the following changes: - - Remove the current 'mirror.bbln.org' entry as mirror - - Please add the following three mirrors: Located in Gravelines, France http://mirror-fr1.bbln.org/gcc https://mirror-fr1.bbln.org/gcc ftp://mirror-fr1.bbln.org/gcc rsync://mirror-fr1.bbln.org/gcc Located in Roubaix, France http://mirror-fr2.bbln.org/gcc https://mirror-fr2.bbln.org/gcc ftp://mirror-fr2.bbln.org/gcc rsync://mirror-fr2.bbln.org/gcc Located in Amsterdam, The Netherlands http://mirror-nl1.bbln.org/gcc https://mirror-nl1.bbln.org/gcc ftp://mirror-nl1.bbln.org/gcc rsync://mirror-nl1.bbln.org/gcc As contact for these mirrors you can list: BBLN (n...@bbln.org) Thanks in advance! - -- Tim Semeijn pgp 0x08CE9B4D -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) iQEcBAEBAgAGBQJUTqSFAAoJEB4F+FYIzptNRQ4H/imDU1bWiveW0tBkj+YsHS8P 2EXCwoMTLbzASdDZkyqP0dspp3AmvhuypHbmYnjAbbLBYHtUASCxdp0j43fDwPUZ 3V9xDKA8mEvkYWS1WLXbSXxJhjbbURDaj6vGNnl+xHtRpfS1Z4HQix64qXhKX/EE pmK/YkSXh9b1FXyjviMXzJcdK4ehAulUdOBz5n50mCgK50bpS/CGvUGQUM16rz3M 0J3SuGpbjhqNZ/KVJeQzS7O9e+/3aXNpxV9/qZaIDGm18Nu2hd+BuK+m5zO6m0XY WMaSiuMI9QmEVruaWnuamIl0NJ6nGmfmfF3g5xn29R0lvDWKaFfpzkXUndiTihc= =9hVd -END PGP SIGNATURE- 0x08CE9B4D.asc Description: application/pgp-keys
