Ping^3: Patch: Implementation of -Wstrict-aliasing, take 2

[Diego Novillo]

Silvius Rus wrote on 03/15/07 19:27:

> @@ -393,7 +393,7 @@ c_common_handle_option (size_t scode, co
>        if (c_dialect_cxx ())
>  	warn_sign_compare = value;
>        warn_switch = value;
> -      warn_strict_aliasing = value;
> +      set_Wstrict_aliasing (value);

Mixed capitalization like this is confusing.  why not just
set_warn_strict_aliasing for consistency?

>  This option is only active when @option{-fstrict-aliasing} is active.
>  It warns about code which might break the strict aliasing rules that the
> -compiler is using for optimization.  This warning catches more cases than
> -@option{-Wstrict-aliasing}, but it will also give a warning for some ambiguous
> -cases that are safe.
> +compiler is using for optimization.  
> +Higher levels correspond to higher accuracy (fewer false positives).
> +Higher levels also correspond to more effort, similar to the way -O works.
> +-Wstrict-aliasing is equivalent to -Wstrict-aliasing=3.
> +Levels 1-3 are fully functional.  Level 4 is experimental, and  
> +includes checks for aliasing of unnamed objects, such as those created
> +by malloc.  Level 4 will run, and is most effective, with -fno-strict-aliasing.

We also need a clear notice stating that this will *only* work with
optimizations enabled.  A description of each level adapted from
tree-ssa-alias-warnings.c should also help (c-n-p and a removal of
internal code references is fine).

> +/* Return true when we should skip analysis for pointer PTR based on the
> +   fact that their alias information *PI is not considered relevant.  */
> +static bool
> +skip_this_pointer (tree ptr ATTRIBUTE_UNUSED, struct ptr_info_def *pi)

Vertical spacing.


> +/* Detect and report strict aliasing violation of named objects.  */
> +
> +static void
> +detect_strict_aliasing_named (void)
> +{

Not that I want to throw a monkey wrench to this scheme, but I've been
wondering whether it wouldn't make more sense to state this as a forward
problem.

So, now we walk the pointers and their pointed-to sets and then for
every <ptr, alias> that we find we see if they are type-compatible.
That's roughly the idea, right?

If we don't find them to be type-compatible, we go and find the first
type-punned dereference and warn.  In all of this, we are going up the
use-def chains every now and again (when dealing with unnamed aliases)
and even traversing the CFG linearly looking for references.

Why not just make a single CFG pass building on-the-side information
about dereferences that should be warned, together with locus
information and at the end emit the list of warnings?

Seems simpler to me.  It would avoid lots of back and forth, but maybe
there's a reason why you did it this way.


> +/* Print some debug information specific to -Wstrict-aliasing.  */
> +
> +static void
> +maybe_print_some_debug_info (void)
> +{
> +#ifdef _DEBUG_TREE_SSA_ALIAS_WARNING
> +  fprintf (stderr, "\nFrom  warn_strict_aliasing_dataflow_insensitive:\n");
> +  print_generic_expr (stderr, current_function_decl, TDF_SLIM);
> +  fprintf (stderr, ":\n");
> +  dump_function_to_file (current_function_decl, stderr, TDF_SLIM);
> +  dump_alias_info (stderr);
> +  dump_referenced_vars (stderr);
> +  dump_points_to_info (stderr);
> +#endif
> +}
> +

I don't think we need this.  We already have all this information
printed by the dump at the end of compute_may_aliases.  One thing that
may help is to create a new TDF_* flag and sprinkle 'dump_flags & TDF_*'
throughout the strict warning code.