[Fwd: Re: [patch] Re: Accessing tls variables across files causes a bug]

[Fwd: Re: [patch] Re: Accessing tls variables across files causes a bug]

[Vinay Sridhar]

[-- Attachment #1: Type: text/plain, Size: 76 bytes --]

Hello,

Any idea when the below changes are likely to go in?

Thanks,
Vinay

[-- Attachment #2: Forwarded message - Re: [patch] Re: Accessing tls variables across files causes a bug --]
[-- Type: message/rfc822, Size: 21475 bytes --]

From: Vinay Sridhar <vinay@linux.vnet.ibm.com>
To: Jan Kratochvil <jan.kratochvil@redhat.com>
Cc: gdb-patches@sources.redhat.com, Daniel Jacobowitz <drow@false.org>,  luisgpm@linux.vnet.ibm.com, uweigand@de.ibm.com
Subject: Re: [patch] Re: Accessing tls variables across files causes a bug
Date: Wed, 27 Aug 2008 10:07:38 +0530
Message-ID: <1219811851.4179.1.camel@localhost.localdomain>

When can we expect these patches to go in?

Thanks,
Vinay


On Wed, 2008-08-06 at 17:19 +0200, Jan Kratochvil wrote:
> On Wed, 06 Aug 2008 13:42:41 +0200, Jan Kratochvil wrote:
> > Another possibility is that LOC_UNRESOLVED may no longer be needed for recent
> > gcc debuginfos always(?) containting `DW_AT_location's, therefore we would not
> > have to deal with `minimal_symbol's in this case at all.
> 
> Attached.
> 
> It has no regressions on x86_64, Fedora gcc-4.3.1-6.x86_64 (but gcc-4.3 has
> GDB regressions against gcc-4.1).  Also tried there are no regressions by
> check//unix/-gstabs+ (although the three new TLS testcases FAIL there).
> 
> I do not fully grok why psymtabs were created for DW_AT_type DIEs with no
> DW_AT_location.  IMO (DW_AT_location || DW_AT_const_value) is the right
> condition (DW_AT_const_value requirement was found by a testsuite run).
> 
> 
> Regards,
> Jan
> plain text document attachment (gdb-remove-LOC_UNRESOLVED.patch)
> 2008-08-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix resolving external references to TLS variables.
> 	* ada-lang.c (ada_add_block_symbols): Remove LOC_UNRESOLVED.
> 	* ax-gdb.c (gen_var_ref): Likewise.
> 	* gen_var_ref (symbol_read_needs_frame, read_var_value): Likewise.
> 	* m2-exp.y (yylex): Likewise.
> 	* printcmd.c (address_info): Likewise.
> 	* symmisc.c (print_symbol, print_partial_symbols): Likewise.
> 	* symtab.h (enum address_class): Likewise.
> 	* tracepoint.c (collect_symbol, scope_info): Likewise.
> 	* mi/mi-cmd-stack.c (list_args_or_locals): Likewise.
> 	* stabsread.c (scan_file_globals): Complain even on LOC_STATIC symbols.
> 	Set such symbols to LOC_UNDEF instead of LOC_UNRESOLVED.
> 	* dwarf2read.c (struct partial_die_info): Remove the field HAS_TYPE.
> 	New field HAS_CONST_VALUE.
> 	(add_partial_symbol) <DW_TAG_variable>: HAS_TYPE condition removed.
> 	HAS_CONST_VALUE condition added.  Comment updated.
> 	(read_partial_die) <DW_AT_type>: Case removed.
> 	(read_partial_die) <DW_AT_const_value>: New case.
> 	(new_symbol): Remove setting LOC_UNRESOLVED.  Comment updated.
> 
> 2008-08-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Update for removed LOC_UNRESOLVED, test resolving external references
> 	to TLS variables.
> 	* gdb.dwarf2/dw2-noloc.S: New variable "optloc".
> 	* gdb.dwarf2/dw2-noloc.exp: Test the new variable "optloc".  Update the
> 	current message for the variable "noloc".
> 	* gdb.threads/tls.exp: New tests to print A_THREAD_LOCAL and
> 	FILE2_THREAD_LOCAL.
> 	(testfile2, srcfile2): New variables.
> 	* gdb.threads/tls.c (file2_thread_local)
> 	(function_referencing_file2_thread_local): New.
> 	* gdb.threads/tls2.c: New file.
> 
> --- gdb/ada-lang.c	21 Jul 2008 16:47:10 -0000	1.151
> +++ gdb/ada-lang.c	6 Aug 2008 15:13:51 -0000
> @@ -5211,9 +5211,7 @@ ada_add_block_symbols (struct obstack *o
>                                     SYMBOL_DOMAIN (sym), domain)
>              && wild_match (name, name_len, SYMBOL_LINKAGE_NAME (sym)))
>            {
> -	    if (SYMBOL_CLASS (sym) == LOC_UNRESOLVED)
> -	      continue;
> -	    else if (SYMBOL_IS_ARGUMENT (sym))
> +	    if (SYMBOL_IS_ARGUMENT (sym))
>  	      arg_sym = sym;
>  	    else
>  	      {
> @@ -5236,17 +5234,14 @@ ada_add_block_symbols (struct obstack *o
>              if (cmp == 0
>                  && is_name_suffix (SYMBOL_LINKAGE_NAME (sym) + name_len))
>                {
> -		if (SYMBOL_CLASS (sym) != LOC_UNRESOLVED)
> +		if (SYMBOL_IS_ARGUMENT (sym))
> +		  arg_sym = sym;
> +		else
>  		  {
> -		    if (SYMBOL_IS_ARGUMENT (sym))
> -		      arg_sym = sym;
> -		    else
> -		      {
> -			found_sym = 1;
> -			add_defn_to_vec (obstackp,
> -					 fixup_symbol_section (sym, objfile),
> -					 block);
> -		      }
> +		    found_sym = 1;
> +		    add_defn_to_vec (obstackp,
> +				     fixup_symbol_section (sym, objfile),
> +				     block);
>  		  }
>                }
>            }
> @@ -5284,17 +5279,14 @@ ada_add_block_symbols (struct obstack *o
>              if (cmp == 0
>                  && is_name_suffix (SYMBOL_LINKAGE_NAME (sym) + name_len + 5))
>                {
> -		if (SYMBOL_CLASS (sym) != LOC_UNRESOLVED)
> +		if (SYMBOL_IS_ARGUMENT (sym))
> +		  arg_sym = sym;
> +		else
>  		  {
> -		    if (SYMBOL_IS_ARGUMENT (sym))
> -		      arg_sym = sym;
> -		    else
> -		      {
> -			found_sym = 1;
> -			add_defn_to_vec (obstackp,
> -					 fixup_symbol_section (sym, objfile),
> -					 block);
> -		      }
> +		    found_sym = 1;
> +		    add_defn_to_vec (obstackp,
> +				     fixup_symbol_section (sym, objfile),
> +				     block);
>  		  }
>                }
>            }
> --- gdb/ax-gdb.c	27 May 2008 19:29:51 -0000	1.44
> +++ gdb/ax-gdb.c	6 Aug 2008 15:13:53 -0000
> @@ -596,19 +596,6 @@ gen_var_ref (struct agent_expr *ax, stru
>        value->kind = axs_lvalue_memory;
>        break;
> 
> -    case LOC_UNRESOLVED:
> -      {
> -	struct minimal_symbol *msym
> -	= lookup_minimal_symbol (DEPRECATED_SYMBOL_NAME (var), NULL, NULL);
> -	if (!msym)
> -	  error (_("Couldn't resolve symbol `%s'."), SYMBOL_PRINT_NAME (var));
> -
> -	/* Push the address of the variable.  */
> -	ax_const_l (ax, SYMBOL_VALUE_ADDRESS (msym));
> -	value->kind = axs_lvalue_memory;
> -      }
> -      break;
> -
>      case LOC_COMPUTED:
>        /* FIXME: cagney/2004-01-26: It should be possible to
>  	 unconditionally call the SYMBOL_OPS method when available.
> --- gdb/dwarf2read.c	27 Jun 2008 17:56:47 -0000	1.267
> +++ gdb/dwarf2read.c	6 Aug 2008 15:14:01 -0000
> @@ -462,7 +462,7 @@ struct partial_die_info
>      unsigned int has_children : 1;
>      unsigned int is_external : 1;
>      unsigned int is_declaration : 1;
> -    unsigned int has_type : 1;
> +    unsigned int has_const_value : 1;
>      unsigned int has_specification : 1;
>      unsigned int has_stmt_list : 1;
>      unsigned int has_pc_info : 1;
> @@ -1988,18 +1988,17 @@ add_partial_symbol (struct partial_die_i
>  	     Don't enter into the minimal symbol tables as there is
>  	     a minimal symbol table entry from the ELF symbols already.
>  	     Enter into partial symbol table if it has a location
> -	     descriptor or a type.
> -	     If the location descriptor is missing, new_symbol will create
> -	     a LOC_UNRESOLVED symbol, the address of the variable will then
> -	     be determined from the minimal symbol table whenever the variable
> -	     is referenced.
> +	     descriptor.  It may have a type but still if it has neither
> +	     location descriptor nor it is a constant value it must be just an
> +	     `extern' declaration we are not interested in.  Optimized out
> +	     variables will have a 0-length location descriptor.
>  	     The address for the partial symbol table entry is not
>  	     used by GDB, but it comes in handy for debugging partial symbol
>  	     table building.  */
> 
>  	  if (pdi->locdesc)
>  	    addr = decode_locdesc (pdi->locdesc, cu);
> -	  if (pdi->locdesc || pdi->has_type)
> +	  if (pdi->locdesc || pdi->has_const_value)
>  	    psym = add_psymbol_to_list (actual_name, strlen (actual_name),
>  					VAR_DOMAIN, LOC_STATIC,
>  					&objfile->global_psymbols,
> @@ -5837,8 +5836,8 @@ read_partial_die (struct partial_die_inf
>  	case DW_AT_declaration:
>  	  part_die->is_declaration = DW_UNSND (&attr);
>  	  break;
> -	case DW_AT_type:
> -	  part_die->has_type = 1;
> +	case DW_AT_const_value:
> +	  part_die->has_const_value = 1;
>  	  break;
>  	case DW_AT_abstract_origin:
>  	case DW_AT_specification:
> @@ -7486,19 +7485,15 @@ new_symbol (struct die_info *die, struct
>  	    }
>  	  else
>  	    {
> -	      /* We do not know the address of this symbol.
> -	         If it is an external symbol and we have type information
> -	         for it, enter the symbol as a LOC_UNRESOLVED symbol.
> -	         The address of the variable will then be determined from
> -	         the minimal symbol table whenever the variable is
> -	         referenced.  */
> -	      attr2 = dwarf2_attr (die, DW_AT_external, cu);
> -	      if (attr2 && (DW_UNSND (attr2) != 0)
> -		  && dwarf2_attr (die, DW_AT_type, cu) != NULL)
> -		{
> -		  SYMBOL_CLASS (sym) = LOC_UNRESOLVED;
> -		  add_symbol_to_list (sym, &global_symbols);
> -		}
> +	      /* We do not know the address of this symbol.  If it is an
> +		 external symbol and we have type information
> +		 for it, we could enter the symbol as a LOC_UNRESOLVED symbol.
> +		 The address of the variable could then be determined from the
> +		 minimal symbol table whenever the variable is referenced.
> +		 Just it needs an exception in the code whenever we get
> +		 LOC_UNRESOLVED and some places miss it, fortunately recent GCC
> +		 puts DW_AT_location everywhere so this workaround is no longer
> +		 needed.  */
>  	    }
>  	  break;
>  	case DW_TAG_formal_parameter:
> --- gdb/findvar.c	15 Jul 2008 17:53:11 -0000	1.115
> +++ gdb/findvar.c	6 Aug 2008 15:14:02 -0000
> @@ -371,7 +371,6 @@ symbol_read_needs_frame (struct symbol *
> 
>      case LOC_BLOCK:
>      case LOC_CONST_BYTES:
> -    case LOC_UNRESOLVED:
>      case LOC_OPTIMIZED_OUT:
>        return 0;
>      }
> @@ -533,21 +532,6 @@ read_var_value (struct symbol *var, stru
>  	return 0;
>        return SYMBOL_OPS (var)->read_variable (var, frame);
> 
> -    case LOC_UNRESOLVED:
> -      {
> -	struct minimal_symbol *msym;
> -
> -	msym = lookup_minimal_symbol (DEPRECATED_SYMBOL_NAME (var), NULL, NULL);
> -	if (msym == NULL)
> -	  return 0;
> -	if (overlay_debugging)
> -	  addr = symbol_overlayed_address (SYMBOL_VALUE_ADDRESS (msym),
> -					   SYMBOL_BFD_SECTION (msym));
> -	else
> -	  addr = SYMBOL_VALUE_ADDRESS (msym);
> -      }
> -      break;
> -
>      case LOC_OPTIMIZED_OUT:
>        VALUE_LVAL (v) = not_lval;
>        set_value_optimized_out (v, 1);
> --- gdb/m2-exp.y	27 May 2008 19:29:51 -0000	1.21
> +++ gdb/m2-exp.y	6 Aug 2008 15:14:02 -0000
> @@ -1059,7 +1059,6 @@ yylex ()
>  	  error("internal:  Undefined class in m2lex()");
> 
>         case LOC_LABEL:
> -       case LOC_UNRESOLVED:
>  	  error("internal:  Unforseen case in m2lex()");
> 
>         default:
> --- gdb/printcmd.c	6 Jun 2008 20:58:08 -0000	1.127
> +++ gdb/printcmd.c	6 Aug 2008 15:14:05 -0000
> @@ -1169,30 +1169,6 @@ address_info (char *exp, int from_tty)
>  	}
>        break;
> 
> -    case LOC_UNRESOLVED:
> -      {
> -	struct minimal_symbol *msym;
> -
> -	msym = lookup_minimal_symbol (DEPRECATED_SYMBOL_NAME (sym), NULL, NULL);
> -	if (msym == NULL)
> -	  printf_filtered ("unresolved");
> -	else
> -	  {
> -	    section = SYMBOL_BFD_SECTION (msym);
> -	    printf_filtered (_("static storage at address "));
> -	    load_addr = SYMBOL_VALUE_ADDRESS (msym);
> -	    fputs_filtered (paddress (load_addr), gdb_stdout);
> -	    if (section_is_overlay (section))
> -	      {
> -		load_addr = overlay_unmapped_address (load_addr, section);
> -		printf_filtered (_(",\n -- loaded at "));
> -		fputs_filtered (paddress (load_addr), gdb_stdout);
> -		printf_filtered (_(" in overlay section %s"), section->name);
> -	      }
> -	  }
> -      }
> -      break;
> -
>      case LOC_OPTIMIZED_OUT:
>        printf_filtered (_("optimized out"));
>        break;
> --- gdb/stabsread.c	27 May 2008 19:29:51 -0000	1.108
> +++ gdb/stabsread.c	6 Aug 2008 15:14:08 -0000
> @@ -4501,7 +4501,7 @@ scan_file_globals (struct objfile *objfi
>      }
> 
>    /* Change the storage class of any remaining unresolved globals to
> -     LOC_UNRESOLVED and remove them from the chain.  */
> +     LOC_UNDEF and remove them from the chain.  */
>    for (hash = 0; hash < HASHSIZE; hash++)
>      {
>        sym = global_sym_chain[hash];
> @@ -4514,13 +4514,11 @@ scan_file_globals (struct objfile *objfi
>  	     to address zero.  */
>  	  SYMBOL_VALUE_ADDRESS (prev) = 0;
> 
> +	  SYMBOL_CLASS (prev) = LOC_UNDEF;
>  	  /* Complain about unresolved common block symbols.  */
> -	  if (SYMBOL_CLASS (prev) == LOC_STATIC)
> -	    SYMBOL_CLASS (prev) = LOC_UNRESOLVED;
> -	  else
> -	    complaint (&symfile_complaints,
> -		       _("%s: common block `%s' from global_sym_chain unresolved"),
> -		       objfile->name, DEPRECATED_SYMBOL_NAME (prev));
> +	  complaint (&symfile_complaints,
> +		     _("%s: common block `%s' from global_sym_chain unresolved"),
> +		     objfile->name, DEPRECATED_SYMBOL_NAME (prev));
>  	}
>      }
>    memset (global_sym_chain, 0, sizeof (global_sym_chain));
> --- gdb/symmisc.c	27 May 2008 19:29:51 -0000	1.53
> +++ gdb/symmisc.c	6 Aug 2008 15:14:09 -0000
> @@ -701,10 +701,6 @@ print_symbol (void *args)
>  	  fprintf_filtered (outfile, "computed at runtime");
>  	  break;
> 
> -	case LOC_UNRESOLVED:
> -	  fprintf_filtered (outfile, "unresolved");
> -	  break;
> -
>  	case LOC_OPTIMIZED_OUT:
>  	  fprintf_filtered (outfile, "optimized out");
>  	  break;
> @@ -837,9 +833,6 @@ print_partial_symbols (struct partial_sy
>  	case LOC_CONST_BYTES:
>  	  fputs_filtered ("constant bytes", outfile);
>  	  break;
> -	case LOC_UNRESOLVED:
> -	  fputs_filtered ("unresolved", outfile);
> -	  break;
>  	case LOC_OPTIMIZED_OUT:
>  	  fputs_filtered ("optimized out", outfile);
>  	  break;
> --- gdb/symtab.h	27 May 2008 19:29:51 -0000	1.128
> +++ gdb/symtab.h	6 Aug 2008 15:14:12 -0000
> @@ -476,18 +476,6 @@ enum address_class
> 
>    LOC_CONST_BYTES,
> 
> -  /* Value is at fixed address, but the address of the variable has
> -     to be determined from the minimal symbol table whenever the
> -     variable is referenced.
> -     This happens if debugging information for a global symbol is
> -     emitted and the corresponding minimal symbol is defined
> -     in another object file or runtime common storage.
> -     The linker might even remove the minimal symbol if the global
> -     symbol is never referenced, in which case the symbol remains
> -     unresolved.  */
> -
> -  LOC_UNRESOLVED,
> -
>    /* The variable does not actually exist in the program.
>       The value is ignored.  */
> 
> --- gdb/tracepoint.c	25 Jul 2008 16:12:03 -0000	1.107
> +++ gdb/tracepoint.c	6 Aug 2008 15:14:14 -0000
> @@ -1289,10 +1289,6 @@ collect_symbol (struct collection_list *
>  	}
>        add_memrange (collect, reg, offset, len);
>        break;
> -    case LOC_UNRESOLVED:
> -      printf_filtered ("Don't know LOC_UNRESOLVED %s\n", 
> -		       DEPRECATED_SYMBOL_NAME (sym));
> -      break;
>      case LOC_OPTIMIZED_OUT:
>        printf_filtered ("%s has been optimized out of existence.\n",
>  		       DEPRECATED_SYMBOL_NAME (sym));
> @@ -2457,17 +2453,6 @@ scope_info (char *args, int from_tty)
>  	      printf_filtered ("a function at address ");
>  	      printf_filtered ("%s", paddress (BLOCK_START (SYMBOL_BLOCK_VALUE (sym))));
>  	      break;
> -	    case LOC_UNRESOLVED:
> -	      msym = lookup_minimal_symbol (DEPRECATED_SYMBOL_NAME (sym), 
> -					    NULL, NULL);
> -	      if (msym == NULL)
> -		printf_filtered ("Unresolved Static");
> -	      else
> -		{
> -		  printf_filtered ("static storage at address ");
> -		  printf_filtered ("%s", paddress (SYMBOL_VALUE_ADDRESS (msym)));
> -		}
> -	      break;
>  	    case LOC_OPTIMIZED_OUT:
>  	      printf_filtered ("optimized out.\n");
>  	      continue;
> --- gdb/mi/mi-cmd-stack.c	25 Jun 2008 15:15:42 -0000	1.41
> +++ gdb/mi/mi-cmd-stack.c	6 Aug 2008 15:14:15 -0000
> @@ -238,7 +238,6 @@ list_args_or_locals (int locals, int val
>  	    case LOC_LABEL:	/* local label           */
>  	    case LOC_BLOCK:	/* local function        */
>  	    case LOC_CONST_BYTES:	/* loc. byte seq.        */
> -	    case LOC_UNRESOLVED:	/* unresolved static     */
>  	    case LOC_OPTIMIZED_OUT:	/* optimized out         */
>  	      print_me = 0;
>  	      break;
> --- gdb/testsuite/gdb.dwarf2/dw2-noloc.S	1 Jan 2008 22:53:19 -0000	1.3
> +++ gdb/testsuite/gdb.dwarf2/dw2-noloc.S	6 Aug 2008 15:14:15 -0000
> @@ -69,6 +69,12 @@ func_cu1:
>  	.4byte		.Ltype_int-.Lcu1_begin	/* DW_AT_type */
>  	.byte		1			/* DW_AT_external */
> 
> +	.uleb128	5			/* Abbrev: DW_TAG_variable */
> +	.ascii		"optloc\0"		/* DW_AT_name */
> +	.4byte		.Ltype_int-.Lcu1_begin	/* DW_AT_type */
> +	.byte		1			/* DW_AT_external */
> +	.byte		0			/* DW_AT_location: length */
> +
>  	.byte		0			/* End of children of CU */
> 
>  .Lcu1_end:
> @@ -140,6 +146,20 @@ func_cu1:
>  	.byte		0x0			/* Terminator */
>  	.byte		0x0			/* Terminator */
> 
> +	.uleb128	5			/* Abbrev code */
> +	.uleb128	0x34			/* DW_TAG_variable */
> +	.byte		0			/* has_children */
> +	.uleb128	0x3			/* DW_AT_name */
> +	.uleb128	0x8			/* DW_FORM_string */
> +	.uleb128	0x49			/* DW_AT_type */
> +	.uleb128	0x13			/* DW_FORM_ref4 */
> +	.uleb128	0x3f			/* DW_AT_external */
> +	.uleb128	0xc			/* DW_FORM_flag */
> +	.uleb128	0x2			/* DW_AT_location */
> +	.uleb128	0xa			/* DW_FORM_block1 */
> +	.byte		0x0			/* Terminator */
> +	.byte		0x0			/* Terminator */
> +
>  	.byte		0x0			/* Terminator */
>  	.byte		0x0			/* Terminator */
> 
> --- gdb/testsuite/gdb.dwarf2/dw2-noloc.exp	1 Jan 2008 22:53:19 -0000	1.3
> +++ gdb/testsuite/gdb.dwarf2/dw2-noloc.exp	6 Aug 2008 15:14:15 -0000
> @@ -45,4 +45,5 @@ gdb_start
>  gdb_reinitialize_dir $srcdir/$subdir
>  gdb_load ${binfile}
> 
> -gdb_test "print noloc" "Address of symbol \"noloc\" is unknown." "print noloc"
> +gdb_test "print noloc" "No symbol \"noloc\" in current context." "print noloc"
> +gdb_test "print optloc" " = <value optimized out>" "print optloc"
> --- gdb/testsuite/gdb.threads/tls.c	29 Jul 2003 21:51:25 -0000	1.2
> +++ gdb/testsuite/gdb.threads/tls.c	6 Aug 2008 15:14:17 -0000
> @@ -20,6 +20,9 @@
>  __thread int a_thread_local;
>  __thread int another_thread_local;
> 
> +/* psymtabs->symtabs resolving check. */
> +extern __thread int file2_thread_local;
> +
>  /* Global variable just for info addr in gdb.  */
>  int a_global;
> 
> @@ -119,6 +122,12 @@ void *spin( vp )
>  }
> 
>  void
> +function_referencing_file2_thread_local (void)
> +{
> +  file2_thread_local = file2_thread_local;
> +}
> +
> +void
>  do_pass()
>  {
>      int i;
> --- gdb/testsuite/gdb.threads/tls.exp	6 Aug 2008 12:52:08 -0000	1.9
> +++ gdb/testsuite/gdb.threads/tls.exp	6 Aug 2008 15:14:17 -0000
> @@ -15,7 +15,9 @@
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> 
>  set testfile tls
> +set testfile2 tls2
>  set srcfile ${testfile}.c
> +set srcfile2 ${testfile2}.c
>  set binfile ${objdir}/${subdir}/${testfile}
> 
>  if [istarget "*-*-linux"] then {
> @@ -24,7 +26,7 @@ if [istarget "*-*-linux"] then {
>      set target_cflags ""
>  }
> 
> -if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
> +if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile} ${srcdir}/${subdir}/${srcfile2}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
>      return -1
>  }
> 
> @@ -284,6 +286,15 @@ gdb_test "info address a_global" \
>  setup_kfail "gdb/1294" "*-*-*"
>  gdb_test "info address me" ".*me.*is a variable at offset.*" "info address me"
> 
> +
> +# Test LOC_UNRESOLVED references for the `extern' variables.
> +
> +gdb_test "p a_thread_local" " = \[0-9\]+"
> +# Here it could crash with: Cannot access memory at address 0x0
> +gdb_test "p file2_thread_local" " = \[0-9\]+"
> +# Here it could crash with: Cannot access memory at address 0x0
> +gdb_test "p a_thread_local" " = \[0-9\]+"
> +
>  # Done!
>  #
>  gdb_exit
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.threads/tls2.c	6 Aug 2008 15:14:17 -0000
> @@ -0,0 +1,28 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2008 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +   Please email any bugs, comments, and/or additions to this file to:
> +   bug-gdb@prep.ai.mit.edu  */
> +
> +extern __thread int a_thread_local;
> +__thread int file2_thread_local;
> +
> +void
> +function_referencing_a_thread_local (void)
> +{
> +  a_thread_local = a_thread_local;
> +}

Re: [patch] Accessing tls variables across files causes a bug

[Gary Funck]

On 09/10/08 09:43:13, Vinay Sridhar wrote:
> Any idea when the below changes are likely to go in?

Recently, we've run into a situation where we need this bug fix.
I've corresponded with Jan on this, and he mentioned
that the fix may have some unknown impact or regressions on
the main line (though none that he knows of), and that
this patch is pending review, before it can be committed.

Have the GDB maintainers had a chance to review this patch?
Is there anything that I can do to help move it along?

thanks,

- Gary

Re: [patch] Accessing tls variables across files causes a bug

[Jan Kratochvil]

On Wed, 26 Nov 2008 18:23:32 +0100, Gary Funck wrote:
> Recently, we've run into a situation where we need this bug fix.
> I've corresponded with Jan on this, and he mentioned
> that the fix may have some unknown impact or regressions

The TLS-fixing patch breaks the new testcase since that time -
- `gdb.dwarf2/dw2-cu-size.exp' and I agree the patch is wrong.
I will post an updated patch.


Sorry,
Jan

Re: [patch] Accessing tls variables across files causes a bug

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

Hi,

(re)patched.  GDB misses getters to do it universally.

Testsuite addition, no regressions (x86_64-unknown-linux-gnu):
+PASS: gdb.threads/tls.exp: p a_thread_local
+PASS: gdb.threads/tls.exp: p file2_thread_local
+PASS: gdb.threads/tls.exp: info address file2_thread_local
+PASS: gdb.threads/tls.exp: p a_thread_local second time
+PASS: gdb.threads/tls.exp: info address a_thread_local


Regards,
Jan

[-- Attachment #2: gdb-extern-tls.patch --]
[-- Type: text/plain, Size: 6500 bytes --]

2008-11-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix resolving external references to TLS variables.
	* findvar.c: Include `objfiles.h'.
	(read_var_value <LOC_UNRESOLVED>): New variable `obj_section'.  Handle
	SEC_THREAD_LOCAL variables.
	* printcmd.c (address_info <LOC_UNRESOLVED>): New variable
	`obj_section'.  Handle SEC_THREAD_LOCAL variables.

2008-11-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Test resolving external references to TLS variables.
	* gdb.threads/tls.exp: New tests to examine A_THREAD_LOCAL and
	FILE2_THREAD_LOCAL.
	(testfile2, srcfile2): New variables.
	* gdb.threads/tls.c (file2_thread_local)
	(function_referencing_file2_thread_local): New.
	* gdb.threads/tls2.c: New file.

--- gdb/findvar.c	5 Sep 2008 11:37:17 -0000	1.118
+++ gdb/findvar.c	30 Nov 2008 21:10:02 -0000
@@ -34,6 +34,7 @@
 #include "regcache.h"
 #include "user-regs.h"
 #include "block.h"
+#include "objfiles.h"
 
 /* Basic byte-swapping routines.  GDB has needed these for a long time...
    All extract a target-format integer at ADDR which is LEN bytes long.  */
@@ -536,6 +537,7 @@ read_var_value (struct symbol *var, stru
     case LOC_UNRESOLVED:
       {
 	struct minimal_symbol *msym;
+	struct obj_section *obj_section;
 
 	msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (var), NULL, NULL);
 	if (msym == NULL)
@@ -545,6 +547,12 @@ read_var_value (struct symbol *var, stru
 					   SYMBOL_OBJ_SECTION (msym));
 	else
 	  addr = SYMBOL_VALUE_ADDRESS (msym);
+
+	/* SYMBOL_VALUE_ADDRESS should return the translated address.  */
+	obj_section = SYMBOL_OBJ_SECTION (msym);
+	if (obj_section
+	    && (obj_section->the_bfd_section->flags & SEC_THREAD_LOCAL) != 0)
+	  addr = target_translate_tls_address (obj_section->objfile, addr);
       }
       break;
 
--- gdb/printcmd.c	20 Nov 2008 16:13:11 -0000	1.138
+++ gdb/printcmd.c	30 Nov 2008 21:10:06 -0000
@@ -1234,6 +1234,7 @@ address_info (char *exp, int from_tty)
     case LOC_UNRESOLVED:
       {
 	struct minimal_symbol *msym;
+	struct obj_section *obj_section;
 
 	msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (sym), NULL, NULL);
 	if (msym == NULL)
@@ -1241,8 +1242,19 @@ address_info (char *exp, int from_tty)
 	else
 	  {
 	    section = SYMBOL_OBJ_SECTION (msym);
-	    printf_filtered (_("static storage at address "));
 	    load_addr = SYMBOL_VALUE_ADDRESS (msym);
+
+	    /* SYMBOL_VALUE_ADDRESS should return the translated address.  */
+	    if (section
+		&& (section->the_bfd_section->flags & SEC_THREAD_LOCAL) != 0)
+	      {
+		printf_filtered (_("a thread-local variable at offset %s "
+				   "at final address "), paddr_nz (load_addr));
+		load_addr = target_translate_tls_address (section->objfile,
+							  load_addr);
+	      }
+	    else
+	      printf_filtered (_("static storage at address "));
 	    fputs_filtered (paddress (load_addr), gdb_stdout);
 	    if (section_is_overlay (section))
 	      {
--- gdb/testsuite/gdb.threads/tls.c	29 Jul 2003 21:51:25 -0000	1.2
+++ gdb/testsuite/gdb.threads/tls.c	30 Nov 2008 21:10:09 -0000
@@ -20,6 +20,9 @@
 __thread int a_thread_local;
 __thread int another_thread_local;
 
+/* psymtabs->symtabs resolving check.  */
+extern __thread int file2_thread_local;
+
 /* Global variable just for info addr in gdb.  */
 int a_global;
 
@@ -119,6 +122,12 @@ void *spin( vp )
 }
 
 void
+function_referencing_file2_thread_local (void)
+{
+  file2_thread_local = file2_thread_local;
+}
+
+void
 do_pass()
 {
     int i;
--- gdb/testsuite/gdb.threads/tls.exp	6 Aug 2008 12:52:08 -0000	1.9
+++ gdb/testsuite/gdb.threads/tls.exp	30 Nov 2008 21:10:09 -0000
@@ -15,7 +15,9 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 set testfile tls
+set testfile2 tls2
 set srcfile ${testfile}.c
+set srcfile2 ${testfile2}.c
 set binfile ${objdir}/${subdir}/${testfile}
 
 if [istarget "*-*-linux"] then {
@@ -24,7 +26,7 @@ if [istarget "*-*-linux"] then {
     set target_cflags ""
 }
 
-if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile} ${srcdir}/${subdir}/${srcfile2}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
     return -1
 }
 
@@ -284,6 +286,20 @@ gdb_test "info address a_global" \
 setup_kfail "gdb/1294" "*-*-*"
 gdb_test "info address me" ".*me.*is a variable at offset.*" "info address me"
 
+
+# Test LOC_UNRESOLVED references resolving for `extern' TLS variables.
+
+gdb_test "p a_thread_local" " = \[0-9\]+"
+# Here it could crash with: Cannot access memory at address 0x0
+gdb_test "p file2_thread_local" " = \[0-9\]+"
+# Depending on the current lookup scope we may get two answers:
+# LOC_UNRESOLVED: Symbol "file2_thread_local" is a thread-local variable at offset 8 at final address 0x7ffff7fdb94c.
+# LOC_COMPUTED:   Symbol "file2_thread_local" is a thread-local variable at offset 8 in the thread-local storage for `.../gdb.threads/tls'.
+gdb_test "info address file2_thread_local" "Symbol \"file2_thread_local\" is a thread-local variable.*"
+# Here it could also crash with: Cannot access memory at address 0x0
+gdb_test "p a_thread_local" " = \[0-9\]+" "p a_thread_local second time"
+gdb_test "info address a_thread_local" "Symbol \"a_thread_local\" is a thread-local variable.*"
+
 # Done!
 #
 gdb_exit
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.threads/tls2.c	30 Nov 2008 21:10:09 -0000
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@prep.ai.mit.edu  */
+
+extern __thread int a_thread_local;
+__thread int file2_thread_local;
+
+void
+function_referencing_a_thread_local (void)
+{
+  a_thread_local = a_thread_local;
+}

Re: [patch] Accessing tls variables across files causes a bug

[Ulrich Weigand]

Jan Kratochvil wrote:

> +	/* SYMBOL_VALUE_ADDRESS should return the translated address.  */

I'm not sure I understand this comment -- the translated address of
a thread-local variable obviously depends on the thread in which it
is evaluated, so how should a single location like SYMBOL_VALUE_ADDRESS
be able to hold that value for all threads?

Otherwise, I guess it makes sense to do that translation for 
LOC_UNRESOLVED TLS symbols in read_var_value, just like it would
be done for LOC_COMPUTED symbols; in both cases assuming evaluation
in the current thread.

> +	    /* SYMBOL_VALUE_ADDRESS should return the translated address.  */
> +	    if (section
> +		&& (section->the_bfd_section->flags & SEC_THREAD_LOCAL) != 0)
> +	      {
> +		printf_filtered (_("a thread-local variable at offset %s "
> +				   "at final address "), paddr_nz (load_addr));
> +		load_addr = target_translate_tls_address (section->objfile,
> +							  load_addr);
> +	      }

Again, I think "at final address" may be misleading; if we give an absolute
address, it should explicitly mention it is relative to the current thread.

On the other hand, I'm wondering if we should perform the resolution here
at all; isn't "info address" also allowed when the target is not actually
running, so we don't even have a current thread?

In the LOC_COMPUTED case, we'd output something like "at offset ...
in the thread-local storage for ...".  For consistency reasons, I'd
prefer to have the same output in the LOC_UNDEFINED case as well.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Accessing tls variables across files causes a bug

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]

On Mon, 01 Dec 2008 19:29:55 +0100, Ulrich Weigand wrote:

Followed your advices, updated the patch.


Thanks,
Jan

------------------------------------------------------------------------------

> Jan Kratochvil wrote:
> 
> > +	/* SYMBOL_VALUE_ADDRESS should return the translated address.  */
> 
> I'm not sure I understand this comment -- the translated address of
> a thread-local variable obviously depends on the thread in which it
> is evaluated, so how should a single location like SYMBOL_VALUE_ADDRESS
> be able to hold that value for all threads?

SYMBOL_VALUE_ADDRESS is used many times across GDB code and I assumed in most
of such cases it would be more appropriate to return the absolute address
valid for current inferior_ptid.

But I now briefly checked all these points and found out in fact all of them
work with SYMBOL_VALUE_ADDRESS referring to function symbols - PC values (just
except value_static_field() but that is also not in use with TLS symbols).
Sure PC values are never TLS-translated.


> > +	    /* SYMBOL_VALUE_ADDRESS should return the translated address.  */
> > +	    if (section
> > +		&& (section->the_bfd_section->flags & SEC_THREAD_LOCAL) != 0)
> > +	      {
> > +		printf_filtered (_("a thread-local variable at offset %s "
> > +				   "at final address "), paddr_nz (load_addr));
> > +		load_addr = target_translate_tls_address (section->objfile,
> > +							  load_addr);
> > +	      }
> 
> Again, I think "at final address" may be misleading; if we give an absolute
> address, it should explicitly mention it is relative to the current thread.

`final' was put there as it printed before:

Symbol "X" is a thread-local variable at offset 0x1234 at address 0xabcd0000.

and I found it ambiguous whether the bytes of the variable X are in fact
placed at (a) address 0xabcd0000 or (b) address 0xabcd1234.


> In the LOC_COMPUTED case, we'd output something like "at offset ...
> in the thread-local storage for ...".  For consistency reasons, I'd
> prefer to have the same output in the LOC_UNDEFINED case as well.

While I copied the message from the LOC_COMPUTED case I agree it should be
consistent and if I wish the absolute address there it should be printed even
in the LOC_COMPUTED case which would be more a scope of a different patch.


> On the other hand, I'm wondering if we should perform the resolution here
> at all; isn't "info address" also allowed when the target is not actually
> running, so we don't even have a current thread?

While I forgot about this case I find the current behavior appropriate:

(gdb) info address a_thread_local
Symbol "a_thread_local" is a thread-local variable at offset 0 at final address Cannot find thread-local variables on this target

But it is no longer printed due to the consistency advice above.


On Mon, 01 Dec 2008 19:29:31 +0100, Gary Funck wrote:
> --- gdb/printcmd.c      20 Nov 2008 16:13:11 -0000      1.138
> +++ gdb/printcmd.c      30 Nov 2008 21:10:06 -0000
> @@ -1234,6 +1234,7 @@ address_info (char *exp, int from_tty)
> +       struct obj_section *obj_section;
> Is obj_section referenced?

Removed.

[-- Attachment #2: gdb-extern-tls2.patch --]
[-- Type: text/plain, Size: 6509 bytes --]

2008-12-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix resolving external references to TLS variables.
	* findvar.c: Include `objfiles.h'.
	(read_var_value <LOC_UNRESOLVED>): New variable `obj_section'.  Handle
	SEC_THREAD_LOCAL variables.
	* printcmd.c (address_info <LOC_UNRESOLVED>): Handle SEC_THREAD_LOCAL
	variables.

2008-12-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Test resolving external references to TLS variables.
	* gdb.threads/tls.exp: New tests to examine A_THREAD_LOCAL and
	FILE2_THREAD_LOCAL.
	(testfile2, srcfile2): New variables.
	* gdb.threads/tls.c (file2_thread_local)
	(function_referencing_file2_thread_local): New.
	* gdb.threads/tls2.c: New file.

--- gdb/findvar.c	5 Sep 2008 11:37:17 -0000	1.118
+++ gdb/findvar.c	1 Dec 2008 21:50:25 -0000
@@ -34,6 +34,7 @@
 #include "regcache.h"
 #include "user-regs.h"
 #include "block.h"
+#include "objfiles.h"
 
 /* Basic byte-swapping routines.  GDB has needed these for a long time...
    All extract a target-format integer at ADDR which is LEN bytes long.  */
@@ -536,6 +537,7 @@ read_var_value (struct symbol *var, stru
     case LOC_UNRESOLVED:
       {
 	struct minimal_symbol *msym;
+	struct obj_section *obj_section;
 
 	msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (var), NULL, NULL);
 	if (msym == NULL)
@@ -545,6 +547,11 @@ read_var_value (struct symbol *var, stru
 					   SYMBOL_OBJ_SECTION (msym));
 	else
 	  addr = SYMBOL_VALUE_ADDRESS (msym);
+
+	obj_section = SYMBOL_OBJ_SECTION (msym);
+	if (obj_section
+	    && (obj_section->the_bfd_section->flags & SEC_THREAD_LOCAL) != 0)
+	  addr = target_translate_tls_address (obj_section->objfile, addr);
       }
       break;
 
--- gdb/printcmd.c	20 Nov 2008 16:13:11 -0000	1.138
+++ gdb/printcmd.c	1 Dec 2008 21:50:28 -0000
@@ -1241,16 +1241,25 @@ address_info (char *exp, int from_tty)
 	else
 	  {
 	    section = SYMBOL_OBJ_SECTION (msym);
-	    printf_filtered (_("static storage at address "));
 	    load_addr = SYMBOL_VALUE_ADDRESS (msym);
-	    fputs_filtered (paddress (load_addr), gdb_stdout);
-	    if (section_is_overlay (section))
+
+	    if (section
+		&& (section->the_bfd_section->flags & SEC_THREAD_LOCAL) != 0)
+	      printf_filtered (_("a thread-local variable at offset %s "
+				 "in the thread-local storage for `%s'"),
+			       paddr_nz (load_addr), section->objfile->name);
+	    else
 	      {
-		load_addr = overlay_unmapped_address (load_addr, section);
-		printf_filtered (_(",\n -- loaded at "));
+		printf_filtered (_("static storage at address "));
 		fputs_filtered (paddress (load_addr), gdb_stdout);
-		printf_filtered (_(" in overlay section %s"),
-				 section->the_bfd_section->name);
+		if (section_is_overlay (section))
+		  {
+		    load_addr = overlay_unmapped_address (load_addr, section);
+		    printf_filtered (_(",\n -- loaded at "));
+		    fputs_filtered (paddress (load_addr), gdb_stdout);
+		    printf_filtered (_(" in overlay section %s"),
+				     section->the_bfd_section->name);
+		  }
 	      }
 	  }
       }
--- gdb/testsuite/gdb.threads/tls.c	29 Jul 2003 21:51:25 -0000	1.2
+++ gdb/testsuite/gdb.threads/tls.c	1 Dec 2008 21:50:30 -0000
@@ -20,6 +20,9 @@
 __thread int a_thread_local;
 __thread int another_thread_local;
 
+/* psymtabs->symtabs resolving check.  */
+extern __thread int file2_thread_local;
+
 /* Global variable just for info addr in gdb.  */
 int a_global;
 
@@ -119,6 +122,12 @@ void *spin( vp )
 }
 
 void
+function_referencing_file2_thread_local (void)
+{
+  file2_thread_local = file2_thread_local;
+}
+
+void
 do_pass()
 {
     int i;
--- gdb/testsuite/gdb.threads/tls.exp	6 Aug 2008 12:52:08 -0000	1.9
+++ gdb/testsuite/gdb.threads/tls.exp	1 Dec 2008 21:50:30 -0000
@@ -15,7 +15,9 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 set testfile tls
+set testfile2 tls2
 set srcfile ${testfile}.c
+set srcfile2 ${testfile2}.c
 set binfile ${objdir}/${subdir}/${testfile}
 
 if [istarget "*-*-linux"] then {
@@ -24,7 +26,7 @@ if [istarget "*-*-linux"] then {
     set target_cflags ""
 }
 
-if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile} ${srcdir}/${subdir}/${srcfile2}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
     return -1
 }
 
@@ -284,6 +286,20 @@ gdb_test "info address a_global" \
 setup_kfail "gdb/1294" "*-*-*"
 gdb_test "info address me" ".*me.*is a variable at offset.*" "info address me"
 
+
+# Test LOC_UNRESOLVED references resolving for `extern' TLS variables.
+
+gdb_test "p a_thread_local" " = \[0-9\]+"
+# Here it could crash with: Cannot access memory at address 0x0
+gdb_test "p file2_thread_local" " = \[0-9\]+"
+# Depending on the current lookup scope we get LOC_UNRESOLVED or LOC_COMPUTED
+# both printing:
+# Symbol "file2_thread_local" is a thread-local variable at offset 8 in the thread-local storage for `.../gdb.threads/tls'.
+gdb_test "info address file2_thread_local" "Symbol \"file2_thread_local\" is a thread-local variable.*"
+# Here it could also crash with: Cannot access memory at address 0x0
+gdb_test "p a_thread_local" " = \[0-9\]+" "p a_thread_local second time"
+gdb_test "info address a_thread_local" "Symbol \"a_thread_local\" is a thread-local variable.*"
+
 # Done!
 #
 gdb_exit
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.threads/tls2.c	1 Dec 2008 21:50:30 -0000
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@prep.ai.mit.edu  */
+
+extern __thread int a_thread_local;
+__thread int file2_thread_local;
+
+void
+function_referencing_a_thread_local (void)
+{
+  a_thread_local = a_thread_local;
+}

Re: [patch] Accessing tls variables across files causes a bug

[Ulrich Weigand]

Jan Kratochvil wrote:

> > I'm not sure I understand this comment -- the translated address of
> > a thread-local variable obviously depends on the thread in which it
> > is evaluated, so how should a single location like SYMBOL_VALUE_ADDRESS
> > be able to hold that value for all threads?
> 
> SYMBOL_VALUE_ADDRESS is used many times across GDB code and I assumed in most
> of such cases it would be more appropriate to return the absolute address
> valid for current inferior_ptid.
>
> But I now briefly checked all these points and found out in fact all of them
> work with SYMBOL_VALUE_ADDRESS referring to function symbols - PC values (just
> except value_static_field() but that is also not in use with TLS symbols).
> Sure PC values are never TLS-translated.

Agreed.

> > In the LOC_COMPUTED case, we'd output something like "at offset ...
> > in the thread-local storage for ...".  For consistency reasons, I'd
> > prefer to have the same output in the LOC_UNDEFINED case as well.
> 
> While I copied the message from the LOC_COMPUTED case I agree it should be
> consistent and if I wish the absolute address there it should be printed even
> in the LOC_COMPUTED case which would be more a scope of a different patch.

OK, thanks.


> 2008-12-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix resolving external references to TLS variables.
> 	* findvar.c: Include `objfiles.h'.
> 	(read_var_value <LOC_UNRESOLVED>): New variable `obj_section'.  Handle
> 	SEC_THREAD_LOCAL variables.
> 	* printcmd.c (address_info <LOC_UNRESOLVED>): Handle SEC_THREAD_LOCAL
> 	variables.
> 
> 2008-12-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Test resolving external references to TLS variables.
> 	* gdb.threads/tls.exp: New tests to examine A_THREAD_LOCAL and
> 	FILE2_THREAD_LOCAL.
> 	(testfile2, srcfile2): New variables.
> 	* gdb.threads/tls.c (file2_thread_local)
> 	(function_referencing_file2_thread_local): New.
> 	* gdb.threads/tls2.c: New file.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Accessing tls variables across files causes a bug

[Gary Funck]

On 12/02/08 14:52:33, Ulrich Weigand wrote:
> > 2008-12-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Fix resolving external references to TLS variables.
> > 	* findvar.c: Include `objfiles.h'.
> > 	(read_var_value <LOC_UNRESOLVED>): New variable `obj_section'.  Handle
> > 	SEC_THREAD_LOCAL variables.
> > 	* printcmd.c (address_info <LOC_UNRESOLVED>): Handle SEC_THREAD_LOCAL
> > 	variables.
> > 
> > 2008-12-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Test resolving external references to TLS variables.
> > 	* gdb.threads/tls.exp: New tests to examine A_THREAD_LOCAL and
> > 	FILE2_THREAD_LOCAL.
> > 	(testfile2, srcfile2): New variables.
> > 	* gdb.threads/tls.c (file2_thread_local)
> > 	(function_referencing_file2_thread_local): New.
> > 	* gdb.threads/tls2.c: New file.
> 
> This is OK. Thanks, Ulrich

If anyone ports this patch to 6.8 or can share what's
needed to implement the patch in 6.8, I'd appreciate
learning about that.  I quickly tried it, and it seems
that this part of the fix runs into difficulties:

diff -r1.4 findvar.c
36a37
> #include "objfiles.h"
580a582
>       struct obj_section *obj_section;
589a592,596
> 
>       obj_section = SYMBOL_OBJ_SECTION (msym);
>       if (obj_section
>           && (obj_section->the_bfd_section->flags & SEC_THREAD_LOCAL) != 0)
>         addr = target_translate_tls_address (obj_section->objfile, addr);


with related difficulties in printcmd.c.

SYMBOL_OBJ_SECTION was introduced in this mod to symtab.h

revision 1.131
date: 2008/09/05 11:37:17;  author: uweigand;  state: Exp;  lines: +17 -15
        * breakpoint.h (struct bp_location): Change type of section
        member to "struct obj_section *".
        * tracepoint.h (struct tracepoint): Likewise.
        * symtab.h (struct general_symbol_info): Replace bfd_section
        member with obj_section.
        (struct symtab_and_line): Change type of section member to
        "struct obj_section *".
        (SYMBOL_BFD_SECTION): Remove macro, replace by ...
        (SYMBOL_OBJ_SECTION): ... this.


thanks,
- Gary

Re: [patch] Accessing tls variables across files causes a bug

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 195 bytes --]

On Tue, 02 Dec 2008 15:52:46 +0100, Gary Funck wrote:
> If anyone ports this patch to 6.8

Did not run full testsuite runs (and the testcase could be better) but it
looks working.


Regards,
Jan

[-- Attachment #2: gdb-6.8-extern-tls2.patch --]
[-- Type: text/plain, Size: 6673 bytes --]

--- ./gdb/Makefile.in	2008-03-17 13:15:08.000000000 +0100
+++ ./gdb/Makefile.in	2008-12-02 16:04:02.000000000 +0100
@@ -2104,7 +2104,7 @@ f-exp.o: f-exp.c $(defs_h) $(gdb_string_
 findvar.o: findvar.c $(defs_h) $(symtab_h) $(gdbtypes_h) $(frame_h) \
 	$(value_h) $(gdbcore_h) $(inferior_h) $(target_h) $(gdb_string_h) \
 	$(gdb_assert_h) $(floatformat_h) $(symfile_h) $(regcache_h) \
-	$(user_regs_h) $(block_h)
+	$(user_regs_h) $(block_h) $(objfiles_h)
 f-lang.o: f-lang.c $(defs_h) $(gdb_string_h) $(symtab_h) $(gdbtypes_h) \
 	$(expression_h) $(parser_defs_h) $(language_h) $(f_lang_h) \
 	$(valprint_h) $(value_h)
--- ./gdb/findvar.c	2008-01-01 23:53:09.000000000 +0100
+++ ./gdb/findvar.c	2008-12-02 16:03:49.000000000 +0100
@@ -34,6 +34,7 @@
 #include "regcache.h"
 #include "user-regs.h"
 #include "block.h"
+#include "objfiles.h"
 
 /* Basic byte-swapping routines.  GDB has needed these for a long time...
    All extract a target-format integer at ADDR which is LEN bytes long.  */
@@ -559,6 +560,7 @@ addresses have not been bound by the dyn
     case LOC_UNRESOLVED:
       {
 	struct minimal_symbol *msym;
+	asection *section;
 
 	msym = lookup_minimal_symbol (DEPRECATED_SYMBOL_NAME (var), NULL, NULL);
 	if (msym == NULL)
@@ -568,6 +570,19 @@ addresses have not been bound by the dyn
 					   SYMBOL_BFD_SECTION (msym));
 	else
 	  addr = SYMBOL_VALUE_ADDRESS (msym);
+
+	section = SYMBOL_BFD_SECTION (msym);
+	if (section && (section->flags & SEC_THREAD_LOCAL) != 0)
+	  {
+	    struct objfile *objfile;
+
+	    ALL_OBJFILES (objfile)
+	      if (objfile->obfd == section->owner)
+	        {
+		  addr = target_translate_tls_address (objfile, addr);
+		  break;
+		}
+	  }
       }
       break;
 
--- ./gdb/printcmd.c	2008-01-30 20:19:51.000000000 +0100
+++ ./gdb/printcmd.c	2008-12-02 16:05:51.000000000 +0100
@@ -1219,15 +1219,32 @@ address_info (char *exp, int from_tty)
 	else
 	  {
 	    section = SYMBOL_BFD_SECTION (msym);
-	    printf_filtered (_("static storage at address "));
 	    load_addr = SYMBOL_VALUE_ADDRESS (msym);
-	    fputs_filtered (paddress (load_addr), gdb_stdout);
-	    if (section_is_overlay (section))
+
+	    if (section && (section->flags & SEC_THREAD_LOCAL) != 0)
+	      {
+		struct objfile *objfile;
+
+		ALL_OBJFILES (objfile)
+		  if (objfile->obfd == section->owner)
+		    {
+		      printf_filtered (_("a thread-local variable at offset %s "
+					 "in the thread-local storage for `%s'"),
+				       paddr_nz (load_addr), objfile->name);
+		      break;
+		    }
+	      }
+	    else
 	      {
-		load_addr = overlay_unmapped_address (load_addr, section);
-		printf_filtered (_(",\n -- loaded at "));
+		printf_filtered (_("static storage at address "));
 		fputs_filtered (paddress (load_addr), gdb_stdout);
-		printf_filtered (_(" in overlay section %s"), section->name);
+		if (section_is_overlay (section))
+		  {
+		    load_addr = overlay_unmapped_address (load_addr, section);
+		    printf_filtered (_(",\n -- loaded at "));
+		    fputs_filtered (paddress (load_addr), gdb_stdout);
+		    printf_filtered (_(" in overlay section %s"), section->name);
+		  }
 	      }
 	  }
       }
--- ./gdb/testsuite/gdb.threads/tls.c	2003-07-29 23:51:25.000000000 +0200
+++ ./gdb/testsuite/gdb.threads/tls.c	2008-12-02 15:56:48.000000000 +0100
@@ -20,6 +20,9 @@
 __thread int a_thread_local;
 __thread int another_thread_local;
 
+/* psymtabs->symtabs resolving check.  */
+extern __thread int file2_thread_local;
+
 /* Global variable just for info addr in gdb.  */
 int a_global;
 
@@ -119,6 +122,12 @@ void *spin( vp )
 }
 
 void
+function_referencing_file2_thread_local (void)
+{
+  file2_thread_local = file2_thread_local;
+}
+
+void
 do_pass()
 {
     int i;
--- ./gdb/testsuite/gdb.threads/tls.exp	2008-01-01 23:53:22.000000000 +0100
+++ ./gdb/testsuite/gdb.threads/tls.exp	2008-12-02 15:56:48.000000000 +0100
@@ -18,7 +18,9 @@
 # bug-gdb@prep.ai.mit.edu
 
 set testfile tls
+set testfile2 tls2
 set srcfile ${testfile}.c
+set srcfile2 ${testfile2}.c
 set binfile ${objdir}/${subdir}/${testfile}
 
 if [istarget "*-*-linux"] then {
@@ -27,7 +29,7 @@ if [istarget "*-*-linux"] then {
     set target_cflags ""
 }
 
-if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile} ${srcdir}/${subdir}/${srcfile2}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
     return -1
 }
 
@@ -287,6 +289,20 @@ gdb_test "info address a_global" \
 setup_kfail "gdb/1294" "*-*-*"
 gdb_test "info address me" ".*me.*is a variable at offset.*" "info address me"
 
+
+# Test LOC_UNRESOLVED references resolving for `extern' TLS variables.
+
+gdb_test "p a_thread_local" " = \[0-9\]+"
+# Here it could crash with: Cannot access memory at address 0x0
+gdb_test "p file2_thread_local" " = \[0-9\]+"
+# Depending on the current lookup scope we get LOC_UNRESOLVED or LOC_COMPUTED
+# both printing:
+# Symbol "file2_thread_local" is a thread-local variable at offset 8 in the thread-local storage for `.../gdb.threads/tls'.
+gdb_test "info address file2_thread_local" "Symbol \"file2_thread_local\" is a thread-local variable.*"
+# Here it could also crash with: Cannot access memory at address 0x0
+gdb_test "p a_thread_local" " = \[0-9\]+" "p a_thread_local second time"
+gdb_test "info address a_thread_local" "Symbol \"a_thread_local\" is a thread-local variable.*"
+
 # Done!
 #
 gdb_exit
--- ./gdb/testsuite/gdb.threads/tls2.c	1970-01-01 01:00:00.000000000 +0100
+++ ./gdb/testsuite/gdb.threads/tls2.c	2008-12-02 15:56:48.000000000 +0100
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@prep.ai.mit.edu  */
+
+extern __thread int a_thread_local;
+__thread int file2_thread_local;
+
+void
+function_referencing_a_thread_local (void)
+{
+  a_thread_local = a_thread_local;
+}

Re: [patch] Accessing tls variables across files causes a bug

[Gary Funck]

On 12/02/08 16:11:45, Jan Kratochvil wrote:
> On Tue, 02 Dec 2008 15:52:46 +0100, Gary Funck wrote:
> > If anyone ports this patch to 6.8
> 
> Did not run full testsuite runs (and the testcase could be better) but it
> looks working.

Jan, works like a champ.  Thanks.  (I didn't run the full test
suite either, but it fixes the case in which the access/display
of an extern TLS variable was failing here.)