Fix DECL_ABSTRACT_P/BLOCK_ABSTRACT handling in dwarf2out.c (PR middle-end/64937)

Hi!

On the following testcase we fail -fcompare-debug, because in .LASAN0
initializer we get a NOP_EXPR optimized away only in one of the cases.
The reason is that cgraph* is looking at DECL_ABSTRACT_P flag of
__PRETTY_FUNCTION__ and that flag is different between -g and -g0.
The DECL_ABSTRACT_P is normally set by the FE on some decls (common to -g
and -g0), but then dwarf2out_abstract_function can set/reset it in a weird
way, leaving it in different state from what it has been in initially.

The dwarf2out.c code pretty much assumes that if some decl is
DECL_ABSTRACT_P, then all the blocks in it and all the VAR_DECLs in
BLOCK_VARS and parameters etc. will necessarily be equal to that, but that
is usually not the case.

So, this patch changes it, so that it always reverts it to the original
state (temporarily it is set as before).

Bootstrapped/regtested on x86_64-linux and i686-linux, additionally I've
checked the debug info of cc1* and various *.so* libraries created during
gcc bootstrap and the patch doesn't result in any debug info differences.
Ok for trunk?

2015-02-05  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/64937
	* dwarf2out.c (set_block_abstract_flags, set_decl_abstract_flags):
	Replace setting argument with abstract_vec, always set BLOCK_ABSTRACT
	or DECL_ABSTRACT_P flags to 1 rather than to setting, and if it wasn't
	1 before, push it to abstract_vec.
	(dwarf2out_abstract_function): Adjust caller.  Don't call
	set_decl_abstract_flags second time, instead clear BLOCK_ABSTRACT or
	DECL_ABSTRACT_P flags for all abstract_vec elts.

	* g++.dg/asan/pr64937.C: New test.


	Jakub

On February 5, 2015 4:42:50 PM CET, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>On the following testcase we fail -fcompare-debug, because in .LASAN0
>initializer we get a NOP_EXPR optimized away only in one of the cases.
>The reason is that cgraph* is looking at DECL_ABSTRACT_P flag of
>__PRETTY_FUNCTION__ and that flag is different between -g and -g0.
>The DECL_ABSTRACT_P is normally set by the FE on some decls (common to
>-g
>and -g0), but then dwarf2out_abstract_function can set/reset it in a
>weird
>way, leaving it in different state from what it has been in initially.
>
>The dwarf2out.c code pretty much assumes that if some decl is
>DECL_ABSTRACT_P, then all the blocks in it and all the VAR_DECLs in
>BLOCK_VARS and parameters etc. will necessarily be equal to that, but
>that
>is usually not the case.
>
>So, this patch changes it, so that it always reverts it to the original
>state (temporarily it is set as before).
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, additionally
>I've
>checked the debug info of cc1* and various *.so* libraries created
>during
>gcc bootstrap and the patch doesn't result in any debug info
>differences.
>Ok for trunk?

Looks good to me. I wonder if this will also help Aldyh...

I also wonder what it takes to make dwarf2out use DECL_ABSTRACT_P || force_abstract with maintaining that new state and whether that would be both cheaper and cleaner?

Thanks,
Richard.

