This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Sat, 2008-08-02 at 19:18 +0200, Jan Kratochvil wrote:
> Hi,
> 
> just coded it as Vinay Sridhar suggested, thanks.  lookup_block_symbol()
> behaves right as it has only the scope of a single BLOCK.
> 
> While there should be some more general approach for multiple instances of
> a single symbol name in different files this patch has no known regressions.
> 
> 
> Regards,
> Jan
> 
> 
> On Thu, 31 Jul 2008 06:53:40 +0200, Vinay Sridhar wrote:
> [...]
> > We came across this bug when debugging tls variables.
> > 
> > ------------------------
> > Consider file1.c : 
> > ------------------------
> > #include<...>
> > ...
> > __thread int thr;
> > int stopHere;
> > ...
> > void foo()
> > {
> > stopHere=8;
> > }
> > 
> > main()
> > {
> > thr = 0;
> > foo();
> > }
> > 
> > -------------------------
> > Now consider file2.c : 
> > -------------------------
> > 
> > __thread char myChar;
> > extern __thread int thr;
> > 
> > void foo1()
> > {
> > myChar = 'a' + thr;
> > }
> > 
> > 
> > On compiling these 2 and creating the executable, the bug is produced on
> > running the following commands : 
> > 
> > 1. gdb <executable>
> > 2. b 8 //at the stopHere part of file1
> > 3. run
> > 4. print thr //prints value of thr
> > 5. print myChar
> > 6. print thr
> > 
> > Now on the final print command, you get a "Cannot access memory at
> > address 0x4"
> > 
> > The reason for this is "thr", being a tls variable, is stored as an
> > offset in the minsymtab. So for the "extern thr", gdb sets it to
> > LOC_UNRESOLVED and by convention, looks up minsymtab to find its
> > address. Here it justs picks up the offset and tries to dereference it.
> > This works fine for other global variables that are extern but fails for
> > "tls" variables that are extern. 
> > 
> > What I propose is that in "lookup_symbol_aux_symtabs()" @symtab.c, when
> > the tls variable is to be looked up, once we find it in the current
> > file's symtab and is at LOC_UNRESOLVED, we ignore it and look further in
> > other symtabs of the object file as well and find one that has
> > LOC_COMPUTED, i.e, we look into the symtab of the file which has defined
> > this and retrieve the symbol information from this block of the objfile.
> > This will be restricted to tls variables only. 
> > 
> > Is this fix acceptable?
> > 
> > Request for Comments..
> > 
> > Regards,
> > Vinay 
> > 
> > ïVinay Sridhar,
> > IBM-Linux Technology Centre
> > vinay@linux.vnet.ibm.com
> 
> plain text document attachment (gdb-extern-loc_unresolved.patch)
> 2008-08-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* symtab.c (lookup_symbol_aux_symtabs): Continue the symtabs search if
> 	only LOC_UNRESOLVED or LOC_OPTIMIZED_OUT symbol was found so far.
> 	Fix suggested by Vinay Sridhar <vinay@linux.vnet.ibm.com>.
> 
> 2008-08-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.threads/tls.exp: New tests to print `a_thread_local' after
> 	`file2_thread_local'.
> 	(testfile2, srcfile2): New variables.
> 	* gdb.threads/tls2.c: New file.
> 
> --- ./gdb/symtab.c	21 Jul 2008 16:47:10 -0000	1.192
> +++ ./gdb/symtab.c	2 Aug 2008 17:05:36 -0000
> @@ -1475,7 +1475,7 @@ lookup_symbol_aux_symtabs (int block_ind
>  			   const char *name, const char *linkage_name,
>  			   const domain_enum domain)
>  {
> -  struct symbol *sym;
> +  struct symbol *sym_best = NULL;
>    struct objfile *objfile;
>    struct blockvector *bv;
>    const struct block *block;
> @@ -1483,16 +1483,26 @@ lookup_symbol_aux_symtabs (int block_ind
> 
>    ALL_PRIMARY_SYMTABS (objfile, s)
>    {
> +    struct symbol *sym;
> +
>      bv = BLOCKVECTOR (s);
>      block = BLOCKVECTOR_BLOCK (bv, block_index);
>      sym = lookup_block_symbol (block, name, linkage_name, domain);
> -    if (sym)
> +    if (sym != NULL)
>        {
> -	block_found = block;
> -	return fixup_symbol_section (sym, objfile);
> +	sym_best = sym;
> +	if (SYMBOL_CLASS (sym) != LOC_UNRESOLVED
> +	    && SYMBOL_CLASS (sym) != LOC_OPTIMIZED_OUT)
> +	  break;
>        }
>    }
> 
> +  if (sym_best)
> +    {
> +      block_found = block;
> +      return fixup_symbol_section (sym_best, objfile);
> +    }
> +
>    return NULL;
>  }
> 
> --- ./gdb/testsuite/gdb.threads/tls.exp	1 Jan 2008 22:53:22 -0000	1.8
> +++ ./gdb/testsuite/gdb.threads/tls.exp	2 Aug 2008 17:05:40 -0000
> @@ -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,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\]+"
> +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	2 Aug 2008 17:05:40 -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;
> +}


