* RFC: initial TLS patch
@ 2002-07-02 8:23 Jim Blandy
2002-07-02 10:08 ` Daniel Berlin
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jim Blandy @ 2002-07-02 8:23 UTC (permalink / raw)
To: gdb-patches
This is a work-in-progress, posted for comments. This code has never
been executed, since I only last night got glibc and GCC to actually
compile and run a program that uses `__thread', and GCC doesn't yet
emit the Dwarf 2 extension for thread-local variables.
If you actually want to try it out, remember to run both autoheader
and autoconf after patching configure.in. But I'm mostly just hoping
for eyeball reviews.
Index: gdb/configure.in
===================================================================
RCS file: /cvs/src/src/gdb/configure.in,v
retrieving revision 1.88
diff -c -r1.88 configure.in
*** gdb/configure.in 21 Jun 2002 23:48:39 -0000 1.88
--- gdb/configure.in 29 Jun 2002 03:38:37 -0000
***************
*** 599,604 ****
--- 599,621 ----
AC_SUBST(CONFIG_LDFLAGS)
fi
+ dnl See if we have a thread_db header file that #defines TD_NOTALLOC.
+ if test "x$ac_cv_header_thread_db_h" = "xyes"; then
+ AC_CACHE_CHECK([whether <thread_db.h> defines TD_NOTALLOC],
+ gdb_cv_thread_db_h_defines_td_notalloc,
+ AC_TRY_COMPILE(
+ [#include <thread_db.h>],
+ [int i = TD_NOTALLOC;],
+ gdb_cv_thread_db_h_defines_td_notalloc=yes,
+ gdb_cv_thread_db_h_defines_td_notalloc=no
+ )
+ )
+ fi
+ if test "x$gdb_cv_thread_db_h_defines_td_notalloc" = "xyes"; then
+ AC_DEFINE(THREAD_DB_DEFINES_TD_NOTALLOC, 1,
+ [Define if <thread_db.h> defines the TD_NOTALLOC error code.])
+ fi
+
dnl The CLI cannot be disabled yet, but may be in the future
dnl Handle CLI sub-directory configury.
Index: gdb/dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.60
diff -c -r1.60 dwarf2read.c
*** gdb/dwarf2read.c 22 Jun 2002 00:05:59 -0000 1.60
--- gdb/dwarf2read.c 29 Jun 2002 03:38:40 -0000
***************
*** 389,394 ****
--- 389,400 ----
this function, so we can't say
which register it's relative to;
use LOC_LOCAL. */
+ static int is_thread_local; /* Variable is at a constant offset in the
+ thread-local storage block for the
+ current thread and the dynamic linker
+ module containing this expression.
+ decode_locdesc returns the offset from
+ that base. */
/* DW_AT_frame_base values for the current function.
frame_base_reg is -1 if DW_AT_frame_base is missing, otherwise it
***************
*** 4652,4657 ****
--- 4658,4669 ----
{
SYMBOL_CLASS (sym) = LOC_LOCAL;
}
+ else if (is_thread_local)
+ {
+ SYMBOL_CLASS (sym) = LOC_THREAD_LOCAL_STATIC;
+ SYMBOL_OBJFILE (sym) = objfile;
+ SYMBOL_VALUE_ADDRESS (sym) = addr;
+ }
else
{
fixup_symbol_section (sym, objfile);
***************
*** 6152,6157 ****
--- 6164,6170 ----
offreg = 0;
isderef = 0;
islocal = 0;
+ is_thread_local = 0;
optimized_out = 1;
while (i < size)
***************
*** 6374,6379 ****
--- 6387,6397 ----
if (i < size)
complain (&dwarf2_complex_location_expr);
break;
+
+ case DW_OP_GNU_push_tls_address:
+ is_thread_local = 1;
+ stack[++stacki] = 0;
+ break;
default:
complain (&dwarf2_unsupported_stack_op, dwarf_stack_op_name (op));
Index: gdb/findvar.c
===================================================================
RCS file: /cvs/src/src/gdb/findvar.c,v
retrieving revision 1.34
diff -c -r1.34 findvar.c
*** gdb/findvar.c 15 May 2002 01:01:56 -0000 1.34
--- gdb/findvar.c 29 Jun 2002 03:38:40 -0000
***************
*** 529,535 ****
case LOC_BASEREG:
case LOC_BASEREG_ARG:
- case LOC_THREAD_LOCAL_STATIC:
{
struct value *regval;
--- 529,534 ----
***************
*** 540,545 ****
--- 539,564 ----
addr = value_as_address (regval);
addr += SYMBOL_VALUE (var);
break;
+ }
+
+ case LOC_THREAD_LOCAL_STATIC:
+ {
+ /* We want to let the target / ABI-specific code construct
+ this value for us, so we need to dispose of the value
+ allocated for us above. */
+ release_value (v);
+ value_free (v);
+ if (target_has_get_thread_local_value ())
+ return target_get_thread_local_value (inferior_ptid,
+ SYMBOL_OBJFILE (var),
+ SYMBOL_VALUE (var),
+ SYMBOL_TYPE (var));
+ /* It wouldn't be wrong here to try a gdbarch method, too;
+ finding TLS is an ABI-specific thing. But we don't do that
+ yet. */
+ else
+ error ("Cannot find thread-local variables on this target");
+ break;
}
case LOC_TYPEDEF:
Index: gdb/gdb_thread_db.h
===================================================================
RCS file: /cvs/src/src/gdb/gdb_thread_db.h,v
retrieving revision 1.3
diff -c -r1.3 gdb_thread_db.h
*** gdb/gdb_thread_db.h 6 Mar 2001 08:21:07 -0000 1.3
--- gdb/gdb_thread_db.h 29 Jun 2002 03:38:40 -0000
***************
*** 63,69 ****
TD_NOTSD, /* No thread-specific data available. */
TD_MALLOC, /* Out of memory. */
TD_PARTIALREG, /* Not entire register set was read or written. */
! TD_NOXREGS /* X register set not available for given thread. */
} td_err_e;
--- 63,70 ----
TD_NOTSD, /* No thread-specific data available. */
TD_MALLOC, /* Out of memory. */
TD_PARTIALREG, /* Not entire register set was read or written. */
! TD_NOXREGS, /* X register set not available for given thread. */
! TD_NOTALLOC /* TLS memory not yet allocated. */
} td_err_e;
Index: gdb/hpread.c
===================================================================
RCS file: /cvs/src/src/gdb/hpread.c,v
retrieving revision 1.21
diff -c -r1.21 hpread.c
*** gdb/hpread.c 14 Jun 2002 14:34:25 -0000 1.21
--- gdb/hpread.c 29 Jun 2002 03:38:44 -0000
***************
*** 5743,5749 ****
{
/* Thread-local variable.
*/
! SYMBOL_CLASS (sym) = LOC_THREAD_LOCAL_STATIC;
SYMBOL_BASEREG (sym) = CR27_REGNUM;
if (objfile->flags & OBJF_SHARED)
--- 5743,5749 ----
{
/* Thread-local variable.
*/
! SYMBOL_CLASS (sym) = LOC_BASEREG;
SYMBOL_BASEREG (sym) = CR27_REGNUM;
if (objfile->flags & OBJF_SHARED)
Index: gdb/printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.39
diff -c -r1.39 printcmd.c
*** gdb/printcmd.c 11 May 2002 23:48:23 -0000 1.39
--- gdb/printcmd.c 29 Jun 2002 03:38:45 -0000
***************
*** 1275,1283 ****
break;
case LOC_THREAD_LOCAL_STATIC:
! printf_filtered (
! "a thread-local variable at offset %ld from the thread base register %s",
! val, REGISTER_NAME (basereg));
break;
case LOC_OPTIMIZED_OUT:
--- 1275,1283 ----
break;
case LOC_THREAD_LOCAL_STATIC:
! printf_filtered ("a thread-local variable at offset %ld in the "
! "thread-local storage for `%s'",
! val, SYMBOL_OBJFILE (sym)->name);
break;
case LOC_OPTIMIZED_OUT:
Index: gdb/solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.50
diff -c -r1.50 solib.c
*** gdb/solib.c 12 May 2002 04:20:06 -0000 1.50
--- gdb/solib.c 29 Jun 2002 03:38:46 -0000
***************
*** 843,848 ****
--- 843,870 ----
do_clear_solib (NULL);
}
+
+ /* Search the current shared library list for an entry whose object
+ file is OBJFILE. Return zero if there is no such entry. */
+ struct so_list *
+ get_solib_by_objfile (struct objfile *objfile)
+ {
+ struct so_list *l;
+
+ for (l = so_list_head; l; l = l->next)
+ if (l->objfile == objfile)
+ return l;
+
+ /* Refresh our list from the inferior, and try again. */
+ update_solib_list (0, 0);
+ for (l = so_list_head; l; l = l->next)
+ if (l->objfile == objfile)
+ return l;
+
+ return 0;
+ }
+
+
void
_initialize_solib (void)
{
Index: gdb/solist.h
===================================================================
RCS file: /cvs/src/src/gdb/solist.h,v
retrieving revision 1.7
diff -c -r1.7 solist.h
*** gdb/solist.h 21 Oct 2001 19:20:30 -0000 1.7
--- gdb/solist.h 29 Jun 2002 03:38:46 -0000
***************
*** 106,111 ****
--- 106,114 ----
/* Find solib binary file and open it. */
extern int solib_open (char *in_pathname, char **found_pathname);
+ /* Return the so_list entry whose object file is OBJFILE. */
+ extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
+
/* FIXME: gdbarch needs to control this variable */
extern struct target_so_ops *current_target_so_ops;
Index: gdb/symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.32
diff -c -r1.32 symtab.h
*** gdb/symtab.h 15 May 2002 21:19:21 -0000 1.32
--- gdb/symtab.h 29 Jun 2002 03:38:47 -0000
***************
*** 587,594 ****
LOC_UNRESOLVED,
/* Value is at a thread-specific location calculated by a
! target-specific method. */
!
LOC_THREAD_LOCAL_STATIC,
/* The variable does not actually exist in the program.
--- 587,596 ----
LOC_UNRESOLVED,
/* Value is at a thread-specific location calculated by a
! target-specific method. SYMBOL_OBJFILE gives the object file
! in which the symbol is defined; the symbol's value is the
! offset into that objfile's thread-local storage for the current
! thread. */
LOC_THREAD_LOCAL_STATIC,
/* The variable does not actually exist in the program.
***************
*** 661,670 ****
{
/* Used by LOC_BASEREG and LOC_BASEREG_ARG. */
short basereg;
}
aux_value;
-
/* Link to a list of aliases for this symbol.
Only a "primary/main symbol may have aliases. */
struct alias_list *aliases;
--- 663,677 ----
{
/* Used by LOC_BASEREG and LOC_BASEREG_ARG. */
short basereg;
+
+ /* The objfile in which this symbol is defined. To find a
+ thread-local variable (e.g., a variable declared with the
+ `__thread' storage class), we may need to know which shared
+ library it's in. */
+ struct objfile *objfile;
}
aux_value;
/* Link to a list of aliases for this symbol.
Only a "primary/main symbol may have aliases. */
struct alias_list *aliases;
***************
*** 680,685 ****
--- 687,693 ----
#define SYMBOL_TYPE(symbol) (symbol)->type
#define SYMBOL_LINE(symbol) (symbol)->line
#define SYMBOL_BASEREG(symbol) (symbol)->aux_value.basereg
+ #define SYMBOL_OBJFILE(symbol) (symbol)->aux_value.objfile
#define SYMBOL_ALIASES(symbol) (symbol)->aliases
#define SYMBOL_RANGES(symbol) (symbol)->ranges
\f
Index: gdb/target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.36
diff -c -r1.36 target.c
*** gdb/target.c 15 Jun 2002 21:07:58 -0000 1.36
--- gdb/target.c 29 Jun 2002 03:38:48 -0000
***************
*** 610,615 ****
--- 610,616 ----
INHERIT (to_async_mask_value, t);
INHERIT (to_find_memory_regions, t);
INHERIT (to_make_corefile_notes, t);
+ INHERIT (to_get_thread_local_value, t);
INHERIT (to_magic, t);
#undef INHERIT
Index: gdb/target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.24
diff -c -r1.24 target.h
*** gdb/target.h 18 Apr 2002 18:09:06 -0000 1.24
--- gdb/target.h 29 Jun 2002 03:38:49 -0000
***************
*** 180,186 ****
/* Returns zero to leave the inferior alone, one to interrupt it. */
extern int (*target_activity_function) (void);
\f
! struct thread_info; /* fwd decl for parameter list below: */
struct target_ops
{
--- 180,191 ----
/* Returns zero to leave the inferior alone, one to interrupt it. */
extern int (*target_activity_function) (void);
\f
!
! /* Forward declarations for these structure tags, used in target
! operation parameter lists. */
! struct thread_info;
! struct value;
! struct type;
struct target_ops
{
***************
*** 319,324 ****
--- 324,341 ----
void *),
void *);
char * (*to_make_corefile_notes) (bfd *, int *);
+
+ /* Return the thread-local value of type TYPE at OFFSET in the
+ thread-local storage for the thread PTID and the shared library
+ or executable file given by OBJFILE. If that block of
+ thread-local storage hasn't been allocated yet, this function
+ may return an error, or try to concoct an appropriate
+ (non-lvalue) value based on the initialization image. */
+ struct value *(*to_get_thread_local_value) (ptid_t PTID,
+ struct objfile *OBJFILE,
+ CORE_ADDR OFFSET,
+ struct type *TYPE);
+
int to_magic;
/* Need sub-structure for target machine related rather than comm related?
*/
***************
*** 1021,1026 ****
--- 1038,1050 ----
#define target_make_corefile_notes(BFD, SIZE_P) \
(current_target.to_make_corefile_notes) (BFD, SIZE_P)
+
+
+ /* Thread-local values. */
+ #define target_get_thread_local_value \
+ (current_target.to_get_thread_local_value)
+ #define target_has_get_thread_local_value() \
+ (target_get_thread_local_value != 0)
/* Hook to call target-dependent code after reading in a new symbol table. */
Index: gdb/thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/thread-db.c,v
retrieving revision 1.22
diff -c -r1.22 thread-db.c
*** gdb/thread-db.c 23 Mar 2002 17:38:13 -0000 1.22
--- gdb/thread-db.c 29 Jun 2002 03:38:49 -0000
***************
*** 32,37 ****
--- 32,40 ----
#include "objfiles.h"
#include "target.h"
#include "regcache.h"
+ #include "solib.h"
+ #include "solist.h"
+ #include "value.h"
#ifndef LIBTHREAD_DB_SO
#define LIBTHREAD_DB_SO "libthread_db.so.1"
***************
*** 108,113 ****
--- 111,122 ----
prgregset_t gregs);
static td_err_e (*td_thr_event_enable_p) (const td_thrhandle_t *th, int event);
+ struct link_map;
+ static td_err_e (*td_thr_tls_get_addr_p) (const td_thrhandle_t *th,
+ struct link_map *map,
+ size_t offset,
+ void **address);
+
/* Location of the thread creation event breakpoint. The code at this
location in the child process will be called by the pthread library
whenever a new thread is created. By setting a special breakpoint
***************
*** 348,353 ****
--- 357,363 ----
td_ta_set_event_p = dlsym (handle, "td_ta_set_event");
td_ta_event_getmsg_p = dlsym (handle, "td_ta_event_getmsg");
td_thr_event_enable_p = dlsym (handle, "td_thr_event_enable");
+ td_thr_tls_get_addr_p = dlsym (handle, "td_thr_tls_get_addr");
return 1;
}
***************
*** 1003,1008 ****
--- 1013,1095 ----
return normal_pid_to_str (ptid);
}
+ static struct value *
+ thread_db_get_thread_local_value (ptid_t ptid, struct objfile *objfile,
+ CORE_ADDR offset, struct type *type)
+ {
+ if (is_thread (ptid))
+ {
+ struct so_list *so;
+ int objfile_is_library = (objfile->flags & OBJF_SHARED);
+ td_err_e err;
+ td_thrhandle_t th;
+ void *address;
+
+ if (! td_thr_tls_get_addr_p)
+ error ("Cannot find thread-local variables in this thread library.");
+
+ so = get_solib_by_objfile (objfile);
+ if (! so)
+ error ((objfile_is_library
+ ? ("Cannot find shared library `%s' in dynamic linker's "
+ "module list")
+ : ("Cannot find executable file `%s' in dynamic linker's "
+ "module list")),
+ objfile->name);
+
+ if (! so->lm_info)
+ error ("Missing `struct link_map' value in GDB's shared library list");
+
+ err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
+ if (err != TD_OK)
+ error ("Cannot find thread %ld: %s",
+ (long) GET_THREAD (ptid), thread_db_err_str (err));
+
+ /* That cast there is pretty icky. But thread_db's interface
+ isn't cross-debugging clean, so there's not much we can do
+ about it. */
+ err = td_thr_tls_get_addr_p (&th, (struct link_map *) so->lm_info,
+ offset, &address);
+
+ #ifdef THREAD_DB_DEFINES_TD_NOTALLOC
+ if (err == TD_NOTALLOC)
+ /* Now, if libthread_db provided the initialization image's
+ address, we *could* try to build a non-lvalue value from
+ the initialization image. */
+ error ((objfile_is_library
+ ? ("The inferior has not yet allocated storage for"
+ " thread-local variables in\n"
+ "the shared library `%s'\n"
+ "for the thread %ld")
+ : ("The inferior has not yet allocated storage for"
+ " thread-local variables in\n"
+ "the executable `%s'\n"
+ "for the thread %ld")),
+ objfile->name,
+ (long) GET_THREAD (ptid));
+ #endif
+
+ if (err != TD_OK)
+ error ((objfile_is_library
+ ? ("Cannot find thread-local storage for thread %ld, "
+ "shared library %s:\n%s")
+ : ("Cannot find thread-local storage for thread %ld, "
+ "executable file %s:\n%s")),
+ (long) GET_THREAD (ptid),
+ objfile->name,
+ thread_db_err_str (err));
+
+ /* Another cast assuming host == target. Joy. */
+ return value_at_lazy (type, (CORE_ADDR) address, 0);
+ }
+
+ if (target_beneath->to_get_thread_local_value)
+ return target_beneath->to_get_thread_local_value (ptid, objfile,
+ offset, type);
+
+ error ("Cannot find thread-local values on this target.");
+ }
+
static void
init_thread_db_ops (void)
{
***************
*** 1025,1030 ****
--- 1112,1118 ----
thread_db_ops.to_pid_to_str = thread_db_pid_to_str;
thread_db_ops.to_stratum = thread_stratum;
thread_db_ops.to_has_thread_control = tc_schedlock;
+ thread_db_ops.to_get_thread_local_value = thread_db_get_thread_local_value;
thread_db_ops.to_magic = OPS_MAGIC;
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: initial TLS patch
2002-07-02 8:23 RFC: initial TLS patch Jim Blandy
@ 2002-07-02 10:08 ` Daniel Berlin
2002-07-03 2:39 ` Michael Snyder
2002-07-03 8:28 ` Andrew Cagney
2 siblings, 0 replies; 10+ messages in thread
From: Daniel Berlin @ 2002-07-02 10:08 UTC (permalink / raw)
To: Jim Blandy; +Cc: gdb-patches
On 2 Jul 2002, Jim Blandy wrote:
>
> This is a work-in-progress, posted for comments. This code has never
> been executed, since I only last night got glibc and GCC to actually
> compile and run a program that uses `__thread', and GCC doesn't yet
> emit the Dwarf 2 extension for thread-local variables.
Maybe you could review the patch from april that implements the suggested
location function mechanism, and use that here to handle thread local
variable locations, rather than reuse LOC_THREAD_LOCAL_STATIC (Which,
correct me if i'm wrong, is only currently used by hpread, and pretty
esay to get rid of before your patch)?
After all, aren't we trying to get rid of all the location types eventually?
It would seem to make sense to not add more uses of them if it can be
avoided.
Of course, if you have major problems with that patch, it's understandable
that you'd rather not have this work depend on it, but if it's just minor
issues, it would seem to make sense to me.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: initial TLS patch
2002-07-02 8:23 RFC: initial TLS patch Jim Blandy
2002-07-02 10:08 ` Daniel Berlin
@ 2002-07-03 2:39 ` Michael Snyder
2002-07-03 13:10 ` Jim Blandy
2002-07-03 8:28 ` Andrew Cagney
2 siblings, 1 reply; 10+ messages in thread
From: Michael Snyder @ 2002-07-03 2:39 UTC (permalink / raw)
To: Jim Blandy; +Cc: gdb-patches
Jim Blandy wrote:
>
> This is a work-in-progress, posted for comments. This code has never
> been executed, since I only last night got glibc and GCC to actually
> compile and run a program that uses `__thread', and GCC doesn't yet
> emit the Dwarf 2 extension for thread-local variables.
>
> If you actually want to try it out, remember to run both autoheader
> and autoconf after patching configure.in. But I'm mostly just hoping
> for eyeball reviews.
As a preliminary comment, I think the design is pretty sound.
I haven't finished my "eyeball review", but I hope to look at
it some more tomorrow (before the 4th of july break).
One thing -- I'm a little leary about the target module
actually building a struct value. Seems like that should
be done at a higher level.
Michael
>
> Index: gdb/configure.in
> ===================================================================
> RCS file: /cvs/src/src/gdb/configure.in,v
> retrieving revision 1.88
> diff -c -r1.88 configure.in
> *** gdb/configure.in 21 Jun 2002 23:48:39 -0000 1.88
> --- gdb/configure.in 29 Jun 2002 03:38:37 -0000
> ***************
> *** 599,604 ****
> --- 599,621 ----
> AC_SUBST(CONFIG_LDFLAGS)
> fi
>
> + dnl See if we have a thread_db header file that #defines TD_NOTALLOC.
> + if test "x$ac_cv_header_thread_db_h" = "xyes"; then
> + AC_CACHE_CHECK([whether <thread_db.h> defines TD_NOTALLOC],
> + gdb_cv_thread_db_h_defines_td_notalloc,
> + AC_TRY_COMPILE(
> + [#include <thread_db.h>],
> + [int i = TD_NOTALLOC;],
> + gdb_cv_thread_db_h_defines_td_notalloc=yes,
> + gdb_cv_thread_db_h_defines_td_notalloc=no
> + )
> + )
> + fi
> + if test "x$gdb_cv_thread_db_h_defines_td_notalloc" = "xyes"; then
> + AC_DEFINE(THREAD_DB_DEFINES_TD_NOTALLOC, 1,
> + [Define if <thread_db.h> defines the TD_NOTALLOC error code.])
> + fi
> +
> dnl The CLI cannot be disabled yet, but may be in the future
>
> dnl Handle CLI sub-directory configury.
> Index: gdb/dwarf2read.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2read.c,v
> retrieving revision 1.60
> diff -c -r1.60 dwarf2read.c
> *** gdb/dwarf2read.c 22 Jun 2002 00:05:59 -0000 1.60
> --- gdb/dwarf2read.c 29 Jun 2002 03:38:40 -0000
> ***************
> *** 389,394 ****
> --- 389,400 ----
> this function, so we can't say
> which register it's relative to;
> use LOC_LOCAL. */
> + static int is_thread_local; /* Variable is at a constant offset in the
> + thread-local storage block for the
> + current thread and the dynamic linker
> + module containing this expression.
> + decode_locdesc returns the offset from
> + that base. */
>
> /* DW_AT_frame_base values for the current function.
> frame_base_reg is -1 if DW_AT_frame_base is missing, otherwise it
> ***************
> *** 4652,4657 ****
> --- 4658,4669 ----
> {
> SYMBOL_CLASS (sym) = LOC_LOCAL;
> }
> + else if (is_thread_local)
> + {
> + SYMBOL_CLASS (sym) = LOC_THREAD_LOCAL_STATIC;
> + SYMBOL_OBJFILE (sym) = objfile;
> + SYMBOL_VALUE_ADDRESS (sym) = addr;
> + }
> else
> {
> fixup_symbol_section (sym, objfile);
> ***************
> *** 6152,6157 ****
> --- 6164,6170 ----
> offreg = 0;
> isderef = 0;
> islocal = 0;
> + is_thread_local = 0;
> optimized_out = 1;
>
> while (i < size)
> ***************
> *** 6374,6379 ****
> --- 6387,6397 ----
> if (i < size)
> complain (&dwarf2_complex_location_expr);
> break;
> +
> + case DW_OP_GNU_push_tls_address:
> + is_thread_local = 1;
> + stack[++stacki] = 0;
> + break;
>
> default:
> complain (&dwarf2_unsupported_stack_op, dwarf_stack_op_name (op));
> Index: gdb/findvar.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/findvar.c,v
> retrieving revision 1.34
> diff -c -r1.34 findvar.c
> *** gdb/findvar.c 15 May 2002 01:01:56 -0000 1.34
> --- gdb/findvar.c 29 Jun 2002 03:38:40 -0000
> ***************
> *** 529,535 ****
>
> case LOC_BASEREG:
> case LOC_BASEREG_ARG:
> - case LOC_THREAD_LOCAL_STATIC:
> {
> struct value *regval;
>
> --- 529,534 ----
> ***************
> *** 540,545 ****
> --- 539,564 ----
> addr = value_as_address (regval);
> addr += SYMBOL_VALUE (var);
> break;
> + }
> +
> + case LOC_THREAD_LOCAL_STATIC:
> + {
> + /* We want to let the target / ABI-specific code construct
> + this value for us, so we need to dispose of the value
> + allocated for us above. */
> + release_value (v);
> + value_free (v);
> + if (target_has_get_thread_local_value ())
> + return target_get_thread_local_value (inferior_ptid,
> + SYMBOL_OBJFILE (var),
> + SYMBOL_VALUE (var),
> + SYMBOL_TYPE (var));
> + /* It wouldn't be wrong here to try a gdbarch method, too;
> + finding TLS is an ABI-specific thing. But we don't do that
> + yet. */
> + else
> + error ("Cannot find thread-local variables on this target");
> + break;
> }
>
> case LOC_TYPEDEF:
> Index: gdb/gdb_thread_db.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdb_thread_db.h,v
> retrieving revision 1.3
> diff -c -r1.3 gdb_thread_db.h
> *** gdb/gdb_thread_db.h 6 Mar 2001 08:21:07 -0000 1.3
> --- gdb/gdb_thread_db.h 29 Jun 2002 03:38:40 -0000
> ***************
> *** 63,69 ****
> TD_NOTSD, /* No thread-specific data available. */
> TD_MALLOC, /* Out of memory. */
> TD_PARTIALREG, /* Not entire register set was read or written. */
> ! TD_NOXREGS /* X register set not available for given thread. */
> } td_err_e;
>
>
> --- 63,70 ----
> TD_NOTSD, /* No thread-specific data available. */
> TD_MALLOC, /* Out of memory. */
> TD_PARTIALREG, /* Not entire register set was read or written. */
> ! TD_NOXREGS, /* X register set not available for given thread. */
> ! TD_NOTALLOC /* TLS memory not yet allocated. */
> } td_err_e;
>
>
> Index: gdb/hpread.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/hpread.c,v
> retrieving revision 1.21
> diff -c -r1.21 hpread.c
> *** gdb/hpread.c 14 Jun 2002 14:34:25 -0000 1.21
> --- gdb/hpread.c 29 Jun 2002 03:38:44 -0000
> ***************
> *** 5743,5749 ****
> {
> /* Thread-local variable.
> */
> ! SYMBOL_CLASS (sym) = LOC_THREAD_LOCAL_STATIC;
> SYMBOL_BASEREG (sym) = CR27_REGNUM;
>
> if (objfile->flags & OBJF_SHARED)
> --- 5743,5749 ----
> {
> /* Thread-local variable.
> */
> ! SYMBOL_CLASS (sym) = LOC_BASEREG;
> SYMBOL_BASEREG (sym) = CR27_REGNUM;
>
> if (objfile->flags & OBJF_SHARED)
> Index: gdb/printcmd.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/printcmd.c,v
> retrieving revision 1.39
> diff -c -r1.39 printcmd.c
> *** gdb/printcmd.c 11 May 2002 23:48:23 -0000 1.39
> --- gdb/printcmd.c 29 Jun 2002 03:38:45 -0000
> ***************
> *** 1275,1283 ****
> break;
>
> case LOC_THREAD_LOCAL_STATIC:
> ! printf_filtered (
> ! "a thread-local variable at offset %ld from the thread base register %s",
> ! val, REGISTER_NAME (basereg));
> break;
>
> case LOC_OPTIMIZED_OUT:
> --- 1275,1283 ----
> break;
>
> case LOC_THREAD_LOCAL_STATIC:
> ! printf_filtered ("a thread-local variable at offset %ld in the "
> ! "thread-local storage for `%s'",
> ! val, SYMBOL_OBJFILE (sym)->name);
> break;
>
> case LOC_OPTIMIZED_OUT:
> Index: gdb/solib.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib.c,v
> retrieving revision 1.50
> diff -c -r1.50 solib.c
> *** gdb/solib.c 12 May 2002 04:20:06 -0000 1.50
> --- gdb/solib.c 29 Jun 2002 03:38:46 -0000
> ***************
> *** 843,848 ****
> --- 843,870 ----
> do_clear_solib (NULL);
> }
>
> +
> + /* Search the current shared library list for an entry whose object
> + file is OBJFILE. Return zero if there is no such entry. */
> + struct so_list *
> + get_solib_by_objfile (struct objfile *objfile)
> + {
> + struct so_list *l;
> +
> + for (l = so_list_head; l; l = l->next)
> + if (l->objfile == objfile)
> + return l;
> +
> + /* Refresh our list from the inferior, and try again. */
> + update_solib_list (0, 0);
> + for (l = so_list_head; l; l = l->next)
> + if (l->objfile == objfile)
> + return l;
> +
> + return 0;
> + }
> +
> +
> void
> _initialize_solib (void)
> {
> Index: gdb/solist.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/solist.h,v
> retrieving revision 1.7
> diff -c -r1.7 solist.h
> *** gdb/solist.h 21 Oct 2001 19:20:30 -0000 1.7
> --- gdb/solist.h 29 Jun 2002 03:38:46 -0000
> ***************
> *** 106,111 ****
> --- 106,114 ----
> /* Find solib binary file and open it. */
> extern int solib_open (char *in_pathname, char **found_pathname);
>
> + /* Return the so_list entry whose object file is OBJFILE. */
> + extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
> +
> /* FIXME: gdbarch needs to control this variable */
> extern struct target_so_ops *current_target_so_ops;
>
> Index: gdb/symtab.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/symtab.h,v
> retrieving revision 1.32
> diff -c -r1.32 symtab.h
> *** gdb/symtab.h 15 May 2002 21:19:21 -0000 1.32
> --- gdb/symtab.h 29 Jun 2002 03:38:47 -0000
> ***************
> *** 587,594 ****
> LOC_UNRESOLVED,
>
> /* Value is at a thread-specific location calculated by a
> ! target-specific method. */
> !
> LOC_THREAD_LOCAL_STATIC,
>
> /* The variable does not actually exist in the program.
> --- 587,596 ----
> LOC_UNRESOLVED,
>
> /* Value is at a thread-specific location calculated by a
> ! target-specific method. SYMBOL_OBJFILE gives the object file
> ! in which the symbol is defined; the symbol's value is the
> ! offset into that objfile's thread-local storage for the current
> ! thread. */
> LOC_THREAD_LOCAL_STATIC,
>
> /* The variable does not actually exist in the program.
> ***************
> *** 661,670 ****
> {
> /* Used by LOC_BASEREG and LOC_BASEREG_ARG. */
> short basereg;
> }
> aux_value;
>
> -
> /* Link to a list of aliases for this symbol.
> Only a "primary/main symbol may have aliases. */
> struct alias_list *aliases;
> --- 663,677 ----
> {
> /* Used by LOC_BASEREG and LOC_BASEREG_ARG. */
> short basereg;
> +
> + /* The objfile in which this symbol is defined. To find a
> + thread-local variable (e.g., a variable declared with the
> + `__thread' storage class), we may need to know which shared
> + library it's in. */
> + struct objfile *objfile;
> }
> aux_value;
>
> /* Link to a list of aliases for this symbol.
> Only a "primary/main symbol may have aliases. */
> struct alias_list *aliases;
> ***************
> *** 680,685 ****
> --- 687,693 ----
> #define SYMBOL_TYPE(symbol) (symbol)->type
> #define SYMBOL_LINE(symbol) (symbol)->line
> #define SYMBOL_BASEREG(symbol) (symbol)->aux_value.basereg
> + #define SYMBOL_OBJFILE(symbol) (symbol)->aux_value.objfile
> #define SYMBOL_ALIASES(symbol) (symbol)->aliases
> #define SYMBOL_RANGES(symbol) (symbol)->ranges
>
> Index: gdb/target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.36
> diff -c -r1.36 target.c
> *** gdb/target.c 15 Jun 2002 21:07:58 -0000 1.36
> --- gdb/target.c 29 Jun 2002 03:38:48 -0000
> ***************
> *** 610,615 ****
> --- 610,616 ----
> INHERIT (to_async_mask_value, t);
> INHERIT (to_find_memory_regions, t);
> INHERIT (to_make_corefile_notes, t);
> + INHERIT (to_get_thread_local_value, t);
> INHERIT (to_magic, t);
>
> #undef INHERIT
> Index: gdb/target.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.h,v
> retrieving revision 1.24
> diff -c -r1.24 target.h
> *** gdb/target.h 18 Apr 2002 18:09:06 -0000 1.24
> --- gdb/target.h 29 Jun 2002 03:38:49 -0000
> ***************
> *** 180,186 ****
> /* Returns zero to leave the inferior alone, one to interrupt it. */
> extern int (*target_activity_function) (void);
>
> ! struct thread_info; /* fwd decl for parameter list below: */
>
> struct target_ops
> {
> --- 180,191 ----
> /* Returns zero to leave the inferior alone, one to interrupt it. */
> extern int (*target_activity_function) (void);
>
> !
> ! /* Forward declarations for these structure tags, used in target
> ! operation parameter lists. */
> ! struct thread_info;
> ! struct value;
> ! struct type;
>
> struct target_ops
> {
> ***************
> *** 319,324 ****
> --- 324,341 ----
> void *),
> void *);
> char * (*to_make_corefile_notes) (bfd *, int *);
> +
> + /* Return the thread-local value of type TYPE at OFFSET in the
> + thread-local storage for the thread PTID and the shared library
> + or executable file given by OBJFILE. If that block of
> + thread-local storage hasn't been allocated yet, this function
> + may return an error, or try to concoct an appropriate
> + (non-lvalue) value based on the initialization image. */
> + struct value *(*to_get_thread_local_value) (ptid_t PTID,
> + struct objfile *OBJFILE,
> + CORE_ADDR OFFSET,
> + struct type *TYPE);
> +
> int to_magic;
> /* Need sub-structure for target machine related rather than comm related?
> */
> ***************
> *** 1021,1026 ****
> --- 1038,1050 ----
>
> #define target_make_corefile_notes(BFD, SIZE_P) \
> (current_target.to_make_corefile_notes) (BFD, SIZE_P)
> +
> +
> + /* Thread-local values. */
> + #define target_get_thread_local_value \
> + (current_target.to_get_thread_local_value)
> + #define target_has_get_thread_local_value() \
> + (target_get_thread_local_value != 0)
>
> /* Hook to call target-dependent code after reading in a new symbol table. */
>
> Index: gdb/thread-db.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/thread-db.c,v
> retrieving revision 1.22
> diff -c -r1.22 thread-db.c
> *** gdb/thread-db.c 23 Mar 2002 17:38:13 -0000 1.22
> --- gdb/thread-db.c 29 Jun 2002 03:38:49 -0000
> ***************
> *** 32,37 ****
> --- 32,40 ----
> #include "objfiles.h"
> #include "target.h"
> #include "regcache.h"
> + #include "solib.h"
> + #include "solist.h"
> + #include "value.h"
>
> #ifndef LIBTHREAD_DB_SO
> #define LIBTHREAD_DB_SO "libthread_db.so.1"
> ***************
> *** 108,113 ****
> --- 111,122 ----
> prgregset_t gregs);
> static td_err_e (*td_thr_event_enable_p) (const td_thrhandle_t *th, int event);
>
> + struct link_map;
> + static td_err_e (*td_thr_tls_get_addr_p) (const td_thrhandle_t *th,
> + struct link_map *map,
> + size_t offset,
> + void **address);
> +
> /* Location of the thread creation event breakpoint. The code at this
> location in the child process will be called by the pthread library
> whenever a new thread is created. By setting a special breakpoint
> ***************
> *** 348,353 ****
> --- 357,363 ----
> td_ta_set_event_p = dlsym (handle, "td_ta_set_event");
> td_ta_event_getmsg_p = dlsym (handle, "td_ta_event_getmsg");
> td_thr_event_enable_p = dlsym (handle, "td_thr_event_enable");
> + td_thr_tls_get_addr_p = dlsym (handle, "td_thr_tls_get_addr");
>
> return 1;
> }
> ***************
> *** 1003,1008 ****
> --- 1013,1095 ----
> return normal_pid_to_str (ptid);
> }
>
> + static struct value *
> + thread_db_get_thread_local_value (ptid_t ptid, struct objfile *objfile,
> + CORE_ADDR offset, struct type *type)
> + {
> + if (is_thread (ptid))
> + {
> + struct so_list *so;
> + int objfile_is_library = (objfile->flags & OBJF_SHARED);
> + td_err_e err;
> + td_thrhandle_t th;
> + void *address;
> +
> + if (! td_thr_tls_get_addr_p)
> + error ("Cannot find thread-local variables in this thread library.");
> +
> + so = get_solib_by_objfile (objfile);
> + if (! so)
> + error ((objfile_is_library
> + ? ("Cannot find shared library `%s' in dynamic linker's "
> + "module list")
> + : ("Cannot find executable file `%s' in dynamic linker's "
> + "module list")),
> + objfile->name);
> +
> + if (! so->lm_info)
> + error ("Missing `struct link_map' value in GDB's shared library list");
> +
> + err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (ptid), &th);
> + if (err != TD_OK)
> + error ("Cannot find thread %ld: %s",
> + (long) GET_THREAD (ptid), thread_db_err_str (err));
> +
> + /* That cast there is pretty icky. But thread_db's interface
> + isn't cross-debugging clean, so there's not much we can do
> + about it. */
> + err = td_thr_tls_get_addr_p (&th, (struct link_map *) so->lm_info,
> + offset, &address);
> +
> + #ifdef THREAD_DB_DEFINES_TD_NOTALLOC
> + if (err == TD_NOTALLOC)
> + /* Now, if libthread_db provided the initialization image's
> + address, we *could* try to build a non-lvalue value from
> + the initialization image. */
> + error ((objfile_is_library
> + ? ("The inferior has not yet allocated storage for"
> + " thread-local variables in\n"
> + "the shared library `%s'\n"
> + "for the thread %ld")
> + : ("The inferior has not yet allocated storage for"
> + " thread-local variables in\n"
> + "the executable `%s'\n"
> + "for the thread %ld")),
> + objfile->name,
> + (long) GET_THREAD (ptid));
> + #endif
> +
> + if (err != TD_OK)
> + error ((objfile_is_library
> + ? ("Cannot find thread-local storage for thread %ld, "
> + "shared library %s:\n%s")
> + : ("Cannot find thread-local storage for thread %ld, "
> + "executable file %s:\n%s")),
> + (long) GET_THREAD (ptid),
> + objfile->name,
> + thread_db_err_str (err));
> +
> + /* Another cast assuming host == target. Joy. */
> + return value_at_lazy (type, (CORE_ADDR) address, 0);
> + }
> +
> + if (target_beneath->to_get_thread_local_value)
> + return target_beneath->to_get_thread_local_value (ptid, objfile,
> + offset, type);
> +
> + error ("Cannot find thread-local values on this target.");
> + }
> +
> static void
> init_thread_db_ops (void)
> {
> ***************
> *** 1025,1030 ****
> --- 1112,1118 ----
> thread_db_ops.to_pid_to_str = thread_db_pid_to_str;
> thread_db_ops.to_stratum = thread_stratum;
> thread_db_ops.to_has_thread_control = tc_schedlock;
> + thread_db_ops.to_get_thread_local_value = thread_db_get_thread_local_value;
> thread_db_ops.to_magic = OPS_MAGIC;
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: initial TLS patch
2002-07-02 8:23 RFC: initial TLS patch Jim Blandy
2002-07-02 10:08 ` Daniel Berlin
2002-07-03 2:39 ` Michael Snyder
@ 2002-07-03 8:28 ` Andrew Cagney
2002-07-03 11:30 ` Jim Blandy
2 siblings, 1 reply; 10+ messages in thread
From: Andrew Cagney @ 2002-07-03 8:28 UTC (permalink / raw)
To: Jim Blandy; +Cc: gdb-patches
> + else if (is_thread_local)
> + {
> + SYMBOL_CLASS (sym) = LOC_THREAD_LOCAL_STATIC;
> + SYMBOL_OBJFILE (sym) = objfile;
> + SYMBOL_VALUE_ADDRESS (sym) = addr;
> + }
Can I suggest awarding this mechanism a new LOC name. That way the past
history of LOC_THREAD_LOCAL_STATIC won't flow into this new mechanism.
As for thread local static, looks like a separate pass can purge GDB of it.
> ! TD_NOTALLOC /* TLS memory not yet allocated. */
Suggest TLS -> Thread [Local] memory
> + extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
Tipo OBJFILE -> objfile.
> Index: gdb/target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.36
> diff -c -r1.36 target.c
> *** gdb/target.c 15 Jun 2002 21:07:58 -0000 1.36
> --- gdb/target.c 29 Jun 2002 03:38:48 -0000
> ***************
> *** 610,615 ****
> --- 610,616 ----
> INHERIT (to_async_mask_value, t);
> INHERIT (to_find_memory_regions, t);
> INHERIT (to_make_corefile_notes, t);
> + INHERIT (to_get_thread_local_value, t);
> INHERIT (to_magic, t);
Does this also need some debug stuff added?
> #undef INHERIT
> Index: gdb/target.h
> +
> + /* Return the thread-local value of type TYPE at OFFSET in the
> + thread-local storage for the thread PTID and the shared library
> + or executable file given by OBJFILE. If that block of
> + thread-local storage hasn't been allocated yet, this function
> + may return an error, or try to concoct an appropriate
> + (non-lvalue) value based on the initialization image. */
> + struct value *(*to_get_thread_local_value) (ptid_t PTID,
> + struct objfile *OBJFILE,
> + CORE_ADDR OFFSET,
Per other [discussion] thread. I understand that at this time all that
this needs to compute / return are: thread storage base address;
conversion status - not allocated, readonly, read-write. Having it
return something more complicated like a ``struct value'' can be left to
the person that actually needs the mechanism - I figure they will be in
a better position to determine exactly what mechanism is needed.
Perhaphs there should be a separate ``struct location'' object?
> ***************
> *** 1021,1026 ****
> --- 1038,1050 ----
>
> #define target_make_corefile_notes(BFD, SIZE_P) \
> (current_target.to_make_corefile_notes) (BFD, SIZE_P)
> +
> +
> + /* Thread-local values. */
> + #define target_get_thread_local_value \
> + (current_target.to_get_thread_local_value)
> + #define target_has_get_thread_local_value() \
> + (target_get_thread_local_value != 0)
``_has_'' in the target vector appears to be used when testing
attributes such as ``the target has memory'' so
target_get_thread_local_value_p() would be better. Suggest ``!= NULL''.
> + if (! so)
> + error ((objfile_is_library
> + ? ("Cannot find shared library `%s' in dynamic linker's "
> + "module list")
> + : ("Cannot find executable file `%s' in dynamic linker's "
> + "module list")),
> + objfile->name);
Can I suggest writing these as:
if (objfile_is_library)
error (...)
else
error (...)
it is going to make checking and for correct i18n much easier. All
errors will match:
error[[:space:]]\(_\"[A-Z]
> + #ifdef THREAD_DB_DEFINES_TD_NOTALLOC
I suspect you mean THREAD_DB_HAS_TD_NOTALLOC, if it were a #define,
#ifdef TD_NOTALLOC would work :-)
enjoy,
Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: initial TLS patch
2002-07-03 8:28 ` Andrew Cagney
@ 2002-07-03 11:30 ` Jim Blandy
2002-07-08 19:19 ` Andrew Cagney
2002-07-08 20:06 ` Andrew Cagney
0 siblings, 2 replies; 10+ messages in thread
From: Jim Blandy @ 2002-07-03 11:30 UTC (permalink / raw)
To: Andrew Cagney; +Cc: gdb-patches
This was a very helpful review. I'll revise the patch and re-post.
Specific comments below:
Andrew Cagney <ac131313@ges.redhat.com> writes:
> > + else if (is_thread_local)
> > + {
> > + SYMBOL_CLASS (sym) = LOC_THREAD_LOCAL_STATIC;
> > + SYMBOL_OBJFILE (sym) = objfile;
> > + SYMBOL_VALUE_ADDRESS (sym) = addr;
> > + }
>
> Can I suggest awarding this mechanism a new LOC name. That way the
> past history of LOC_THREAD_LOCAL_STATIC won't flow into this new
> mechanism. As for thread local static, looks like a separate pass can
> purge GDB of it.
I don't mind doing that. But check out the comment in symtab.h:
/* Value is at a thread-specific location calculated by a
target-specific method. */
LOC_THREAD_LOCAL_STATIC,
That's the pre-existing text. It describes *exactly* what we're doing
here. The only existing use of this address class, HP's, is actually
an abuse of this, because it implements this "target-specific method"
in non-target-specific code. I would argue that I'm actually
correcting a broken situation: I'm making LOC_THREAD_LOCAL_STATIC mean
what it says it means.
I don't think the past history of LOC_THREAD_LOCAL_STATIC is a big
deal; If you grep for it through the current sources, it's pretty
obvious that the patch doesn't break anything.
> > ! TD_NOTALLOC /* TLS memory not yet allocated. */
>
> Suggest TLS -> Thread [Local] memory
Well, `thread-local storage' is the term used in all the ABI's and
compiler docs. I think we should use the terms people are going to
see elsewhere.
> > + extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
>
> Tipo OBJFILE -> objfile.
It was capitalized to match the comment above:
+ /* Return the so_list entry whose object file is OBJFILE. */
+ extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
+
But the rest of GDB doesn't do that, so I'll change this.
> > Index: gdb/target.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/target.c,v
> > retrieving revision 1.36
> > diff -c -r1.36 target.c
> > *** gdb/target.c 15 Jun 2002 21:07:58 -0000 1.36
> > --- gdb/target.c 29 Jun 2002 03:38:48 -0000
> > ***************
> > *** 610,615 ****
> > --- 610,616 ----
> > INHERIT (to_async_mask_value, t);
> > INHERIT (to_find_memory_regions, t);
> > INHERIT (to_make_corefile_notes, t);
> > + INHERIT (to_get_thread_local_value, t);
> > INHERIT (to_magic, t);
>
> Does this also need some debug stuff added?
Yes, thanks.
> > #undef INHERIT
> > Index: gdb/target.h
>
> > + + /* Return the thread-local value of type TYPE at OFFSET in
> > the
> > + thread-local storage for the thread PTID and the shared library
> > + or executable file given by OBJFILE. If that block of
> > + thread-local storage hasn't been allocated yet, this function
> > + may return an error, or try to concoct an appropriate
> > + (non-lvalue) value based on the initialization image. */
> > + struct value *(*to_get_thread_local_value) (ptid_t PTID,
> > + struct objfile *OBJFILE,
> > + CORE_ADDR OFFSET,
I assume you want these in lower-case as well, right?
> Per other [discussion] thread. I understand that at this time all
> that this needs to compute / return are: thread storage base address;
> conversion status - not allocated, readonly, read-write. Having it
> return something more complicated like a ``struct value'' can be left
> to the person that actually needs the mechanism - I figure they will
> be in a better position to determine exactly what mechanism is needed.
>
> Perhaphs there should be a separate ``struct location'' object?
Well, at the moment, the only extant implementation of this will
either return an address of the true value, or raise an error. So
it's even simpler than that. The prototype could simply be:
CORE_ADDR (*to_get_thread_local_value) (ptid_t ptid,
struct objfile *objfile,
CORE_ADDR offset);
I agree that a `struct value' is more high-level than we'd really like
for a target operation. Target methods should be messing with
CORE_ADDRs and sizes and machine-level stuff like that.
But the motivation here is to keep the interface innocent of the
different ways target-specific code might want to approximate the
"right" answer. For example, since the thread-local storage
implementation at hand may not have the storage allocated yet, but
will have a read-only copy of the variable at the wrong address, an
interface like this would be fine:
/* Return the address of PTID's thread-local storage for the load
module (shared lib or main program) given by OBJFILE, at
OFFSET, and set *ALLOCATED to 1. If the thread-local storage
has not been allocated, set *ALLOCATED to zero, and return the
corresponding address in the initialization image, which
contains the bits the variable *would* have if it *were*
allocated. Raise an error if we can't find the thread-local
storage or the initialization image. */
CORE_ADDR (*to_get_thread_local_value) (ptid_t ptid,
struct objfile *objfile,
CORE_ADDR offset,
int *allocated);
This is fine, and it covers everything that the IA-32 Linux TLS
implementation will do. No problems here.
But, to me, it seems like this interface explicitly reflects the
quirks of the TLS implementation. What if some other TLS
implementation requires, say, relocs to be applied to the
initialization image? What if some processor with lots of registers
puts small TLS variables in registers? (You could have register-sized
relocs, and let the static linker assign the register number.
Dynamically linked code couldn't do this, but that's okay.)
Maybe that's contrived. But given how hairy TLS seems to be, I expect
to see some variety in the implementations. And each time we
encounter another variant, then this interface will need to again be
expanded to accomodate that. This target method will end up showing
every possible way anyone has ever constructed a thread-local value.
The motivation for returning a `struct value' is that we know the
interface will be adequate for any future TLS system, no matter what
tricks it wants to play. We know the end result has to be a `struct
value' anyway in GDB, so that interface places no more constraints on
GDB's TLS support than the rest of GDB does.
So, although I do share folks' intuition that `struct value' is too
high-level for the target interface, it seems like a really nice
solution to the problem.
When I spoke with Michael S. on the phone, he expressed concern that
we were spreading knowledge about the internals of `struct value' into
areas of GDB that shouldn't know about it. But I don't think that's
quite so. Check out the code in thread_db_get_thread_local_value that
constructs the value. It's trivial:
return value_at_lazy (type, (CORE_ADDR) address, 0);
It constructs the value through clean, simple public interfaces. And
we can add more interfaces to value.h if we need them.
In this approach, it is certainly the case that the target method
becomes aware of the different ways one might construct `struct value'
objects: lazy objects, non-lvalue, etc. But the alternative is for
the get_thread_local_value method's interface to reflect all the
different states TLS might be in. You've got complexity exported
across an interface either way --- but if let the TLS code return a
`struct value' it's not new complexity.
> > ***************
> > *** 1021,1026 ****
> > --- 1038,1050 ----
> > #define target_make_corefile_notes(BFD, SIZE_P) \
> > (current_target.to_make_corefile_notes) (BFD, SIZE_P)
> > + + + /* Thread-local values. */
> > + #define target_get_thread_local_value \
> > + (current_target.to_get_thread_local_value)
> > + #define target_has_get_thread_local_value() \
> > + (target_get_thread_local_value != 0)
>
> ``_has_'' in the target vector appears to be used when testing
> attributes such as ``the target has memory'' so
> target_get_thread_local_value_p() would be better. Suggest ``!=
> NULL''.
Okay, I wasn't sure what was right here --- I'll fix this.
By the way --- won't the presence of the debug method break this test?
That is, if one enables debugging, then
`target_has_get_thread_local_value' will return true, even when it
doesn't. Should there be a separate `int' member of the target saying
whether it supports `get_thread_local_value'?
> > + if (! so)
> > + error ((objfile_is_library
> > + ? ("Cannot find shared library `%s' in dynamic linker's "
> > + "module list")
> > + : ("Cannot find executable file `%s' in dynamic linker's "
> > + "module list")),
> > + objfile->name);
>
> Can I suggest writing these as:
> if (objfile_is_library)
> error (...)
> else
> error (...)
> it is going to make checking and for correct i18n much easier. All
> errors will match:
> error[[:space:]]\(_\"[A-Z]
Sure thing. I was trying to make this i18n-friendly by not
substituting words into the middle of the sentence, but I'll do
whatever the i18n person wants.
> > + #ifdef THREAD_DB_DEFINES_TD_NOTALLOC
>
> I suspect you mean THREAD_DB_HAS_TD_NOTALLOC, if it were a #define,
> #ifdef TD_NOTALLOC would work :-)
Yes, thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: initial TLS patch
2002-07-03 2:39 ` Michael Snyder
@ 2002-07-03 13:10 ` Jim Blandy
0 siblings, 0 replies; 10+ messages in thread
From: Jim Blandy @ 2002-07-03 13:10 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches
Michael Snyder <msnyder@redhat.com> writes:
> As a preliminary comment, I think the design is pretty sound.
> I haven't finished my "eyeball review", but I hope to look at
> it some more tomorrow (before the 4th of july break).
Okay, great.
> One thing -- I'm a little leary about the target module
> actually building a struct value. Seems like that should
> be done at a higher level.
Yes, I agree that's somewhat questionable. I'm going to take that out
for this pass; when thread_db provides the support we need to do
fancier things, we can revisit the question.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: initial TLS patch
2002-07-03 11:30 ` Jim Blandy
@ 2002-07-08 19:19 ` Andrew Cagney
2002-07-08 20:48 ` Jim Blandy
2002-07-08 20:06 ` Andrew Cagney
1 sibling, 1 reply; 10+ messages in thread
From: Andrew Cagney @ 2002-07-08 19:19 UTC (permalink / raw)
To: Jim Blandy; +Cc: gdb-patches
> But, to me, it seems like this interface explicitly reflects the
> quirks of the TLS implementation. What if some other TLS
> implementation requires, say, relocs to be applied to the
> initialization image? What if some processor with lots of registers
> puts small TLS variables in registers? (You could have register-sized
> relocs, and let the static linker assign the register number.
> Dynamically linked code couldn't do this, but that's okay.)
> Maybe that's contrived. But given how hairy TLS seems to be, I expect
> to see some variety in the implementations. And each time we
> encounter another variant, then this interface will need to again be
> expanded to accomodate that. This target method will end up showing
> every possible way anyone has ever constructed a thread-local value.
For all we know, that thread implementation could be so incompatible
with what you're adding that they need to add yet another LOC. As I
noted before:
> Having it return something more complicated like a ``struct value''
can be left to the person that actually needs the mechanism - I figure
they will be in a better position to determine exactly what mechanism is
needed.
I think this is very important. To apply the old engineering motto -
K.I.S.S.
I also noted that:
> Perhaphs there should be a separate ``struct location'' object?
I'll post this to gdb@.
Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: initial TLS patch
2002-07-03 11:30 ` Jim Blandy
2002-07-08 19:19 ` Andrew Cagney
@ 2002-07-08 20:06 ` Andrew Cagney
2002-07-08 20:15 ` Jim Blandy
1 sibling, 1 reply; 10+ messages in thread
From: Andrew Cagney @ 2002-07-08 20:06 UTC (permalink / raw)
To: Jim Blandy; +Cc: gdb-patches
> + extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
>
>>
>> Tipo OBJFILE -> objfile.
>
>
> It was capitalized to match the comment above:
>
> + /* Return the so_list entry whose object file is OBJFILE. */
> + extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
> +
>
> But the rest of GDB doesn't do that, so I'll change this.
Just FYI, from the coding doco:
> The comment on a function is much clearer if you use the argument names to speak about the argument values. The variable name itself should be lower case, but write it in upper case when you are speaking about the value rather than the variable itself. Thus, "the inode number NODE_NUM" rather than "an inode".
enjoy,
Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: initial TLS patch
2002-07-08 20:06 ` Andrew Cagney
@ 2002-07-08 20:15 ` Jim Blandy
0 siblings, 0 replies; 10+ messages in thread
From: Jim Blandy @ 2002-07-08 20:15 UTC (permalink / raw)
To: Andrew Cagney; +Cc: gdb-patches
Andrew Cagney <ac131313@ges.redhat.com> writes:
> > + extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
> >
> >> Tipo OBJFILE -> objfile.
> > It was capitalized to match the comment above:
> > + /* Return the so_list entry whose object file is OBJFILE. */
> > + extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
> > + But the rest of GDB doesn't do that, so I'll change this.
>
> Just FYI, from the coding doco:
>
> > The comment on a function is much clearer if you use the argument
> > names to speak about the argument values. The variable name itself
> > should be lower case, but write it in upper case when you are
> > speaking about the value rather than the variable itself. Thus, "the
> > inode number NODE_NUM" rather than "an inode".
Chapter and verse, indeed. Already fixed.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFC: initial TLS patch
2002-07-08 19:19 ` Andrew Cagney
@ 2002-07-08 20:48 ` Jim Blandy
0 siblings, 0 replies; 10+ messages in thread
From: Jim Blandy @ 2002-07-08 20:48 UTC (permalink / raw)
To: Andrew Cagney; +Cc: gdb-patches
Andrew Cagney <ac131313@ges.redhat.com> writes:
> > But, to me, it seems like this interface explicitly reflects the
> > quirks of the TLS implementation. What if some other TLS
> > implementation requires, say, relocs to be applied to the
> > initialization image? What if some processor with lots of registers
> > puts small TLS variables in registers? (You could have register-sized
> > relocs, and let the static linker assign the register number.
> > Dynamically linked code couldn't do this, but that's okay.)
>
>
> > Maybe that's contrived. But given how hairy TLS seems to be, I expect
> > to see some variety in the implementations. And each time we
> > encounter another variant, then this interface will need to again be
> > expanded to accomodate that. This target method will end up showing
> > every possible way anyone has ever constructed a thread-local value.
>
> For all we know, that thread implementation could be so incompatible
> with what you're adding that they need to add yet another LOC. As I
> noted before:
>
> > Having it return something more complicated like a ``struct value''
> can be left to the person that actually needs the mechanism - I figure
> they will be in a better position to determine exactly what mechanism
> is needed.
>
> I think this is very important. To apply the old engineering motto -
> K.I.S.S.
Yes, of course. And I think returning a `struct value' is the simpler
thing to do. :)
Right now, here's the interface which would accomodate the
possibilities we know of today using the simplest datatypes:
CORE_ADDR (*to_get_thread_local) (ptid_t thread,
struct objfile *load_module,
CORE_ADDR offset,
int *init_image);
Explaining what `init_image' means, and how to use CORE_ADDR in each
case, and why it must be so, basically requires explaining the whole
lazy thread-local storage allocation model. I can do that, but it
seems more complex to me than explaining:
struct value *(*to_get_thread_local) (ptid_t thread,
struct objfile *load_module,
CORE_ADDR offset,
struct type *type);
This is easy: "Return the object of type TYPE at offset OFFSET in
THREAD's thread-local storage for LOAD_MODULE." One has to point out
that thread-local storage blocks are both per-thread and per-load
module --- the cartesian product --- but then you're done.
At this point, I feel like I've explained my position about as well as
I can, and you're not persuaded; since Michael S. hasn't said
anything, I assume he isn't either. Since I don't think it'll be so
bad to do it the way you suggest, I'll go with that, and we'll see how
it goes.
> I also noted that:
>
> > Perhaphs there should be a separate ``struct location'' object?
>
> I'll post this to gdb@.
See, this makes no sense to me. If a `struct value' is too high-level
a thing for a target method to return, why is `struct location' any
different? The two seem part of the same structural layer to me.
*shrug*
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2002-07-09 3:20 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2002-07-02 8:23 RFC: initial TLS patch Jim Blandy
2002-07-02 10:08 ` Daniel Berlin
2002-07-03 2:39 ` Michael Snyder
2002-07-03 13:10 ` Jim Blandy
2002-07-03 8:28 ` Andrew Cagney
2002-07-03 11:30 ` Jim Blandy
2002-07-08 19:19 ` Andrew Cagney
2002-07-08 20:48 ` Jim Blandy
2002-07-08 20:06 ` Andrew Cagney
2002-07-08 20:15 ` Jim Blandy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox