| Message ID | AM5PR0701MB26571284440889EF6AFF47B8E4910@AM5PR0701MB2657.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add a -Wcast-align=strict warning | expand |
Ping... On 09/04/17 10:07, Bernd Edlinger wrote: > Hi, > > as you know we have a -Wcast-align warning which works only for > STRICT_ALIGNMENT targets. But occasionally it would be nice to be > able to switch this warning on even for other targets. > > Therefore I would like to add a strict version of this option > which can be invoked with -Wcast-align=strict. With the only > difference that it does not depend on STRICT_ALIGNMENT. > > I used the code from check_effective_target_non_strict_align > in target-supports.exp for the first version of the test case, > where we have this: > > return [check_no_compiler_messages non_strict_align assembly { > char *y; > typedef char __attribute__ ((__aligned__(__BIGGEST_ALIGNMENT__))) c; > c *z; > void foo(void) { z = (c *) y; } > } "-Wcast-align"] > > ... and to my big surprise it did _not_ work for C++ as-is, > because same_type_p considers differently aligned types identical, > and therefore cp_build_c_cast tries the conversion first via a > const_cast which succeeds, but did not emit the cast-align warning > in this case. > > As a work-around I had to check the alignment in build_const_cast_1 > as well. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > > > Thanks > Bernd. >
What does this warning do in cases where a type has different alignments
inside and outside structs? I'm thinking of something like
struct s { long long x; } *p;
/* ... */
(long long *)p
on 32-bit x86 - where long long's preferred alignment is 8 bytes, but in
structures it's 4 bytes. (Likewise for double in place of long long.) I
think a warning for a (long long *)p cast might be surprising in that
case.
On 09/13/17 19:06, Joseph Myers wrote: > What does this warning do in cases where a type has different alignments > inside and outside structs? I'm thinking of something like > > struct s { long long x; } *p; > /* ... */ > (long long *)p > > on 32-bit x86 - where long long's preferred alignment is 8 bytes, but in > structures it's 4 bytes. (Likewise for double in place of long long.) I > think a warning for a (long long *)p cast might be surprising in that > case. > Well, yes this does get a warning. But doesn't that cast then violate the underlying alignment requirement of long long* ? Of course there is probably a reason why -Wcast-align is not enabled by default, and likewise this warning emits a fair amount of false positives, but nevertheless I think it is often worth looking at the places where this warning flags a possible alignment issue. However, neither -Wcast-align nor -Wcast-align=strict are enabled unless explicitly requested. Bernd.
On Wed, 13 Sep 2017, Bernd Edlinger wrote: > On 09/13/17 19:06, Joseph Myers wrote: > > What does this warning do in cases where a type has different alignments > > inside and outside structs? I'm thinking of something like > > > > struct s { long long x; } *p; > > /* ... */ > > (long long *)p > > > > on 32-bit x86 - where long long's preferred alignment is 8 bytes, but in > > structures it's 4 bytes. (Likewise for double in place of long long.) I > > think a warning for a (long long *)p cast might be surprising in that > > case. > > > > Well, yes this does get a warning. But doesn't that cast then violate > the underlying alignment requirement of long long* ? That's the difference between preferred alignment (__alignof__) and alignment required in all contexts (C11 _Alignof). The above seems valid, just like it's valid to take the address of a long long struct element. That is, the alignment for the target of a pointer to long long is really 4 bytes here, even though the alignment for a standalone long long object is 8 bytes. And there's a case for the warning to look at the required alignment in all contexts, not TYPE_ALIGN.
On 09/13/17 22:03, Joseph Myers wrote: > On Wed, 13 Sep 2017, Bernd Edlinger wrote: > >> On 09/13/17 19:06, Joseph Myers wrote: >>> What does this warning do in cases where a type has different alignments >>> inside and outside structs? I'm thinking of something like >>> >>> struct s { long long x; } *p; >>> /* ... */ >>> (long long *)p >>> >>> on 32-bit x86 - where long long's preferred alignment is 8 bytes, but in >>> structures it's 4 bytes. (Likewise for double in place of long long.) I >>> think a warning for a (long long *)p cast might be surprising in that >>> case. >>> >> >> Well, yes this does get a warning. But doesn't that cast then violate >> the underlying alignment requirement of long long* ? > > That's the difference between preferred alignment (__alignof__) and > alignment required in all contexts (C11 _Alignof). The above seems valid, > just like it's valid to take the address of a long long struct element. > That is, the alignment for the target of a pointer to long long is really > 4 bytes here, even though the alignment for a standalone long long object > is 8 bytes. And there's a case for the warning to look at the required > alignment in all contexts, not TYPE_ALIGN. > So you suggest to use min_align_of_type instead of TYPE_ALIGN. That would also make sense for the traditional -Wcast-align on strict-alignment targets, right? Thanks, Bernd.
On Wed, 13 Sep 2017, Bernd Edlinger wrote: > So you suggest to use min_align_of_type instead of TYPE_ALIGN. > > That would also make sense for the traditional -Wcast-align on > strict-alignment targets, right? Yes, and yes (though I'm not sure if any strict-alignment targets have this peculiarity).
Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 251617) +++ gcc/c/c-typeck.c (working copy) @@ -5578,7 +5578,7 @@ build_c_cast (location_t loc, tree type, tree expr } /* Warn about possible alignment problems. */ - if (STRICT_ALIGNMENT + if ((STRICT_ALIGNMENT || warn_cast_align == 2) && TREE_CODE (type) == POINTER_TYPE && TREE_CODE (otype) == POINTER_TYPE && TREE_CODE (TREE_TYPE (otype)) != VOID_TYPE Index: gcc/common.opt =================================================================== --- gcc/common.opt (revision 251617) +++ gcc/common.opt (working copy) @@ -564,6 +564,10 @@ Wcast-align Common Var(warn_cast_align) Warning Warn about pointer casts which increase alignment. +Wcast-align=strict +Common Var(warn_cast_align,2) Warning +Warn about pointer casts which increase alignment. + Wcpp Common Var(warn_cpp) Init(1) Warning Warn when a #warning directive is encountered. Index: gcc/cp/typeck.c =================================================================== --- gcc/cp/typeck.c (revision 251617) +++ gcc/cp/typeck.c (working copy) @@ -7265,8 +7265,8 @@ build_reinterpret_cast_1 (tree type, tree expr, bo complain)) return error_mark_node; /* Warn about possible alignment problems. */ - if (STRICT_ALIGNMENT && warn_cast_align - && (complain & tf_warning) + if ((STRICT_ALIGNMENT || warn_cast_align == 2) + && (complain & tf_warning) && !VOID_TYPE_P (type) && TREE_CODE (TREE_TYPE (intype)) != FUNCTION_TYPE && COMPLETE_TYPE_P (TREE_TYPE (type)) @@ -7273,7 +7273,7 @@ build_reinterpret_cast_1 (tree type, tree expr, bo && COMPLETE_TYPE_P (TREE_TYPE (intype)) && TYPE_ALIGN (TREE_TYPE (type)) > TYPE_ALIGN (TREE_TYPE (intype))) warning (OPT_Wcast_align, "cast from %qH to %qI " - "increases required alignment of target type", intype, type); + "increases required alignment of target type", intype, type); /* We need to strip nops here, because the front end likes to create (int *)&a for array-to-pointer decay, instead of &a[0]. */ @@ -7447,6 +7447,14 @@ build_const_cast_1 (tree dst_type, tree expr, tsub the user is making a potentially unsafe cast. */ check_for_casting_away_constness (src_type, dst_type, CAST_EXPR, complain); + /* ??? comp_ptr_ttypes_const ignores TYPE_ALIGN. */ + if ((STRICT_ALIGNMENT || warn_cast_align == 2) + && (complain & tf_warning) + && TYPE_ALIGN (TREE_TYPE (dst_type)) + > TYPE_ALIGN (TREE_TYPE (src_type))) + warning (OPT_Wcast_align, "cast from %qH to %qI " + "increases required alignment of target type", + src_type, dst_type); } if (reference_type) { Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 251617) +++ gcc/doc/invoke.texi (working copy) @@ -266,7 +266,8 @@ Objective-C and Objective-C++ Dialects}. -Wno-attributes -Wbool-compare -Wbool-operation @gol -Wno-builtin-declaration-mismatch @gol -Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol --Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align -Wcast-qual @gol +-Wc++-compat -Wc++11-compat -Wc++14-compat @gol +-Wcast-align -Wcast-align=strict -Wcast-qual @gol -Wchar-subscripts -Wchkp -Wcatch-value -Wcatch-value=@var{n} @gol -Wclobbered -Wcomment -Wconditionally-supported @gol -Wconversion -Wcoverage-mismatch -Wno-cpp -Wdangling-else -Wdate-time @gol @@ -5923,6 +5924,12 @@ target is increased. For example, warn if a @code an @code{int *} on machines where integers can only be accessed at two- or four-byte boundaries. +@item -Wcast-align=strict +@opindex Wcast-align=strict +Warn whenever a pointer is cast such that the required alignment of the +target is increased. For example, warn if a @code{char *} is cast to +an @code{int *} regardless of the target machine. + @item -Wwrite-strings @opindex Wwrite-strings @opindex Wno-write-strings Index: gcc/testsuite/c-c++-common/Wcast-align.c =================================================================== --- gcc/testsuite/c-c++-common/Wcast-align.c (revision 0) +++ gcc/testsuite/c-c++-common/Wcast-align.c (working copy) @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-Wcast-align=strict" } */ + +typedef char __attribute__ ((__aligned__(__BIGGEST_ALIGNMENT__))) c; +typedef struct __attribute__ ((__aligned__(__BIGGEST_ALIGNMENT__))) +{ + char x; +} d; + +char *x; +c *y; +d *z; + +void +foo (void) +{ + y = (c *) x; /* { dg-warning "alignment" } */ + z = (d *) x; /* { dg-warning "alignment" } */ +}
Hi, as you know we have a -Wcast-align warning which works only for STRICT_ALIGNMENT targets. But occasionally it would be nice to be able to switch this warning on even for other targets. Therefore I would like to add a strict version of this option which can be invoked with -Wcast-align=strict. With the only difference that it does not depend on STRICT_ALIGNMENT. I used the code from check_effective_target_non_strict_align in target-supports.exp for the first version of the test case, where we have this: return [check_no_compiler_messages non_strict_align assembly { char *y; typedef char __attribute__ ((__aligned__(__BIGGEST_ALIGNMENT__))) c; c *z; void foo(void) { z = (c *) y; } } "-Wcast-align"] ... and to my big surprise it did _not_ work for C++ as-is, because same_type_p considers differently aligned types identical, and therefore cp_build_c_cast tries the conversion first via a const_cast which succeeds, but did not emit the cast-align warning in this case. As a work-around I had to check the alignment in build_const_cast_1 as well. Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. gcc: 2017-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de> * common.opt (Wcast-align=strict): New warning option. * doc/invoke.texi: Document -Wcast-align=strict. c: 2017-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-typeck.c (build_c_cast): Implement -Wcast-align=strict. cp: 2017-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de> * typeck.c (build_reinterpret_cast_1, build_const_cast_1): Implement -Wcast-align=strict. testsuite: 2017-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-c++-common/Wcast-align.c: New test.