This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Accessing tls variables across files causes a bug
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Ulrich Weigand <uweigand at de dot ibm dot com>
- Cc: Vinay Sridhar <vinay at linux dot vnet dot ibm dot com>, gdb-patches at sources dot redhat dot com, Daniel Jacobowitz <drow at false dot org>, luisgpm at linux dot vnet dot ibm dot com, Gary Funck <gary at intrepid dot com>
- Date: Mon, 1 Dec 2008 22:52:36 +0100
- Subject: Re: [patch] Accessing tls variables across files causes a bug
- References: <20081201141408.GA10447@host0.dyn.jankratochvil.net> <200812011829.mB1ITtMU028584@d12av02.megacenter.de.ibm.com> <1221019993.4923.3.camel@localhost.localdomain> <20081126172332.GE19317@intrepid.com> <20081126204316.GA11366@host0.dyn.jankratochvil.net> <20081201141408.GA10447@host0.dyn.jankratochvil.net> <20081201182931.GA18248@intrepid.com>
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.
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;
+}