Improve OpenMP debug info for threadprivate vars and shared local statics

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.


	Jakub

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~

> 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

>>>>> "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