>2015-02-05  Jakub Jelinek  <jakub@redhat.com>
>
>	PR middle-end/64937
>	* dwarf2out.c (set_block_abstract_flags, set_decl_abstract_flags):
>	Replace setting argument with abstract_vec, always set BLOCK_ABSTRACT
>	or DECL_ABSTRACT_P flags to 1 rather than to setting, and if it wasn't
>	1 before, push it to abstract_vec.
>	(dwarf2out_abstract_function): Adjust caller.  Don't call
>	set_decl_abstract_flags second time, instead clear BLOCK_ABSTRACT or
>	DECL_ABSTRACT_P flags for all abstract_vec elts.
>
>	* g++.dg/asan/pr64937.C: New test.
>
>--- gcc/dwarf2out.c.jj	2015-02-04 23:36:33.000000000 +0100
>+++ gcc/dwarf2out.c	2015-02-05 13:19:41.401748148 +0100
>@@ -18062,7 +18062,7 @@ gen_type_die_for_member (tree type, tree
>/* Forward declare these functions, because they are mutually recursive
>   with their set_block_* pairing functions.  */
> static void set_decl_origin_self (tree);
>-static void set_decl_abstract_flags (tree, int);
>+static void set_decl_abstract_flags (tree, vec<tree> &);
> 
>/* Given a pointer to some BLOCK node, if the BLOCK_ABSTRACT_ORIGIN for
>the
>given BLOCK node is NULL, set the BLOCK_ABSTRACT_ORIGIN for the node so
>@@ -18135,59 +18135,72 @@ set_decl_origin_self (tree decl)
>     }
> }
> >
>-/* Given a pointer to some BLOCK node, and a boolean value to set the
>-   "abstract" flags to, set that value into the BLOCK_ABSTRACT flag
>for
>-   the given block, and for all local decls and all local sub-blocks
>-   (recursively) which are contained therein.  */
>+/* Given a pointer to some BLOCK node, set the BLOCK_ABSTRACT flag to
>1
>+   and if it wasn't 1 before, push it to abstract_vec vector.
>+   For all local decls and all local sub-blocks (recursively) do it
>+   too.  */
> 
> static void
>-set_block_abstract_flags (tree stmt, int setting)
>+set_block_abstract_flags (tree stmt, vec<tree> &abstract_vec)
> {
>   tree local_decl;
>   tree subblock;
>   unsigned int i;
> 
>-  BLOCK_ABSTRACT (stmt) = setting;
>+  if (!BLOCK_ABSTRACT (stmt))
>+    {
>+      abstract_vec.safe_push (stmt);
>+      BLOCK_ABSTRACT (stmt) = 1;
>+    }
> 
>   for (local_decl = BLOCK_VARS (stmt);
>        local_decl != NULL_TREE;
>        local_decl = DECL_CHAIN (local_decl))
>     if (! DECL_EXTERNAL (local_decl))
>-      set_decl_abstract_flags (local_decl, setting);
>+      set_decl_abstract_flags (local_decl, abstract_vec);
> 
>   for (i = 0; i < BLOCK_NUM_NONLOCALIZED_VARS (stmt); i++)
>     {
>       local_decl = BLOCK_NONLOCALIZED_VAR (stmt, i);
>  if ((TREE_CODE (local_decl) == VAR_DECL && !TREE_STATIC (local_decl))
> 	  || TREE_CODE (local_decl) == PARM_DECL)
>-	set_decl_abstract_flags (local_decl, setting);
>+	set_decl_abstract_flags (local_decl, abstract_vec);
>     }
> 
>   for (subblock = BLOCK_SUBBLOCKS (stmt);
>        subblock != NULL_TREE;
>        subblock = BLOCK_CHAIN (subblock))
>-    set_block_abstract_flags (subblock, setting);
>+    set_block_abstract_flags (subblock, abstract_vec);
> }
> 
>-/* Given a pointer to some ..._DECL node, and a boolean value to set
>the
>-   "abstract" flags to, set that value into the DECL_ABSTRACT_P flag
>for the
>-   given decl, and (in the case where the decl is a FUNCTION_DECL)
>also
>-   set the abstract flags for all of the parameters, local vars, local
>-   blocks and sub-blocks (recursively) to the same setting.  */
>+/* Given a pointer to some ..._DECL node, set DECL_ABSTRACT_P flag on
>it
>+   to 1 and if it wasn't 1 before, push to abstract_vec vector.
>+   In the case where the decl is a FUNCTION_DECL also set the abstract
>+   flags for all of the parameters, local vars, local
>+   blocks and sub-blocks (recursively).  */
> 
> static void
>-set_decl_abstract_flags (tree decl, int setting)
>+set_decl_abstract_flags (tree decl, vec<tree> &abstract_vec)
> {
>-  DECL_ABSTRACT_P (decl) = setting;
>+  if (!DECL_ABSTRACT_P (decl))
>+    {
>+      abstract_vec.safe_push (decl);
>+      DECL_ABSTRACT_P (decl) = 1;
>+    }
>+
>   if (TREE_CODE (decl) == FUNCTION_DECL)
>     {
>       tree arg;
> 
>       for (arg = DECL_ARGUMENTS (decl); arg; arg = DECL_CHAIN (arg))
>-	DECL_ABSTRACT_P (arg) = setting;
>+	if (!DECL_ABSTRACT_P (arg))
>+	  {
>+	    abstract_vec.safe_push (arg);
>+	    DECL_ABSTRACT_P (arg) = 1;
>+	  }
>       if (DECL_INITIAL (decl) != NULL_TREE
> 	  && DECL_INITIAL (decl) != error_mark_node)
>-	set_block_abstract_flags (DECL_INITIAL (decl), setting);
>+	set_block_abstract_flags (DECL_INITIAL (decl), abstract_vec);
>     }
> }
> 
>@@ -18200,7 +18213,6 @@ dwarf2out_abstract_function (tree decl)
>   dw_die_ref old_die;
>   tree save_fn;
>   tree context;
>-  int was_abstract;
>   hash_table<decl_loc_hasher> *old_decl_loc_table;
>   hash_table<dw_loc_list_hasher> *old_cached_dw_loc_list_table;
>   int old_call_site_count, old_tail_call_site_count;
>@@ -18242,11 +18254,16 @@ dwarf2out_abstract_function (tree decl)
>   save_fn = current_function_decl;
>   current_function_decl = decl;
> 
>-  was_abstract = DECL_ABSTRACT_P (decl);
>-  set_decl_abstract_flags (decl, 1);
>+  auto_vec<tree, 64> abstract_vec;
>+  set_decl_abstract_flags (decl, abstract_vec);
>   dwarf2out_decl (decl);
>-  if (! was_abstract)
>-    set_decl_abstract_flags (decl, 0);
>+  unsigned int i;
>+  tree t;
>+  FOR_EACH_VEC_ELT (abstract_vec, i, t)
>+    if (TREE_CODE (t) == BLOCK)
>+      BLOCK_ABSTRACT (t) = 0;
>+    else
>+      DECL_ABSTRACT_P (t) = 0;
> 
>   current_function_decl = save_fn;
>   decl_loc_table = old_decl_loc_table;
>--- gcc/testsuite/g++.dg/asan/pr64937.C.jj	2015-02-05
>13:22:04.510353994 +0100
>+++ gcc/testsuite/g++.dg/asan/pr64937.C	2015-02-05 13:21:44.000000000
>+0100
>@@ -0,0 +1,30 @@
>+// PR middle-end/64937
>+// { dg-do compile }
>+// { dg-options "-fsanitize=address -fcompare-debug" }
>+
>+namespace foo_aux {
>+  struct BarParser { };
>+}
>+extern "C" {
>+  extern void __assert_fail (__const char *__assertion, __const char
>*__file,
>+                             unsigned int __line, __const char
>*__function);
>+}
>+namespace foo {
>+  class BarBox {
>+  public:
>+    BarBox (int xl = 0, int yl = 0) { }
>+  };
>+  class BarFoo {
>+  public:
>+    explicit BarFoo (BarBox box) {
>+      ((_orig_mask) ? static_cast < void >(0) :
>+       __assert_fail ("_orig_mask", "foo.h", 159,
>__PRETTY_FUNCTION__));
>+    }
>+    BarBox *_orig_mask;
>+  };
>+}
>+static void
>+ProcessOp (foo_aux::BarParser * p, int xl, int yr)
>+{
>+  foo::BarFoo tiles (foo::BarBox (xl, yr));
>+}
>
>	Jakub

On Fri, Feb 06, 2015 at 09:44:40AM +0100, Richard Biener wrote:
> Looks good to me. I wonder if this will also help Aldyh...
> 
> I also wonder what it takes to make dwarf2out use DECL_ABSTRACT_P ||
> force_abstract with maintaining that new state and whether that would be
> both cheaper and cleaner?

I think that wouldn't be so easy, because we don't make DECL_ABSTRACT_P
all decls we see.  In particular we leave out e.g. externs, and handle
differently some non-localized vars. vs. local ones.  I think a global flag
couldn't handle this.

	Jakub

Richard Biener <richard.guenther@gmail.com> writes:

> Looks good to me. I wonder if this will also help Aldyh...

Hmmm, I ran into something very similar.  We solved it on the
debug-early branch by avoiding recursively setting DECL_ABSTRACT_P if
the parent was *not* DECL_ABSTRACT_P.  Like this:

@@ -18274,7 +18312,8 @@ dwarf2out_abstract_function (tree decl)
   current_function_decl = decl;
 
   was_abstract = DECL_ABSTRACT_P (decl);
-  set_decl_abstract_flags (decl, 1);
+  if (!was_abstract)
+    set_decl_abstract_flags (decl, 1);
   dwarf2out_decl (decl);
   if (! was_abstract)

Does this help, or do you need to keep track of everything you changed?

I'll say it again, I think setting tree flags behind everyone's back as
we call dwarf2out_decl() _again_ seems like a broken design.  But I
think Jason mentioned that creating new tree instances for the different
variants was a no-go.  Anyways...

Aldy