On Thu, Oct 27, 2011 at 7:24 PM, Paul Brook <p...@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 <p...@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);
> +}
>
