Re: [patch] Fix GC issue triggered by arithmetic overflow checking

[Richard Biener <richard.guenther@gmail.com>]

On Sat, Oct 8, 2016 at 8:56 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> adding patterns for unsigned arithmetic overflow checking in a back-end can
> have unexpected fallout because of a latent GC issue: when they are present,
> GIMPLE optimization passes can create complex (math. sense) types at will by
> invoking build_complex_type.  Now build_complex_type goes through the type
> caonicalization hashtable, which is GC-ed, so its behavior depends on the
> actual collection points.
>
> The other type-building functions present in tree.c do the same so no big deal
> but build_complex_type is special because it also does:
>
>   /* We need to create a name, since complex is a fundamental type.  */
>   if (! TYPE_NAME (t))
>     {
>       const char *name;
>       if (component_type == char_type_node)
>         name = "complex char";
>       else if (component_type == signed_char_type_node)
>         name = "complex signed char";
>       else if (component_type == unsigned_char_type_node)
>         name = "complex unsigned char";
>       else if (component_type == short_integer_type_node)
>         name = "complex short int";
>       else if (component_type == short_unsigned_type_node)
>         name = "complex short unsigned int";
>       else if (component_type == integer_type_node)
>         name = "complex int";
>       else if (component_type == unsigned_type_node)
>         name = "complex unsigned int";
>       else if (component_type == long_integer_type_node)
>         name = "complex long int";
>       else if (component_type == long_unsigned_type_node)
>         name = "complex long unsigned int";
>       else if (component_type == long_long_integer_type_node)
>         name = "complex long long int";
>       else if (component_type == long_long_unsigned_type_node)
>         name = "complex long long unsigned int";
>       else
>         name = 0;
>
>       if (name != 0)
>         TYPE_NAME (t) = build_decl (UNKNOWN_LOCATION, TYPE_DECL,
>                                     get_identifier (name), t);
>     }
>
> so it creates a DECL node every time a new canonical complex type is created,
> bumping the DECL_UID counter in the process.  Which means that the DECL_UID
> counter is sensitive to the collection points, which in turn means that the
> result of algorithms depending on the DECL_UID counter also is.
>
> This for example resulted in a bootstrap comparison failure on a SPARC/Solaris
> machine doing a strict stage2/stage3 comparison because the contents of the
> .debug_loc section were different: location lists computed by var-tracking
> were slightly different because of a different hashing.
>
> I'm not sure whether the hashing done by var-tracking should be sensitive to
> the DECL_UID of nodes or not, but I think that having the DECL_UID counter
> depend on the collection points is highly undesirable, so the attached patch
> attempts to prevent it; it at least fixed the bootstrap comparison failure.

I believe the rule is that you might only depend on the order of objects
with respect to their DECL_UID, not the actual value of the DECL_UID.
As var-tracking shouldn't look at TYPE_DECLs (?) it's probably a latent
var-tracking bug as well.

> Tested on x86_64-suse-linux, OK for the mainline?

I'd prefer the named parameter to be defaulted to false and the few
places in the FEs fixed (eventually that name business should be
handled like names for nodes like integer_type_node -- I see no
reason why build_complex_type should have this special-case at all!
That is, why are the named vairants in the type hash in the first place?)

Richard.

>
> 2016-10-08  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree.h (build_complex_type): Add second parameter with default.
>         * builtins.c (expand_builtin_cexpi): Pass false in call to above.
>         (fold_builtin_sincos): Likewise.
>         (fold_builtin_arith_overflow): Likewise.
>         * gimple-fold.c (fold_builtin_atomic_compare_exchange): Likewise.
>         (gimple_fold_call): Likewise.
>         * stor-layout.c (bitwise_type_for_mode): Likewise.
>         * tree-ssa-dce.c (maybe_optimize_arith_overflow): Likewise.
>         * tree-ssa-math-opts.c (match_uaddsub_overflow): Likewise.
>         * tree.c (build_complex): Likewise.
>         (build_complex_type): Add NAMED second parameter and adjust recursive
>         call.  Create a TYPE_DECL only if NAMED is true.
>
> --
> Eric Botcazou