[Bug c/63307] New: [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[Bug c/63307] New: [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

            Bug ID: 63307
           Summary: [4.9/5 Regression] Cilk+ breaks -fcompare-debug
                    bootstrap
           Product: gcc
           Version: 4.9.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
                CC: izamyatin at gmail dot com, kyukhin at gcc dot gnu.org

I've just tried a -fcompare-debug bootstrap, and one of the two reasons (other
was Ada) that it failed is Cilk+:
../configure --enable-languages=all,ada,obj-c++,lto,go
--enable-checking=release; GCC_COMPARE_DEBUG=1 make -j48 bootstrap > LOG 2>&1
&& GCC_COMPARE_DEBUG=1 make -j48 -k check > LOGC 2>&1; ../contrib/test_summary
> LOGT 2>&1
...
xg++: error: ../../../libcilkrts/runtime/cilk-abi-cilk-for.cpp: -fcompare-debug
failure (length)

>From what I can see, the bug is (at least) in c-family/cilk.c, where it seems
in all 4 traverse handlers on decl_map it creates decls, adds fields etc.

That is a big NO NO in GCC, hash table traversal order should never affect code
generation.  So, seeing changes like:
-          _cilk_spn_1 (low, grain, mid, body, loop_root_pedigree, data, w);
+          _cilk_spn_1 (data, body, loop_root_pedigree, w, mid, low, grain);
or:
-<built-in> (unsigned int D.3190, int D.3189, unsigned int D.3188, void
(*<T39b>) (void *, unsigned int, unsigned int) D.3187, struct
__cilkrts_pedigree * D.3186, void * D.3185, struct __cilkrts_worker * D.3184)
+<built-in> (void * D.3190, void (*<T39b>) (void *, unsigned int, unsigned int)
D.3189, struct __cilkrts_pedigree * D.3188, struct __cilkrts_worker * D.3187,
unsigned int D.3186, unsigned int D.3185, int D.3184)
in *.gimple dump between -g and -g0 is just wrong (and in this case it isn't
really debug related, just different order in the hash map).

If you don't have the decls (but, apparently you put also BLOCKs in there, what
else?) you want to traverse in some other data structure, you can e.g. traverse
the hash table but only collect the decls (do you need anything else, e.g. the
BLOCKs?, types, etc.) you saw into some vector, then qsort the vector (for
decls e.g. by DECL_UID, if you have also other trees, it might be harder) and
then traverse the sorted vector insert of traversing random order hash map.

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.9.2

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[izamyatin at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

--- Comment #1 from Igor Zamyatin <izamyatin at gmail dot com> ---
Would like to ask here first - will something like following be ok:


diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c
index bf549ad..f453bc5 100644
--- a/gcc/c-family/cilk.c
+++ b/gcc/c-family/cilk.c
@@ -36,6 +36,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "toplev.h" 
 #include "cgraph.h"
 #include "diagnostic.h"
+#include "vec.h"
 #include "cilk.h"

 enum add_variable_type {
@@ -332,15 +333,23 @@ create_cilk_helper_decl (struct wrapper_data *wd)
   return fndecl;
 }

+typedef struct
+{
+  tree parm;
+  tree arg;
+} decl_pair;
+
+static vec<decl_pair> vec_arglist;
+
 /* A function used by walk tree to find wrapper parms.  */

 static bool
 wrapper_parm_cb (const void *key0, void **val0, void *data)
 {
-  struct wrapper_data *wd = (struct wrapper_data *) data;
   tree arg = * (tree *)&key0;
   tree val = (tree)*val0;
   tree parm;
+  decl_pair dp;

   if (val == error_mark_node || val == arg)
     return true;
@@ -370,25 +379,48 @@ wrapper_parm_cb (const void *key0, void **val0, void
*data)
     }
   else
     parm = val;
-  TREE_CHAIN (parm) = wd->parms;
-  wd->parms = parm;
-  wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (parm), wd->argtypes); 
-  wd->arglist = tree_cons (NULL_TREE, arg, wd->arglist); 
+
+  dp.parm = parm;
+  dp.arg = arg;
+  vec_arglist.safe_push(dp);
   return true;
 }

 /* This function is used to build a wrapper of a certain type.  */