Jan,

I have a few additions to suggest:

1. The patch does not ensure that this is performed for "tls" variables
only. Here the behaviour is modified for all variables that are declared
extern. This will be modifying existing gdb behaviour. Is this
acceptable?

2.  If we access an extern tls variable in the file containing main()
before the other symtabs are linked in, we again get a "cannot access
memory at address ..." error, i.e if I declare a thread var as tls in
file2 and it is declared extern in file1, then access of this variable
results in the error.
	ïGDB first links in the block containing main(). Each time a variable
in a different file is accessed, the symtab of that file is linked in.
Gdb then anchors itself there and looks for symbols relative to this
symtab.
Hence, we need to link them in for this scenario. What happens currently
is that "sym_best" is retained and the usual msymtab lookup is
happening. So we "force link-in" the symtab by going the psymtab route.

For this we need to find out if a particular variable is tls or not. I
can think of 2 ways : 
1. access msymtab and try dereferencing. If failure, assume its tls.
2. obtain symbol info from msymtab and check for the section value.
AFAIK, elf has section=17 for tls. 

I prefer (2) if we can obtain section info for all exe types.
For now, I've stuck to the section=17 method. 

I agree with Ulrich. Why does LOC_OPTIMIZED_OUT have to be included?

Re-spin of Jan's earlier patch.

Signed-off by : Jan Kratochvil  <jan.kratochvil@redhat.com>
Signed-off by : Vinay Sridhar  <vinay@linux.vnet.ibm.com>

--------Patch------------

--- symtab.c.orig	2008-08-05 13:29:26.000000000 +0530
+++ symtab.c	2008-08-05 13:57:54.000000000 +0530
@@ -1394,31 +1394,51 @@ lookup_global_symbol_from_objfile (const
    depending on whether or not we want to search global symbols or
    static symbols.  */
 
+#define IS_TLS(val) (val==17?1:0)
+
 static struct symbol *
 lookup_symbol_aux_symtabs (int block_index,
 			   const char *name, const char *linkage_name,
 			   const domain_enum domain,
 			   struct symtab **symtab)
 {
-  struct symbol *sym;
+  struct symbol *sym_best = NULL;
   struct objfile *objfile;
   struct blockvector *bv;
   const struct block *block;
   struct symtab *s;
+  struct minimal_symbol *msym;
 
   ALL_PRIMARY_SYMTABS (objfile, s)
   {
+    struct symbol *sym;
+
     bv = BLOCKVECTOR (s);
     block = BLOCKVECTOR_BLOCK (bv, block_index);
     sym = lookup_block_symbol (block, name, linkage_name, domain);
+
+    if (sym != NULL && SYMBOL_CLASS (sym) == LOC_UNRESOLVED)
+      {
+        msym = lookup_minimal_symbol (name, NULL, NULL);
+        if (IS_TLS (msym->ginfo.section) && s->next == NULL)
+          return NULL;
+      }
+
     if (sym)
       {
-	block_found = block;
-	if (symtab != NULL)
-	  *symtab = s;
-	return fixup_symbol_section (sym, objfile);
+	sym_best = sym;
+	if (SYMBOL_CLASS (sym) != LOC_UNRESOLVED)
+	  break;
       }
   }
+  
+  if (sym_best)
+    {
+      block_found = block;
+      if(symtab != NULL)
+        *symtab = s;
+      return fixup_symbol_section (sym_best, objfile);
+    }	
 
   return NULL;
 }



Regards,
Vinay

ïVinay Sridhar,
ïIBM-Linux Technology Centre,
vinay@linux.vnet.ibm.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]