[PATCH] Improve OpenMP debug info for threadprivate vars and shared local statics

[PATCH] Improve OpenMP debug info for threadprivate vars and shared local statics

[Jakub Jelinek]

Hi!

On testcases like:

program foo
  integer :: x, y
  common /blk/ x, y
  !$omp threadprivate (/blk/)
  x = 6
  y = 7
  !$omp parallel
  x = 8
  y = 9
  x = x + 1
  y = y + 1
  !$omp end parallel
end

and

int
main (void)
{
  static int thr;
  static int svar1, svar2;
  int avar1 = 0, avar2 = 0;
#pragma omp threadprivate (thr)
  thr = 18;
  thr++;
  svar1++;
  svar2++;
  avar1++;
  avar2++;
#pragma omp parallel shared(svar2, avar2)
  {
    thr = 16;
    thr++;
    {
      int vv = 6;
      vv++;
      #pragma omp atomic
        svar2++;
    }
    #pragma omp atomic
    avar2++;
  }
  svar1++;
  avar1++;
  return 0;
}

x and y in the first testcase and thr and svar2 aren't visible in the
debugger when in *omp_fn* function.  While in theory it should be possible
for the debugger to look at the containing function in the initial thread
(*omp_fn* is DW_AT_artificial) and find those vars there, that can take a
long time before it is implemented in gdb.

The following patch adds the referenced fn local threadprivate and statics
to the *omp_fn* DW_TAG_subprogram too.

Do you think that is a good idea?

Bootstrapped/regtested on x86_64-linux and i686-linux.

2010-07-22  Jakub Jelinek  <jakub@redhat.com>

	* gimplify.c (enum gimplify_omp_var_data): Add
	GOVD_THREADPRIVATE_WARNED.
	(gimplify_bind_expr): Add GOVD_LOCAL | GOVD_SEEN even for global vars.
	(omp_notice_threadprivate_variable): Note used threadprivate vars
	with current function's context in shared clauses.
	(gimplify_adjust_omp_clauses_1): Allow globals with current function's
	context in taskreg shared clause.
	* omp-low.c (lower_rec_input_clauses): For function-local is_global_var
	VAR_DECLs in shared clauses add a decl copy with DECL_VALUE_EXPR
	pointing to the original.

	* trans-openmp.c (gfc_omp_private_debug_clause): Return false for
	threadprivate decls.

	* gcc.dg/gomp/tls-3.c: New test.

