Miscompilation of __attribute__((constructor)) functions.

Patch below fixes a miscompilation observed whem building uclibc libpthread on 
a mips-linux system.

The story start with the ipa-split optimization, which turns:

void fn()
{
  if (cond) {
    DO_STUFF;
  }
}

into:

static void fn_helper()
{
  DO_STUFF;
}

void fn()
{
  if (cond)
    fn_helper();
}


The idea is that the new fn() wrapper is a good candidate for inlining, 
whereas the original fn is not.

The optimization uses cgraph function versioning.  The problem is that when we 
clone the cgraph node we propagate the DECL_STATIC_CONSTRUCTOR bit.  Thus both 
fn() and fn_helper() get called on startup.

When fn happens to be pthread_initialize we end up calling both the original 
and a clone with the have-I-already-done-this check removed. Much 
badness ensues.

Patch below fixes this by clearing the DECL_STATIC_{CON,DES}TRUCTOR bit when 
cloning a cgraph node - there's already logic to make sure we keep the 
original.  My guess is this bug is probably latent in other IPA passes.

Tested on mips-linux and bootstrap+test x86_64-linux
Ok?

Paul

2011-10-27  Paul Brook  <paul@codesourcery.com>

	gcc/
	* cgraphunit.c: Don't mark clones as static constructors.

	gcc/testsuite/
	* gcc.dg/constructor-1.c: New test.

On Thu, Oct 27, 2011 at 7:24 PM, Paul Brook <paul@codesourcery.com> wrote:
> Patch below fixes a miscompilation observed whem building uclibc libpthread on
> a mips-linux system.
>
> The story start with the ipa-split optimization, which turns:
>
> void fn()
> {
>  if (cond) {
>    DO_STUFF;
>  }
> }
>
> into:
>
> static void fn_helper()
> {
>  DO_STUFF;
> }
>
> void fn()
> {
>  if (cond)
>    fn_helper();
> }
>
>
> The idea is that the new fn() wrapper is a good candidate for inlining,
> whereas the original fn is not.
>
> The optimization uses cgraph function versioning.  The problem is that when we
> clone the cgraph node we propagate the DECL_STATIC_CONSTRUCTOR bit.  Thus both
> fn() and fn_helper() get called on startup.
>
> When fn happens to be pthread_initialize we end up calling both the original
> and a clone with the have-I-already-done-this check removed. Much
> badness ensues.
>
> Patch below fixes this by clearing the DECL_STATIC_{CON,DES}TRUCTOR bit when
> cloning a cgraph node - there's already logic to make sure we keep the
> original.  My guess is this bug is probably latent in other IPA passes.
>
> Tested on mips-linux and bootstrap+test x86_64-linux
> Ok?

Ok if you move the clearing to after

  /* Generate a new name for the new version. */
  DECL_NAME (new_decl) = clone_function_name (old_decl, clone_name);
  SET_DECL_ASSEMBLER_NAME (new_decl, DECL_NAME (new_decl));
  SET_DECL_RTL (new_decl, NULL);

using new_decl directly, thus add

  /* When the old decl was a con-/destructor make sure the clone isn't.  */
  DECL_STATIC_CONSTRUCTOR(new_decl) = 0;
  DECL_STATIC_DESTRUCTOR(new_decl) = 0;

Thanks,
Richard.

> Paul
>
> 2011-10-27  Paul Brook  <paul@codesourcery.com>
>
>        gcc/
>        * cgraphunit.c: Don't mark clones as static constructors.
>
>        gcc/testsuite/
>        * gcc.dg/constructor-1.c: New test.
>
> Index: gcc/cgraphunit.c
> ===================================================================
> --- gcc/cgraphunit.c    (revision 180439)
> +++ gcc/cgraphunit.c    (working copy)
> @@ -2386,6 +2386,8 @@ cgraph_function_versioning (struct cgrap
>   new_version_node->local.externally_visible = 0;
>   new_version_node->local.local = 1;
>   new_version_node->lowered = true;
> +  DECL_STATIC_CONSTRUCTOR(new_version_node->decl) = 0;
> +  DECL_STATIC_DESTRUCTOR(new_version_node->decl) = 0;
>
>   /* Update the call_expr on the edges to call the new version node. */
>   update_call_expr (new_version_node);
> Index: gcc/testsuite/gcc.dg/constructor-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/constructor-1.c        (revision 0)
> +++ gcc/testsuite/gcc.dg/constructor-1.c        (revision 0)
> @@ -0,0 +1,37 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +/* The ipa-split pass pulls the body of the if(!x) block
> +   into a separate function to make foo a better inlining
> +   candidate.  Make sure this new function isn't also run
> +   as a static constructor.  */
> +
> +#include <stdlib.h>
> +
> +int x, y;
> +
> +void __attribute__((noinline))
> +bar(void)
> +{
> +  y++;
> +}
> +
> +void __attribute__((constructor))
> +foo(void)
> +{
> +  if (!x)
> +    {
> +      bar();
> +      y++;
> +    }
> +}
> +
> +int main()
> +{
> +  x = 1;
> +  foo();
> +  foo();
> +  if (y != 2)
> +    abort();
> +  exit(0);
> +}
>

> Ok if you move the clearing to after
> 
>   /* Generate a new name for the new version. */
>   DECL_NAME (new_decl) = clone_function_name (old_decl, clone_name);
>   SET_DECL_ASSEMBLER_NAME (new_decl, DECL_NAME (new_decl));
>   SET_DECL_RTL (new_decl, NULL);
> 
> using new_decl directly, thus add
> 
>   /* When the old decl was a con-/destructor make sure the clone isn't.  */
>   DECL_STATIC_CONSTRUCTOR(new_decl) = 0;
>   DECL_STATIC_DESTRUCTOR(new_decl) = 0;

Done, and applied.

Paul