Re: Patch: New implementation of -Wstrict-aliasing

[Silvius Rus <rus at google dot com>]

Andrew Pinski wrote:
> On Fri, 2007-01-19 at 18:12 -0800, Silvius Rus wrote:
>   
> > -  return *((int *)buf); /* { dg-warning "strict-aliasing" } */
> > +  return *((int *)buf); /* { dg-bogus "XXX: should really be a
> > warning" } */
> >     
>
> If this should be a warning and we don't warn anymore, then this should
> be instead xfail and a bug should be filed after this patch goes in.
> the way to fail it is to do:
> /* { dg-warning "strict-aliasing" "" { xfail *-*-* } } */
>   
OK, will change it to xfail.
> One more comment about another testcase or two:
> g++.dg/warn/Wstrict-aliasing-bogus-char-2.C 
>
> that testcase is actually violating C/C++ aliasing rules.
> +  char buffer[4];
> +  int* pi = reinterpret_cast<int*> (buffer);  /* { dg-bogus "char" } */
> +  buffer[1] = 0;
> +  return *pi;
>
>   
That's true, my mistake.  It is similar to the first case above.  My 
current implementation is not dataflow sensitive, so it cannot tell 
between the definition and the use.  I will change it to xfail, subject 
to the same bug report as the first case above.
> Take the following C++ code example:
>
> char buffer[4];
> int* pi = reinterpret_cast<int*> (buffer);
> float* pi1 = reinterpret_cast<float*> (buffer);
> buffer[1] = 0;
> *pi1 = *pi;
> return *pi;
>
> Do we warn at all for this case?
>
>   
We should.  Unfortunately, a part of the implementation I sent was 
commented out because it generated a couple of wrong warnings during 
bootstrap.  It can solve the case above by finding two pointers, pi and 
pi1, of different types, and both referenced, and pointing to the same 
object.
pi_1, name memory tag: NMT.10, is dereferenced, points-to vars: { SFT.0 
SFT.1 SFT.2 SFT.3 }
pi1_2, name memory tag: NMT.10, is dereferenced, points-to vars: { SFT.0 
SFT.1 SFT.2 SFT.3 }
> There should be a warning for g
> ++.dg/warn/Wstrict-aliasing-bogus-union.C 
>
> union U {
>   int i;
>   float f;
> };
> float g(int *a, float *b)
> {
>   *a = 1;
>   return *b;
> }
>
> float foo () {
>   union U u;
>   return g(&u.i, &u.f);
> }
>
> The above code is undefined, even though unions are "holly" in GCC.
>
>   
Should we warn based on the standard rather than on what GCC 
guarantees?  Maybe we should not give unions a special treatment just 
for the purposes of the warning, since this could cause much trouble 
when porting code to other compilers.
> I would actually not use "strict" in the warning at all because it is
> wrong as "strict" is not really anything except saying GCC follows the C
> standard here (except we don't fully for character types).
>
>   
I'll take suggestions.  Should I have just kept type punning instead?
> You have a couple of style issues in your patch:
> +		 get_ref_type_str(rt1),
>
> space between the function name and '('.
>   
Will fix.
> Also even though you already make sure the warning is only emitted with
> -Wstrict-aliasing, it is still good to include Wstrict_aliasing as the
> first argument for warning so that people can do
> -Werror=Wstrict-aliasing as documented in the manual. I also think the warning should not really be using %s at all, there are
> pretty-printers for a reason, they really should be quoted if %s is to
> be used.  Also I noticed you will print out (unknown) if the decl is a
> RESULT_DECL, I rather not warn about any artificial decls at all, or
> maybe even ICE at that point with checking enabled because that means
> the middle-end is producing invalid stores/loads.
>   
It sounds like I have to read about diagnostics a bit more.  Will look 
into it and try to fix it properly.


Thank you!
Silvius