+static int
+compare_decls (const void *a, const void *b)
+{
+    const decl_pair* t1 = (const decl_pair*) a;
+    const decl_pair* t2 = (const decl_pair*) b;
+
+    return DECL_UID(t1->arg) > DECL_UID(t2->arg);
+}
+
 static void
 build_wrapper_type (struct wrapper_data *wd)
 {
+  unsigned int j;
+  decl_pair * c;
   wd->arglist = NULL_TREE;
   wd->parms = NULL_TREE;
   wd->argtypes = void_list_node;

-  pointer_map_traverse (wd->decl_map, wrapper_parm_cb, wd);
+  vec_arglist.create (0);
+  pointer_map_traverse (wd->decl_map, wrapper_parm_cb, NULL);
   gcc_assert (wd->type != CILK_BLOCK_FOR);

+  vec_arglist.qsort(compare_decls);
+
+  FOR_EACH_VEC_ELT (vec_arglist, j, c)
+    {
+      TREE_CHAIN (c->parm) = wd->parms;
+      wd->parms = c->parm;
+      wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (c->parm), wd->argtypes);
+      wd->arglist = tree_cons (NULL_TREE, c->arg, wd->arglist);
+    }
+  vec_arglist.release();
+
   /* Now build a function.
      Its return type is void (all side effects are via explicit parameters).
      Its parameters are WRAPPER_PARMS with type WRAPPER_TYPES.


Bootstrapped successfully with GCC_COMPARE_DEBUG=1

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Igor Zamyatin from comment #1)
> Would like to ask here first - will something like following be ok:

> +typedef struct
> +{
> +  tree parm;
> +  tree arg;
> +} decl_pair;
> +
> +static vec<decl_pair> vec_arglist;

Just use struct cilk_decl_pair, no need for typedef in C++ here, and
try to avoid ODR issues.  Also, why a static variable?  Then you supposedly
would need to GTY handle it etc., which is IMHO undesirable.

>  static bool
>  wrapper_parm_cb (const void *key0, void **val0, void *data)
>  {
> -  struct wrapper_data *wd = (struct wrapper_data *) data;
>    tree arg = * (tree *)&key0;
>    tree val = (tree)*val0;
>    tree parm;
> +  decl_pair dp;
>  
>    if (val == error_mark_node || val == arg)
>      return true;
> @@ -370,25 +379,48 @@ wrapper_parm_cb (const void *key0, void **val0, void
> *data)
>      }
>    else
>      parm = val;
> -  TREE_CHAIN (parm) = wd->parms;
> -  wd->parms = parm;
> -  wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (parm), wd->argtypes); 
> -  wd->arglist = tree_cons (NULL_TREE, arg, wd->arglist); 
> +
> +  dp.parm = parm;
> +  dp.arg = arg;
> +  vec_arglist.safe_push(dp);

Formatting, missing space before (.  But more importantly, the diagnostics
still be in random order.  So, I'd strongly suggest to move also the
diagnostics and ADDR_EXPR building etc. into a loop that walks the sorted
vector.

>    return true;
>  }
>  
>  /* This function is used to build a wrapper of a certain type.  */
>  
> +static int
> +compare_decls (const void *a, const void *b)
> +{
> +    const decl_pair* t1 = (const decl_pair*) a;
> +    const decl_pair* t2 = (const decl_pair*) b;
> +
> +    return DECL_UID(t1->arg) > DECL_UID(t2->arg);

Formatting.  Space before *, not after, space before (, indentation 2 columns
rather than 4.

> +}
> +
>  static void
>  build_wrapper_type (struct wrapper_data *wd)
>  {
> +  unsigned int j;
> +  decl_pair * c;
>    wd->arglist = NULL_TREE;
>    wd->parms = NULL_TREE;
>    wd->argtypes = void_list_node;
>  
> -  pointer_map_traverse (wd->decl_map, wrapper_parm_cb, wd);
> +  vec_arglist.create (0);
> +  pointer_map_traverse (wd->decl_map, wrapper_parm_cb, NULL);
>    gcc_assert (wd->type != CILK_BLOCK_FOR);
>  
> +  vec_arglist.qsort(compare_decls);

Formatting.

> +
> +  FOR_EACH_VEC_ELT (vec_arglist, j, c)
> +    {
> +      TREE_CHAIN (c->parm) = wd->parms;
> +      wd->parms = c->parm;
> +      wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (c->parm),
> wd->argtypes);
> +      wd->arglist = tree_cons (NULL_TREE, c->arg, wd->arglist);
> +    }

As I said earlier, I'd do diagnostics and ADDR_EXPR creation in this loop too.

> +  vec_arglist.release();

Formatting.  You could use auto_vec, perhaps with some stack allocated initial
buffer if you think say 16 vector elements would be typically enough.

Also, what about all the remaining 3 callbacks that create or may create decls
and have the same problem?  for_local_cb, wrapper_local_cb and
declare_one_free_variable.

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[izamyatin at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

--- Comment #3 from Igor Zamyatin <izamyatin at gmail dot com> ---
(In reply to Jakub Jelinek from comment #2)

> 
> > +  vec_arglist.release();
> 
> Formatting.  You could use auto_vec, perhaps with some stack allocated
> initial buffer if you think say 16 vector elements would be typically enough.

Is it ok to have auto_vec declaration outside the routine?

> 
> Also, what about all the remaining 3 callbacks that create or may create
> decls and have the same problem?  for_local_cb, wrapper_local_cb and
> declare_one_free_variable.

These are callbacks that seem to be safe in the sense of random ordering - 
perform some 1 to 1 mapping

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Igor Zamyatin from comment #3)
> (In reply to Jakub Jelinek from comment #2)
> 
> > 
> > > +  vec_arglist.release();
> > 
> > Formatting.  You could use auto_vec, perhaps with some stack allocated
> > initial buffer if you think say 16 vector elements would be typically enough.
> 
> Is it ok to have auto_vec declaration outside the routine?

Why do you need to declare it outside of the routine?  That seems undesirable
to me.

> > Also, what about all the remaining 3 callbacks that create or may create
> > decls and have the same problem?  for_local_cb, wrapper_local_cb and
> > declare_one_free_variable.
> 
> These are callbacks that seem to be safe in the sense of random ordering - 
> perform some 1 to 1 mapping

I don't think so.  They copy declarations, i.e. create new declarations, and
the different ordering of their DECL_UID values may result in code generation
differences (e.g. various other spots in the compiler sort based on DECL_UIDs,
if you create them in pretty random order, you'll surely trigger some
-fcompare-debug (perhaps not with current limited testsuite coverage, but with
other tests).

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[izamyatin at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

--- Comment #5 from Igor Zamyatin <izamyatin at gmail dot com> ---
(In reply to Jakub Jelinek from comment #4)
> I don't think so.  They copy declarations, i.e. create new declarations, and
> the different ordering of their DECL_UID values may result in code
> generation differences (e.g. various other spots in the compiler sort based
> on DECL_UIDs,
> if you create them in pretty random order, you'll surely trigger some
> -fcompare-debug (perhaps not with current limited testsuite coverage, but
> with other tests).

Right, thanks for the clarification. Will prepare the whole patch then

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[iverbin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

--- Comment #6 from iverbin at gcc dot gnu.org ---
Author: iverbin
Date: Mon Oct 20 15:22:09 2014
New Revision: 216483

URL: https://gcc.gnu.org/viewcvs?rev=216483&root=gcc&view=rev
Log:
    PR c/63307
gcc/c-family/
    * cilk.c: Include vec.h.
    (struct cilk_decls): New structure.
    (wrapper_parm_cb): Split this function to...
    (fill_decls_vec): ...this...
    (create_parm_list): ...and this.
    (compare_decls): New function.
    (for_local_cb): Remove.
    (wrapper_local_cb): Ditto.
    (build_wrapper_type): For now first traverse and fill vector of
    declarations then sort it and then deal with sorted vector.
    (cilk_outline): Ditto.
    (declare_one_free_variable): Ditto.

Modified:
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/cilk.c

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to iverbin from comment #6)
> Author: iverbin
> Date: Mon Oct 20 15:22:09 2014
> New Revision: 216483

This breaks the build as  wd->decl_map will always contain a BLOCK which does
not have an UID.

Please revert it as it is obvious you did not test it as a simple bootstrap
(with checking enabled which is default on the trunk) would have found this
issue.

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #8 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Note that the patch was reverted in r216502.

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.2                       |4.9.3

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.9.2 has been released.

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Wed Jan 21 21:23:04 2015
New Revision: 219969

URL: https://gcc.gnu.org/viewcvs?rev=219969&root=gcc&view=rev
Log:
    PR c/63307
    * cilk.c (fill_decls_vec): Only put decls into vector v.                    
    (compare_decls): Fix up formatting.

    * c-c++-common/cilk-plus/CK/pr63307.c: New test.

2015-01-21  Igor Zamyatin  <igor.zamyatin@intel.com>

    PR c/63307
    * cilk.c: Include vec.h.
    (struct cilk_decls): New structure.
    (wrapper_parm_cb): Split this function to...
    (fill_decls_vec): ...this...
    (create_parm_list): ...and this.
    (compare_decls): New function.
    (for_local_cb): Remove.
    (wrapper_local_cb): Ditto.
    (build_wrapper_type): For now first traverse and fill vector of
    declarations then sort it and then deal with sorted vector.
    (cilk_outline): Ditto.
    (declare_one_free_variable): Ditto.

Added:
    trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/pr63307.c
Modified:
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/cilk.c
    trunk/gcc/testsuite/ChangeLog

[Bug c/63307] [4.9 Regression] Cilk+ breaks -fcompare-debug bootstrap

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P1                          |P2
             Status|ASSIGNED                    |NEW
           Assignee|jakub at gcc dot gnu.org           |unassigned at gcc dot gnu.org

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Not a P1 for the branch, furthermore not easily backportable.

[Bug c/63307] [4.9 Regression] Cilk+ breaks -fcompare-debug bootstrap

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.9.3 has been released.

[Bug c/63307] [4.9 Regression] Cilk+ breaks -fcompare-debug bootstrap

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.9.3                       |4.9.4