--- gcc/fortran/trans-openmp.c.jj	2010-07-15 14:35:35.000000000 +0200
+++ gcc/fortran/trans-openmp.c	2010-07-22 12:02:26.000000000 +0200
@@ -352,6 +352,18 @@ gfc_omp_disregard_value_expr (tree decl,
 bool
 gfc_omp_private_debug_clause (tree decl, bool shared)
 {
+  if (TREE_STATIC (decl) || DECL_EXTERNAL (decl))
+    {
+      if (DECL_THREAD_LOCAL_P (decl))
+	return false;
+      if (DECL_HAS_VALUE_EXPR_P (decl))
+	{
+	  tree value = get_base_address (DECL_VALUE_EXPR (decl));
+	  if (value && DECL_P (value) && DECL_THREAD_LOCAL_P (value))
+	    return false;
+	}
+    }
+
   if (GFC_DECL_CRAY_POINTEE (decl))
     return true;
 
--- gcc/gimplify.c.jj	2010-07-16 17:55:08.000000000 +0200
+++ gcc/gimplify.c	2010-07-22 14:45:08.000000000 +0200
@@ -64,6 +64,7 @@ enum gimplify_omp_var_data
   GOVD_LOCAL = 128,
   GOVD_DEBUG_PRIVATE = 256,
   GOVD_PRIVATE_OUTER_REF = 512,
+  GOVD_THREADPRIVATE_WARNED = 1024,
   GOVD_DATA_SHARE_CLASS = (GOVD_SHARED | GOVD_PRIVATE | GOVD_FIRSTPRIVATE
 			   | GOVD_LASTPRIVATE | GOVD_REDUCTION | GOVD_LOCAL)
 };
@@ -1140,7 +1141,7 @@ gimplify_bind_expr (tree *expr_p, gimple
 	  struct gimplify_omp_ctx *ctx = gimplify_omp_ctxp;
 
 	  /* Mark variable as local.  */
-	  if (ctx && !is_global_var (t)
+	  if (ctx
 	      && (! DECL_SEEN_IN_BIND_EXPR_P (t)
 		  || splay_tree_lookup (ctx->variables,
 					(splay_tree_key) t) == NULL))
@@ -5520,17 +5521,36 @@ omp_notice_threadprivate_variable (struc
 {
   splay_tree_node n;
 
-  if (ctx->region_type != ORT_UNTIED_TASK)
+  while (ctx && ctx->region_type == ORT_WORKSHARE)
+    {
+      n = splay_tree_lookup (ctx->variables, (splay_tree_key)decl);
+      if (n != NULL)
+	{
+	  gcc_assert (n->value & GOVD_LOCAL);
+	  return false;
+	}
+      ctx = ctx->outer_context;
+    }
+  if (ctx == NULL)
     return false;
+
   n = splay_tree_lookup (ctx->variables, (splay_tree_key)decl);
   if (n == NULL)
+    n = splay_tree_insert (ctx->variables, (splay_tree_key)decl,
+			   DECL_CONTEXT (decl) == current_function_decl
+			   ? GOVD_SHARED | GOVD_SEEN : 0);
+  if (ctx->region_type == ORT_UNTIED_TASK
+      && (n->value & GOVD_THREADPRIVATE_WARNED) == 0)
     {
-      error ("threadprivate variable %qE used in untied task", DECL_NAME (decl));
+      error ("threadprivate variable %qE used in untied task",
+	     DECL_NAME (decl));
       error_at (ctx->location, "enclosing task");
-      splay_tree_insert (ctx->variables, (splay_tree_key)decl, 0);
+      n->value |= GOVD_THREADPRIVATE_WARNED;
     }
   if (decl2)
-    splay_tree_insert (ctx->variables, (splay_tree_key)decl2, 0);
+    splay_tree_insert (ctx->variables, (splay_tree_key)decl2,
+		       DECL_CONTEXT (decl2) == current_function_decl
+		       ? GOVD_SHARED | GOVD_SEEN : 0);
   return false;
 }
 
@@ -5958,7 +5978,9 @@ gimplify_adjust_omp_clauses_1 (splay_tre
 		break;
 	      ctx = ctx->outer_context;
 	    }
-	  if (ctx == NULL)
+	  if (ctx == NULL
+	      && (DECL_CONTEXT (decl) != current_function_decl
+		  || gimplify_omp_ctxp->region_type == ORT_WORKSHARE))
 	    return 0;
 	}
       code = OMP_CLAUSE_SHARED;
--- gcc/omp-low.c.jj	2010-07-16 17:55:08.000000000 +0200
+++ gcc/omp-low.c	2010-07-22 14:09:33.000000000 +0200
@@ -2248,6 +2248,17 @@ lower_rec_input_clauses (tree clauses, g
 		continue;
 	      break;
 	    case OMP_CLAUSE_SHARED:
+	      if (pass == 0
+		  && is_global_var (OMP_CLAUSE_DECL (c))
+		  && (DECL_CONTEXT (OMP_CLAUSE_DECL (c))
+		      == current_function_decl)
+		  && is_taskreg_ctx (ctx)
+		  && !DECL_IGNORED_P (OMP_CLAUSE_DECL (c)))
+		{
+		  new_var = omp_copy_decl_1 (OMP_CLAUSE_DECL (c), ctx);
+		  SET_DECL_VALUE_EXPR (new_var, OMP_CLAUSE_DECL (c));
+		  DECL_HAS_VALUE_EXPR_P (new_var) = 1;
+		}
 	      if (maybe_lookup_decl (OMP_CLAUSE_DECL (c), ctx) == NULL)
 		{
 		  gcc_assert (is_global_var (OMP_CLAUSE_DECL (c)));
--- gcc/testsuite/gcc.dg/gomp/tls-3.c.jj	2010-07-22 15:48:41.000000000 +0200
+++ gcc/testsuite/gcc.dg/gomp/tls-3.c	2010-07-22 15:48:47.000000000 +0200
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target tls_native } */
+
+int thr;
+#pragma omp threadprivate(thr)
+
+void
+foo (void)
+{
+  #pragma omp task untied	/* { dg-error "enclosing task" } */
+  {
+    static int thr2;
+    #pragma omp threadprivate(thr2)
+    static int thr3;
+    #pragma omp threadprivate(thr3)
+    thr++;	/* { dg-error "used in untied task" } */
+    thr2++;	/* { dg-error "used in untied task" } */
+    thr++;
+    thr2++;
+  }
+}

	Jakub

Re: [PATCH] Improve OpenMP debug info for threadprivate vars and shared local statics

[Richard Henderson]

On 07/22/2010 09:25 AM, Jakub Jelinek wrote:
> Do you think that is a good idea?
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux.
> 
> 2010-07-22  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* gimplify.c (enum gimplify_omp_var_data): Add
> 	GOVD_THREADPRIVATE_WARNED.
> 	(gimplify_bind_expr): Add GOVD_LOCAL | GOVD_SEEN even for global vars.
> 	(omp_notice_threadprivate_variable): Note used threadprivate vars
> 	with current function's context in shared clauses.
> 	(gimplify_adjust_omp_clauses_1): Allow globals with current function's
> 	context in taskreg shared clause.
> 	* omp-low.c (lower_rec_input_clauses): For function-local is_global_var
> 	VAR_DECLs in shared clauses add a decl copy with DECL_VALUE_EXPR
> 	pointing to the original.
> 
> 	* trans-openmp.c (gfc_omp_private_debug_clause): Return false for
> 	threadprivate decls.
> 
> 	* gcc.dg/gomp/tls-3.c: New test.

Yes, I think it's a good idea.

Ok.


r~

Re: [PATCH] Improve OpenMP debug info for threadprivate vars and shared local statics

[Roland McGrath]

> x and y in the first testcase and thr and svar2 aren't visible in the
> debugger when in *omp_fn* function.  While in theory it should be possible
> for the debugger to look at the containing function in the initial thread
> (*omp_fn* is DW_AT_artificial) and find those vars there, that can take a
> long time before it is implemented in gdb.
> 
> The following patch adds the referenced fn local threadprivate and statics
> to the *omp_fn* DW_TAG_subprogram too.

IMHO it would be better to do it without such duplication.  I think we
can beat the support out of the gdb people well enough, if the "proper"
indirect representations of all the info are there.

With 4.4.4(RH)-10, I see:

<subprogram name="main.omp_fn.0" prototyped=1 artificial=1 low_pc=0x4005e0 high_pc=0x4005fc frame_base={locexpr}>

Shouldn't that have static_link to refer to main as a nested function
would?  If it did, wouldn't that make the debugger handle it as it
should already handle nested functions?

These following questions are tangential, but it's the first time I've
really looked at -fopenmp DWARF output.

The name "main.omp_fn.0" is not a source identifier.  Should it really
be there in name?  IMHO it seems more proper for that to have only
artificial=true, linkage_name="main.omp_fn.0", and no name.  (Perhaps
linkage_name is not useful or appropriate since it's not DW_AT_external.)

Similarly: 

   <formal_parameter name=".omp_data_i" type="#ref1" artificial=1 location={locexpr}/>

Should that really have a name?  As it stands, the debugger is going to
present ".omp_data_i" as a resolvable identifier in this scope, as it
would for any other artifical parameter (such as C++ "this").

Next:

  <structure_type ref="ref4" name=".omp_data_s.10" byte_size=0x8>

This needs artificial=true too.  Again, it seems quite questionable
whether the non-source name should appear at all.  AIUI, this sort of
thing is exactly what DW_AT_description is for.


Thanks,
Roland

[PATCH] Don't emit certain artificial names into DW_AT_name in debug info

[Jakub Jelinek]

On Thu, Jul 22, 2010 at 12:19:35PM -0700, Roland McGrath wrote:
> Similarly: 
> 
>    <formal_parameter name=".omp_data_i" type="#ref1" artificial=1 location={locexpr}/>
> 
> Should that really have a name?  As it stands, the debugger is going to
> present ".omp_data_i" as a resolvable identifier in this scope, as it
> would for any other artifical parameter (such as C++ "this").
> 
> Next:
> 
>   <structure_type ref="ref4" name=".omp_data_s.10" byte_size=0x8>
> 
> This needs artificial=true too.  Again, it seems quite questionable
> whether the non-source name should appear at all.  AIUI, this sort of
> thing is exactly what DW_AT_description is for.

This patch attempts to deal with the artificial names of DW_AT_artificial
DIEs in debug info, as discussed yesterday on IRC.

We can't just avoid emitting DW_AT_name for DECL_ARTIFICIAL decls,
because e.g. for `this' we want to emit the name.  And, there are also types
for which we don't want to emit the made up names.
The names are useful in dumps though, so instead of clearing them this patch
steals one free bit and uses it to say that DECL_NAME resp. TYPE_NAME
shouldn't be added into debug info.
In the debug info names like array3_integer(kind=4), a.0, .omp_data_i
etc. are undesirable though, they show up when listing variables, stand in
in tab completion, etc.

Tested on a short fortran array testcase as well as OpenMP C testcase,
bootstrapped/regtested on x86_64-linux and i686-linux.  Ok for trunk?

2010-07-23  Jakub Jelinek  <jakub@redhat.com>

	* tree.h (struct tree_base): Add nameless_flag bitfield.
	(TYPE_NAMELESS, DECL_NAMELESS): Define.
	* omp-low.c (create_omp_child_function, scan_omp_parallel,
	scan_omp_task, lower_omp_taskreg): Set DECL_NAMELESS and/or
	DECL_ARTIFICIAL where needed.
	* dwarf2out.c (dwarf2_name): Return NULL if DECL_NAMELESS.
	(type_tag): Return NULL if TYPE_NAMELESS or if TYPE_DECL
	has DECL_NAMELESS set.

	* trans-types.c (gfc_get_array_descriptor_base,
	gfc_get_array_type_bounds): Set TYPE_NAMELESS.
	* trans-decl.c (gfc_build_qualified_array): Set DECL_NAMELESS
	instead of clearing DECL_NAME.
	(gfc_build_dummy_array_decl): Set DECL_NAMELESS.

--- gcc/tree.h.jj	2010-07-16 17:55:08.000000000 +0200
+++ gcc/tree.h	2010-07-23 10:44:34.000000000 +0200
@@ -387,8 +387,9 @@ struct GTY(()) tree_base {
   unsigned visited : 1;
   unsigned packed_flag : 1;
   unsigned user_align : 1;
+  unsigned nameless_flag : 1;
 
-  unsigned spare : 13;
+  unsigned spare : 12;
 
   /* This field is only used with type nodes; the only reason it is present
      in tree_base instead of tree_type is to save space.  The size of the
@@ -2180,6 +2181,9 @@ extern enum machine_mode vector_type_mod
    the term.  */
 #define TYPE_RESTRICT(NODE) (TYPE_CHECK (NODE)->type.restrict_flag)
 
+/* If nonzero, type's name shouldn't be emitted into debug info.  */
+#define TYPE_NAMELESS(NODE) (TYPE_CHECK (NODE)->base.nameless_flag)
+
 /* The address space the type is in.  */
 #define TYPE_ADDR_SPACE(NODE) (TYPE_CHECK (NODE)->base.address_space)
 
@@ -2529,6 +2533,10 @@ struct function;
 #define DECL_CONTEXT(NODE) (DECL_MINIMAL_CHECK (NODE)->decl_minimal.context)
 #define DECL_FIELD_CONTEXT(NODE) \
   (FIELD_DECL_CHECK (NODE)->decl_minimal.context)
+
+/* If nonzero, decl's name shouldn't be emitted into debug info.  */
+#define DECL_NAMELESS(NODE) (DECL_MINIMAL_CHECK (NODE)->base.nameless_flag)
+
 struct GTY(()) tree_decl_minimal {
   struct tree_common common;
   location_t locus;
--- gcc/omp-low.c.jj	2010-07-23 09:07:09.000000000 +0200
+++ gcc/omp-low.c	2010-07-23 11:16:03.000000000 +0200
@@ -1563,6 +1563,7 @@ create_omp_child_function (omp_context *
   TREE_STATIC (decl) = 1;
   TREE_USED (decl) = 1;
   DECL_ARTIFICIAL (decl) = 1;
+  DECL_NAMELESS (decl) = 1;
   DECL_IGNORED_P (decl) = 0;
   TREE_PUBLIC (decl) = 0;
   DECL_UNINLINABLE (decl) = 1;
@@ -1580,6 +1581,7 @@ create_omp_child_function (omp_context *
   t = build_decl (DECL_SOURCE_LOCATION (decl),
 		  PARM_DECL, get_identifier (".omp_data_i"), ptr_type_node);
   DECL_ARTIFICIAL (t) = 1;
+  DECL_NAMELESS (t) = 1;
   DECL_ARG_TYPE (t) = ptr_type_node;
   DECL_CONTEXT (t) = current_function_decl;
   TREE_USED (t) = 1;
@@ -1592,6 +1594,7 @@ create_omp_child_function (omp_context *
 		      PARM_DECL, get_identifier (".omp_data_o"),
 		      ptr_type_node);
       DECL_ARTIFICIAL (t) = 1;
+      DECL_NAMELESS (t) = 1;
       DECL_ARG_TYPE (t) = ptr_type_node;
       DECL_CONTEXT (t) = current_function_decl;
       TREE_USED (t) = 1;
@@ -1638,6 +1641,8 @@ scan_omp_parallel (gimple_stmt_iterator 
   name = create_tmp_var_name (".omp_data_s");
   name = build_decl (gimple_location (stmt),
 		     TYPE_DECL, name, ctx->record_type);
+  DECL_ARTIFICIAL (name) = 1;
+  DECL_NAMELESS (name) = 1;
   TYPE_NAME (ctx->record_type) = name;
   create_omp_child_function (ctx, false);
   gimple_omp_parallel_set_child_fn (stmt, ctx->cb.dst_fn);
@@ -1681,6 +1686,8 @@ scan_omp_task (gimple_stmt_iterator *gsi
   name = create_tmp_var_name (".omp_data_s");
   name = build_decl (gimple_location (stmt),
 		     TYPE_DECL, name, ctx->record_type);
+  DECL_ARTIFICIAL (name) = 1;
+  DECL_NAMELESS (name) = 1;
   TYPE_NAME (ctx->record_type) = name;
   create_omp_child_function (ctx, false);
   gimple_omp_task_set_child_fn (stmt, ctx->cb.dst_fn);
@@ -1692,6 +1699,8 @@ scan_omp_task (gimple_stmt_iterator *gsi
       name = create_tmp_var_name (".omp_data_a");
       name = build_decl (gimple_location (stmt),
 			 TYPE_DECL, name, ctx->srecord_type);
+      DECL_ARTIFICIAL (name) = 1;
+      DECL_NAMELESS (name) = 1;
       TYPE_NAME (ctx->srecord_type) = name;
       create_omp_child_function (ctx, true);
     }
@@ -6487,6 +6496,7 @@ lower_omp_taskreg (gimple_stmt_iterator 
       ctx->sender_decl
 	= create_tmp_var (ctx->srecord_type ? ctx->srecord_type
 			  : ctx->record_type, ".omp_data_o");
+      DECL_NAMELESS (ctx->sender_decl) = 1;
       TREE_ADDRESSABLE (ctx->sender_decl) = 1;
       gimple_omp_taskreg_set_data_arg (stmt, ctx->sender_decl);
     }
--- gcc/dwarf2out.c.jj	2010-07-23 09:07:15.000000000 +0200
+++ gcc/dwarf2out.c	2010-07-23 11:13:59.000000000 +0200
@@ -11243,6 +11243,8 @@ output_comdat_type_unit (comdat_type_nod
 static const char *
 dwarf2_name (tree decl, int scope)
 {
+  if (DECL_NAMELESS (decl))
+    return NULL;
   return lang_hooks.dwarf_name (decl, scope ? 1 : 0);
 }
 
@@ -17711,7 +17713,8 @@ type_tag (const_tree type)
       tree t = 0;
 
       /* Find the IDENTIFIER_NODE for the type name.  */
-      if (TREE_CODE (TYPE_NAME (type)) == IDENTIFIER_NODE)
+      if (TREE_CODE (TYPE_NAME (type)) == IDENTIFIER_NODE
+	  && !TYPE_NAMELESS (type))
 	t = TYPE_NAME (type);
 
       /* The g++ front end makes the TYPE_NAME of *each* tagged type point to
@@ -17724,7 +17727,8 @@ type_tag (const_tree type)
 	     DECL_NAME isn't set.  The default hook for decl_printable_name
 	     doesn't like that, and in this context it's correct to return
 	     0, instead of "<anonymous>" or the like.  */
-	  if (DECL_NAME (TYPE_NAME (type)))
+	  if (DECL_NAME (TYPE_NAME (type))
+	      && !DECL_NAMELESS (TYPE_NAME (type)))
 	    name = lang_hooks.dwarf_name (TYPE_NAME (type), 2);
 	}
 
--- gcc/fortran/trans-types.c.jj	2010-07-16 17:55:04.000000000 +0200
+++ gcc/fortran/trans-types.c	2010-07-23 11:13:59.000000000 +0200
@@ -1546,6 +1546,7 @@ gfc_get_array_descriptor_base (int dimen
 
   sprintf (name, "array_descriptor" GFC_RANK_PRINTF_FORMAT, dimen + codimen);
   TYPE_NAME (fat_type) = get_identifier (name);
+  TYPE_NAMELESS (fat_type) = 1;
 
   /* Add the data member as the first element of the descriptor.  */
   decl = gfc_add_field_to_struct_1 (fat_type,
@@ -1616,6 +1617,7 @@ gfc_get_array_type_bounds (tree etype, i
   sprintf (name, "array" GFC_RANK_PRINTF_FORMAT "_%.*s", dimen + codimen,
 	   GFC_MAX_SYMBOL_LEN, type_name);
   TYPE_NAME (fat_type) = get_identifier (name);
+  TYPE_NAMELESS (fat_type) = 1;
 
   GFC_DESCRIPTOR_TYPE_P (fat_type) = 1;
   TYPE_LANG_SPECIFIC (fat_type)
--- gcc/fortran/trans-decl.c.jj	2010-07-22 11:35:35.000000000 +0200
+++ gcc/fortran/trans-decl.c	2010-07-23 11:27:32.000000000 +0200
@@ -759,16 +759,16 @@ gfc_build_qualified_array (tree decl, gf
 	  gtype = build_array_type (gtype, rtype);
 	  /* Ensure the bound variables aren't optimized out at -O0.
 	     For -O1 and above they often will be optimized out, but
-	     can be tracked by VTA.  Also clear the artificial
-	     lbound.N or ubound.N DECL_NAME, so that it doesn't end up
-	     in debug info.  */
+	     can be tracked by VTA.  Also set DECL_NAMELESS, so that
+	     the artificial lbound.N or ubound.N DECL_NAME doesn't
+	     end up in debug info.  */
 	  if (lbound && TREE_CODE (lbound) == VAR_DECL
 	      && DECL_ARTIFICIAL (lbound) && DECL_IGNORED_P (lbound))
 	    {
 	      if (DECL_NAME (lbound)
 		  && strstr (IDENTIFIER_POINTER (DECL_NAME (lbound)),
 			     "lbound") != 0)
-		DECL_NAME (lbound) = NULL_TREE;
+		DECL_NAMELESS (lbound) = 1;
 	      DECL_IGNORED_P (lbound) = 0;
 	    }
 	  if (ubound && TREE_CODE (ubound) == VAR_DECL
@@ -777,7 +777,7 @@ gfc_build_qualified_array (tree decl, gf
 	      if (DECL_NAME (ubound)
 		  && strstr (IDENTIFIER_POINTER (DECL_NAME (ubound)),
 			     "ubound") != 0)
-		DECL_NAME (ubound) = NULL_TREE;
+		DECL_NAMELESS (ubound) = 1;
 	      DECL_IGNORED_P (ubound) = 0;
 	    }
 	}
@@ -879,6 +879,7 @@ gfc_build_dummy_array_decl (gfc_symbol *
 		     VAR_DECL, get_identifier (name), type);
 
   DECL_ARTIFICIAL (decl) = 1;
+  DECL_NAMELESS (decl) = 1;
   TREE_PUBLIC (decl) = 0;
   TREE_STATIC (decl) = 0;
   DECL_EXTERNAL (decl) = 0;


	Jakub

Re: [PATCH] Don't emit certain artificial names into DW_AT_name in debug info

[Richard Guenther]

On Fri, 23 Jul 2010, Jakub Jelinek wrote:

> On Thu, Jul 22, 2010 at 12:19:35PM -0700, Roland McGrath wrote:
> > Similarly: 
> > 
> >    <formal_parameter name=".omp_data_i" type="#ref1" artificial=1 location={locexpr}/>
> > 
> > Should that really have a name?  As it stands, the debugger is going to
> > present ".omp_data_i" as a resolvable identifier in this scope, as it
> > would for any other artifical parameter (such as C++ "this").
> > 
> > Next:
> > 
> >   <structure_type ref="ref4" name=".omp_data_s.10" byte_size=0x8>
> > 
> > This needs artificial=true too.  Again, it seems quite questionable
> > whether the non-source name should appear at all.  AIUI, this sort of
> > thing is exactly what DW_AT_description is for.
> 
> This patch attempts to deal with the artificial names of DW_AT_artificial
> DIEs in debug info, as discussed yesterday on IRC.
> 
> We can't just avoid emitting DW_AT_name for DECL_ARTIFICIAL decls,
> because e.g. for `this' we want to emit the name.  And, there are also types
> for which we don't want to emit the made up names.
> The names are useful in dumps though, so instead of clearing them this patch
> steals one free bit and uses it to say that DECL_NAME resp. TYPE_NAME
> shouldn't be added into debug info.
> In the debug info names like array3_integer(kind=4), a.0, .omp_data_i
> etc. are undesirable though, they show up when listing variables, stand in
> in tab completion, etc.
> 
> Tested on a short fortran array testcase as well as OpenMP C testcase,
> bootstrapped/regtested on x86_64-linux and i686-linux.  Ok for trunk?

Hmm - looking again I see

/* Nonzero for a given ..._DECL node means that the name of this node 
should
   be ignored for symbolic debug purposes.  Moreover, for a FUNCTION_DECL,
   the body of the function should also be ignored.  */
#define DECL_IGNORED_P(NODE) \
  (DECL_COMMON_CHECK (NODE)->decl_common.ignored_flag)

so why not use that?

Richard.

> 2010-07-23  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tree.h (struct tree_base): Add nameless_flag bitfield.
> 	(TYPE_NAMELESS, DECL_NAMELESS): Define.
> 	* omp-low.c (create_omp_child_function, scan_omp_parallel,
> 	scan_omp_task, lower_omp_taskreg): Set DECL_NAMELESS and/or
> 	DECL_ARTIFICIAL where needed.
> 	* dwarf2out.c (dwarf2_name): Return NULL if DECL_NAMELESS.
> 	(type_tag): Return NULL if TYPE_NAMELESS or if TYPE_DECL
> 	has DECL_NAMELESS set.
> 
> 	* trans-types.c (gfc_get_array_descriptor_base,
> 	gfc_get_array_type_bounds): Set TYPE_NAMELESS.
> 	* trans-decl.c (gfc_build_qualified_array): Set DECL_NAMELESS
> 	instead of clearing DECL_NAME.
> 	(gfc_build_dummy_array_decl): Set DECL_NAMELESS.
> 
> --- gcc/tree.h.jj	2010-07-16 17:55:08.000000000 +0200
> +++ gcc/tree.h	2010-07-23 10:44:34.000000000 +0200
> @@ -387,8 +387,9 @@ struct GTY(()) tree_base {
>    unsigned visited : 1;
>    unsigned packed_flag : 1;
>    unsigned user_align : 1;
> +  unsigned nameless_flag : 1;
>  
> -  unsigned spare : 13;
> +  unsigned spare : 12;
>  
>    /* This field is only used with type nodes; the only reason it is present
>       in tree_base instead of tree_type is to save space.  The size of the
> @@ -2180,6 +2181,9 @@ extern enum machine_mode vector_type_mod
>     the term.  */
>  #define TYPE_RESTRICT(NODE) (TYPE_CHECK (NODE)->type.restrict_flag)
>  
> +/* If nonzero, type's name shouldn't be emitted into debug info.  */
> +#define TYPE_NAMELESS(NODE) (TYPE_CHECK (NODE)->base.nameless_flag)
> +
>  /* The address space the type is in.  */
>  #define TYPE_ADDR_SPACE(NODE) (TYPE_CHECK (NODE)->base.address_space)
>  
> @@ -2529,6 +2533,10 @@ struct function;
>  #define DECL_CONTEXT(NODE) (DECL_MINIMAL_CHECK (NODE)->decl_minimal.context)
>  #define DECL_FIELD_CONTEXT(NODE) \
>    (FIELD_DECL_CHECK (NODE)->decl_minimal.context)
> +
> +/* If nonzero, decl's name shouldn't be emitted into debug info.  */
> +#define DECL_NAMELESS(NODE) (DECL_MINIMAL_CHECK (NODE)->base.nameless_flag)
> +
>  struct GTY(()) tree_decl_minimal {
>    struct tree_common common;
>    location_t locus;
> --- gcc/omp-low.c.jj	2010-07-23 09:07:09.000000000 +0200
> +++ gcc/omp-low.c	2010-07-23 11:16:03.000000000 +0200
> @@ -1563,6 +1563,7 @@ create_omp_child_function (omp_context *
>    TREE_STATIC (decl) = 1;
>    TREE_USED (decl) = 1;
>    DECL_ARTIFICIAL (decl) = 1;
> +  DECL_NAMELESS (decl) = 1;
>    DECL_IGNORED_P (decl) = 0;
>    TREE_PUBLIC (decl) = 0;
>    DECL_UNINLINABLE (decl) = 1;
> @@ -1580,6 +1581,7 @@ create_omp_child_function (omp_context *
>    t = build_decl (DECL_SOURCE_LOCATION (decl),
>  		  PARM_DECL, get_identifier (".omp_data_i"), ptr_type_node);
>    DECL_ARTIFICIAL (t) = 1;
> +  DECL_NAMELESS (t) = 1;
>    DECL_ARG_TYPE (t) = ptr_type_node;
>    DECL_CONTEXT (t) = current_function_decl;
>    TREE_USED (t) = 1;
> @@ -1592,6 +1594,7 @@ create_omp_child_function (omp_context *
>  		      PARM_DECL, get_identifier (".omp_data_o"),
>  		      ptr_type_node);
>        DECL_ARTIFICIAL (t) = 1;
> +      DECL_NAMELESS (t) = 1;
>        DECL_ARG_TYPE (t) = ptr_type_node;
>        DECL_CONTEXT (t) = current_function_decl;
>        TREE_USED (t) = 1;
> @@ -1638,6 +1641,8 @@ scan_omp_parallel (gimple_stmt_iterator 
>    name = create_tmp_var_name (".omp_data_s");
>    name = build_decl (gimple_location (stmt),
>  		     TYPE_DECL, name, ctx->record_type);
> +  DECL_ARTIFICIAL (name) = 1;
> +  DECL_NAMELESS (name) = 1;
>    TYPE_NAME (ctx->record_type) = name;
>    create_omp_child_function (ctx, false);
>    gimple_omp_parallel_set_child_fn (stmt, ctx->cb.dst_fn);
> @@ -1681,6 +1686,8 @@ scan_omp_task (gimple_stmt_iterator *gsi
>    name = create_tmp_var_name (".omp_data_s");
>    name = build_decl (gimple_location (stmt),
>  		     TYPE_DECL, name, ctx->record_type);
> +  DECL_ARTIFICIAL (name) = 1;
> +  DECL_NAMELESS (name) = 1;
>    TYPE_NAME (ctx->record_type) = name;
>    create_omp_child_function (ctx, false);
>    gimple_omp_task_set_child_fn (stmt, ctx->cb.dst_fn);
> @@ -1692,6 +1699,8 @@ scan_omp_task (gimple_stmt_iterator *gsi
>        name = create_tmp_var_name (".omp_data_a");
>        name = build_decl (gimple_location (stmt),
>  			 TYPE_DECL, name, ctx->srecord_type);
> +      DECL_ARTIFICIAL (name) = 1;
> +      DECL_NAMELESS (name) = 1;
>        TYPE_NAME (ctx->srecord_type) = name;
>        create_omp_child_function (ctx, true);
>      }
> @@ -6487,6 +6496,7 @@ lower_omp_taskreg (gimple_stmt_iterator 
>        ctx->sender_decl
>  	= create_tmp_var (ctx->srecord_type ? ctx->srecord_type
>  			  : ctx->record_type, ".omp_data_o");
> +      DECL_NAMELESS (ctx->sender_decl) = 1;
>        TREE_ADDRESSABLE (ctx->sender_decl) = 1;
>        gimple_omp_taskreg_set_data_arg (stmt, ctx->sender_decl);
>      }
> --- gcc/dwarf2out.c.jj	2010-07-23 09:07:15.000000000 +0200
> +++ gcc/dwarf2out.c	2010-07-23 11:13:59.000000000 +0200
> @@ -11243,6 +11243,8 @@ output_comdat_type_unit (comdat_type_nod
>  static const char *
>  dwarf2_name (tree decl, int scope)
>  {
> +  if (DECL_NAMELESS (decl))
> +    return NULL;
>    return lang_hooks.dwarf_name (decl, scope ? 1 : 0);
>  }
>  
> @@ -17711,7 +17713,8 @@ type_tag (const_tree type)
>        tree t = 0;
>  
>        /* Find the IDENTIFIER_NODE for the type name.  */
> -      if (TREE_CODE (TYPE_NAME (type)) == IDENTIFIER_NODE)
> +      if (TREE_CODE (TYPE_NAME (type)) == IDENTIFIER_NODE
> +	  && !TYPE_NAMELESS (type))
>  	t = TYPE_NAME (type);
>  
>        /* The g++ front end makes the TYPE_NAME of *each* tagged type point to
> @@ -17724,7 +17727,8 @@ type_tag (const_tree type)
>  	     DECL_NAME isn't set.  The default hook for decl_printable_name
>  	     doesn't like that, and in this context it's correct to return
>  	     0, instead of "<anonymous>" or the like.  */
> -	  if (DECL_NAME (TYPE_NAME (type)))
> +	  if (DECL_NAME (TYPE_NAME (type))
> +	      && !DECL_NAMELESS (TYPE_NAME (type)))
>  	    name = lang_hooks.dwarf_name (TYPE_NAME (type), 2);
>  	}
>  
> --- gcc/fortran/trans-types.c.jj	2010-07-16 17:55:04.000000000 +0200
> +++ gcc/fortran/trans-types.c	2010-07-23 11:13:59.000000000 +0200
> @@ -1546,6 +1546,7 @@ gfc_get_array_descriptor_base (int dimen
>  
>    sprintf (name, "array_descriptor" GFC_RANK_PRINTF_FORMAT, dimen + codimen);
>    TYPE_NAME (fat_type) = get_identifier (name);
> +  TYPE_NAMELESS (fat_type) = 1;
>  
>    /* Add the data member as the first element of the descriptor.  */
>    decl = gfc_add_field_to_struct_1 (fat_type,
> @@ -1616,6 +1617,7 @@ gfc_get_array_type_bounds (tree etype, i
>    sprintf (name, "array" GFC_RANK_PRINTF_FORMAT "_%.*s", dimen + codimen,
>  	   GFC_MAX_SYMBOL_LEN, type_name);
>    TYPE_NAME (fat_type) = get_identifier (name);
> +  TYPE_NAMELESS (fat_type) = 1;
>  
>    GFC_DESCRIPTOR_TYPE_P (fat_type) = 1;
>    TYPE_LANG_SPECIFIC (fat_type)
> --- gcc/fortran/trans-decl.c.jj	2010-07-22 11:35:35.000000000 +0200
> +++ gcc/fortran/trans-decl.c	2010-07-23 11:27:32.000000000 +0200
> @@ -759,16 +759,16 @@ gfc_build_qualified_array (tree decl, gf
>  	  gtype = build_array_type (gtype, rtype);
>  	  /* Ensure the bound variables aren't optimized out at -O0.
>  	     For -O1 and above they often will be optimized out, but
> -	     can be tracked by VTA.  Also clear the artificial
> -	     lbound.N or ubound.N DECL_NAME, so that it doesn't end up
> -	     in debug info.  */
> +	     can be tracked by VTA.  Also set DECL_NAMELESS, so that
> +	     the artificial lbound.N or ubound.N DECL_NAME doesn't
> +	     end up in debug info.  */
>  	  if (lbound && TREE_CODE (lbound) == VAR_DECL
>  	      && DECL_ARTIFICIAL (lbound) && DECL_IGNORED_P (lbound))
>  	    {
>  	      if (DECL_NAME (lbound)
>  		  && strstr (IDENTIFIER_POINTER (DECL_NAME (lbound)),
>  			     "lbound") != 0)
> -		DECL_NAME (lbound) = NULL_TREE;
> +		DECL_NAMELESS (lbound) = 1;
>  	      DECL_IGNORED_P (lbound) = 0;
>  	    }
>  	  if (ubound && TREE_CODE (ubound) == VAR_DECL
> @@ -777,7 +777,7 @@ gfc_build_qualified_array (tree decl, gf
>  	      if (DECL_NAME (ubound)
>  		  && strstr (IDENTIFIER_POINTER (DECL_NAME (ubound)),
>  			     "ubound") != 0)
> -		DECL_NAME (ubound) = NULL_TREE;
> +		DECL_NAMELESS (ubound) = 1;
>  	      DECL_IGNORED_P (ubound) = 0;
>  	    }
>  	}
> @@ -879,6 +879,7 @@ gfc_build_dummy_array_decl (gfc_symbol *
>  		     VAR_DECL, get_identifier (name), type);
>  
>    DECL_ARTIFICIAL (decl) = 1;
> +  DECL_NAMELESS (decl) = 1;
>    TREE_PUBLIC (decl) = 0;
>    TREE_STATIC (decl) = 0;
>    DECL_EXTERNAL (decl) = 0;
> 
> 
> 	Jakub
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex

Re: [PATCH] Don't emit certain artificial names into DW_AT_name in debug info

[Jakub Jelinek]

On Fri, Jul 23, 2010 at 02:08:58PM +0200, Richard Guenther wrote:
> On Fri, 23 Jul 2010, Jakub Jelinek wrote:
> Hmm - looking again I see
> 
> /* Nonzero for a given ..._DECL node means that the name of this node 
> should
>    be ignored for symbolic debug purposes.  Moreover, for a FUNCTION_DECL,
>    the body of the function should also be ignored.  */
> #define DECL_IGNORED_P(NODE) \
>   (DECL_COMMON_CHECK (NODE)->decl_common.ignored_flag)

DECL_IGNORED_P means it is ignored completely for debug info purposes.
For the ones touched by this patch, we must not do this, we really need to
emit the DIEs as they are referenced from other places, we need to track
their location (DECL_IGNORED_P decls aren't tracked at all e.g. by
var-tracking), etc.
We want to emit them as DW_AT_artificial without DW_AT_name, as the name
is made up.

	Jakub

Re: [PATCH] Don't emit certain artificial names into DW_AT_name in debug info

[Roland McGrath]

I think it may well be worthwhile to emit some DW_AT_description for these.
Perhaps just the made-up name is not that useful in DW_AT_description, but
then perhaps it is.  They could also be slightly prettified things a human
might grok, like "omp worker for foo.c:123", "omp data bundle for foo.c:123",
"array descriptor for [17,23]", etc.

But certainly all that is optional, just eye candy deep internal data
displays in a debugger.  Omitting DW_AT_name is what's really necessary for
correctness.


Thanks,
Roland

Re: [PATCH] Don't emit certain artificial names into DW_AT_name in debug info

[Richard Henderson]

On 07/23/2010 05:00 AM, Jakub Jelinek wrote:
> 	* tree.h (struct tree_base): Add nameless_flag bitfield.
> 	(TYPE_NAMELESS, DECL_NAMELESS): Define.
> 	* omp-low.c (create_omp_child_function, scan_omp_parallel,
> 	scan_omp_task, lower_omp_taskreg): Set DECL_NAMELESS and/or
> 	DECL_ARTIFICIAL where needed.
> 	* dwarf2out.c (dwarf2_name): Return NULL if DECL_NAMELESS.
> 	(type_tag): Return NULL if TYPE_NAMELESS or if TYPE_DECL
> 	has DECL_NAMELESS set.
> 
> 	* trans-types.c (gfc_get_array_descriptor_base,
> 	gfc_get_array_type_bounds): Set TYPE_NAMELESS.
> 	* trans-decl.c (gfc_build_qualified_array): Set DECL_NAMELESS
> 	instead of clearing DECL_NAME.
> 	(gfc_build_dummy_array_decl): Set DECL_NAMELESS.

Ok.

You might also tidy the documentation for DECL_IGNORED_P so
that it isn't confusing wrt nameless here.


r~

Re: [PATCH] Improve OpenMP debug info for threadprivate vars and shared local statics

[Tom Tromey]

>>>>> "Roland" == Roland McGrath <roland@redhat.com> writes:

Roland> IMHO it would be better to do it without such duplication.  I
Roland> think we can beat the support out of the gdb people well enough,
Roland> if the "proper" indirect representations of all the info are
Roland> there.

Please file a bug in gdb bugzilla with all the details.

Tom

[PATCH] Do emit certain artificial names into DW_AT_name in debug info

[Jakub Jelinek]

On Fri, Jul 23, 2010 at 02:00:11PM +0200, Jakub Jelinek wrote:
> On Thu, Jul 22, 2010 at 12:19:35PM -0700, Roland McGrath wrote:
> > Similarly: 
> > 
> >    <formal_parameter name=".omp_data_i" type="#ref1" artificial=1 location={locexpr}/>
> > 
> > Should that really have a name?  As it stands, the debugger is going to
> > present ".omp_data_i" as a resolvable identifier in this scope, as it
> > would for any other artifical parameter (such as C++ "this").
> > 
> > Next:
> > 
> >   <structure_type ref="ref4" name=".omp_data_s.10" byte_size=0x8>
> > 
> > This needs artificial=true too.  Again, it seems quite questionable
> > whether the non-source name should appear at all.  AIUI, this sort of
> > thing is exactly what DW_AT_description is for.
> 
> This patch attempts to deal with the artificial names of DW_AT_artificial
> DIEs in debug info, as discussed yesterday on IRC.
> 
> We can't just avoid emitting DW_AT_name for DECL_ARTIFICIAL decls,
> because e.g. for `this' we want to emit the name.  And, there are also types
> for which we don't want to emit the made up names.
> The names are useful in dumps though, so instead of clearing them this patch
> steals one free bit and uses it to say that DECL_NAME resp. TYPE_NAME
> shouldn't be added into debug info.
> In the debug info names like array3_integer(kind=4), a.0, .omp_data_i
> etc. are undesirable though, they show up when listing variables, stand in
> in tab completion, etc.
> 
> Tested on a short fortran array testcase as well as OpenMP C testcase,
> bootstrapped/regtested on x86_64-linux and i686-linux.  Ok for trunk?
>
> 2010-07-23  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tree.h (struct tree_base): Add nameless_flag bitfield.
> 	(TYPE_NAMELESS, DECL_NAMELESS): Define.
> 	* omp-low.c (create_omp_child_function, scan_omp_parallel,
> 	scan_omp_task, lower_omp_taskreg): Set DECL_NAMELESS and/or
> 	DECL_ARTIFICIAL where needed.
...

Unfortunately, as mentioned in
https://bugzilla.redhat.com/show_bug.cgi?id=844959
it seems that GDB now (not sure since when) relies on DW_AT_name being
present on DW_TAG_subprogram (indeed in the DWARF standard there is "has
DW_AT_name", not "may have" wording) and that seems to be the reason why
almost nothing is debuggable in the outlined parallel or task regions.

Until we have some better attribute for this, I'd like to partially revert
this (i.e. restore the artificial name on DW_TAG_subprogram, keep
DECL_NAMELESS on all the other entities).

Bootstrapped/regtested on x86_64-linux and i686-linux, any objections
from checking this into trunk/4.8 (and 4.9.2 once 4.9.1 is out)?

2014-07-15  Jakub Jelinek  <jakub@redhat.com>

	* omp-low.c (create_omp_child_function): Don't set DECL_NAMELESS
	on the FUNCTION_DECL.

--- gcc/omp-low.c.jj	2014-07-09 10:41:12.000000000 +0200
+++ gcc/omp-low.c	2014-07-14 16:49:41.950235683 +0200
@@ -1872,7 +1872,6 @@ create_omp_child_function (omp_context *
   TREE_STATIC (decl) = 1;
   TREE_USED (decl) = 1;
   DECL_ARTIFICIAL (decl) = 1;
-  DECL_NAMELESS (decl) = 1;
   DECL_IGNORED_P (decl) = 0;
   TREE_PUBLIC (decl) = 0;
   DECL_UNINLINABLE (decl) = 1;


	Jakub