diff mbox

[RFA] Handle DW_OP_form_tls_address

Message ID 1471907067-23458-1-git-send-email-tom@tromey.com
State New
Headers show

Commit Message

Tom Tromey Aug. 22, 2016, 11:04 p.m. UTC
Currently gdb supports DW_OP_GNU_push_tls_address, but not
DW_OP_form_tls_address.  I think it would be better if the toolchain
as a whole moved to using the standard opcode, and the prerequisite to
this is getting gdb to recognize it.

GCC can sometimes emit DW_OP_form_tls_address for emultls targets.  As
far as I know, nobody has ever tried this with gdb (since it wouldn't
work at all).

I don't think there's a major drawback to using a single opcode for
all targets, because computing the location of a thread-local is
already target specific.

This is PR gdb/11616.

I don't know how to write a test case for this; though it's worth
noting that there aren't explicit tests for DW_OP_GNU_push_tls_address
either -- and if I change GCC, these paths will be tested to the same
extent they are now.

2016-08-22  Tom Tromey  <tom@tromey.com>

	PR gdb/11616:
	* dwarf2read.c (decode_locdesc): Handle DW_OP_form_tls_address.
	* dwarf2loc.c (dwarf2_compile_expr_to_ax): Handle
	DW_OP_form_tls_address.
	(locexpr_describe_location_piece): Likewise.
	* dwarf2expr.h (struct dwarf_expr_context_funcs): Update comment.
	* dwarf2expr.c (execute_stack_op): Handle DW_OP_form_tls_address.
	(ctx_no_get_tls_address): Mention DW_OP_form_tls_address.
	* compile/compile-loc2c.c (struct insn_info): Update comment.
	(compute_stack_depth_worker): Handle DW_OP_form_tls_address.
---
 gdb/ChangeLog               | 13 +++++++++++++
 gdb/compile/compile-loc2c.c |  7 ++++---
 gdb/dwarf2expr.c            |  3 ++-
 gdb/dwarf2expr.h            |  2 +-
 gdb/dwarf2loc.c             |  7 +++++--
 gdb/dwarf2read.c            |  1 +
 6 files changed, 26 insertions(+), 7 deletions(-)

Comments

Pedro Alves Sept. 2, 2016, 10:45 a.m. UTC | #1
On 08/23/2016 12:04 AM, Tom Tromey wrote:
> Currently gdb supports DW_OP_GNU_push_tls_address, but not
> DW_OP_form_tls_address.  I think it would be better if the toolchain
> as a whole moved to using the standard opcode, and the prerequisite to
> this is getting gdb to recognize it.

Are the semantics of the two exactly the same?

> 
> GCC can sometimes emit DW_OP_form_tls_address for emultls targets.  As
> far as I know, nobody has ever tried this with gdb (since it wouldn't
> work at all).
> 
> I don't think there's a major drawback to using a single opcode for
> all targets, because computing the location of a thread-local is
> already target specific.
> 
> This is PR gdb/11616.
> 
> I don't know how to write a test case for this; though it's worth
> noting that there aren't explicit tests for DW_OP_GNU_push_tls_address
> either -- and if I change GCC, these paths will be tested to the same
> extent they are now.

I suppose that if we wanted to always test both, ideally we'd use
the dwarf assembler to write something simple like '__thread foo = 2;',
and then make sure we can print foo when we get to main.  We'd start out
with one arch, and then tweak the test to support or be skipped on
other tls model variations as people test on other archs.

> 
> 2016-08-22  Tom Tromey  <tom@tromey.com>
> 
> 	PR gdb/11616:
> 	* dwarf2read.c (decode_locdesc): Handle DW_OP_form_tls_address.
> 	* dwarf2loc.c (dwarf2_compile_expr_to_ax): Handle
> 	DW_OP_form_tls_address.
> 	(locexpr_describe_location_piece): Likewise.
> 	* dwarf2expr.h (struct dwarf_expr_context_funcs): Update comment.
> 	* dwarf2expr.c (execute_stack_op): Handle DW_OP_form_tls_address.
> 	(ctx_no_get_tls_address): Mention DW_OP_form_tls_address.
> 	* compile/compile-loc2c.c (struct insn_info): Update comment.
> 	(compute_stack_depth_worker): Handle DW_OP_form_tls_address.

LGTM as is.

Thanks,
Pedro Alves
Tom Tromey Sept. 2, 2016, 4:06 p.m. UTC | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 08/23/2016 12:04 AM, Tom Tromey wrote:
>> Currently gdb supports DW_OP_GNU_push_tls_address, but not
>> DW_OP_form_tls_address.  I think it would be better if the toolchain
>> as a whole moved to using the standard opcode, and the prerequisite to
>> this is getting gdb to recognize it.

Pedro> Are the semantics of the two exactly the same?

Yes, but with a footnote.

DW_OP_form_tls_address is described in the standard:

    The DW_OP_form_tls_address operation pops a value from the stack,
    translates it into an address in the current thread's thread-local
    storage block, and pushes the address. If the DWARF expression
    containing the DW_OP_form_tls_address operation belongs to the main
    executable's DWARF info, the operation uses the main executable's
    thread-local storage block; if the expression belongs to a shared
    library's DWARF info, then it uses that shared library's
    thread-local storage block.

DW_OP_GNU_push_tls_address isn't documented to my knowledge; however it
pops a value from the stack and translates it into an address.

In either case, it seems to me that there's also a lot left to the
implementation, so much so that we can treat them identically.

My theory is that maybe the GNU one was added first and then
standardized.  But I didn't do any archaeology to try to find out for
sure.

Pedro> LGTM as is.

Thanks.

Tom
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index db3527b..972a533 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@ 
+2016-08-22  Tom Tromey  <tom@tromey.com>
+
+	PR gdb/11616:
+	* dwarf2read.c (decode_locdesc): Handle DW_OP_form_tls_address.
+	* dwarf2loc.c (dwarf2_compile_expr_to_ax): Handle
+	DW_OP_form_tls_address.
+	(locexpr_describe_location_piece): Likewise.
+	* dwarf2expr.h (struct dwarf_expr_context_funcs): Update comment.
+	* dwarf2expr.c (execute_stack_op): Handle DW_OP_form_tls_address.
+	(ctx_no_get_tls_address): Mention DW_OP_form_tls_address.
+	* compile/compile-loc2c.c (struct insn_info): Update comment.
+	(compute_stack_depth_worker): Handle DW_OP_form_tls_address.
+
 2016-08-19  Yao Qi  <yao.qi@linaro.org>
 
 	* aarch64-tdep.c (aarch64_analyze_prologue): Handle register
diff --git a/gdb/compile/compile-loc2c.c b/gdb/compile/compile-loc2c.c
index 1a267b8..e8bef55 100644
--- a/gdb/compile/compile-loc2c.c
+++ b/gdb/compile/compile-loc2c.c
@@ -48,9 +48,9 @@  struct insn_info
 
   unsigned int label : 1;
 
-  /* Whether this instruction is DW_OP_GNU_push_tls_address.  This is
-     a hack until we can add a feature to glibc to let us properly
-     generate code for TLS.  */
+  /* Whether this instruction is DW_OP_GNU_push_tls_address or
+     DW_OP_form_tls_address.  This is a hack until we can add a
+     feature to glibc to let us properly generate code for TLS.  */
 
   unsigned int is_tls : 1;
 };
@@ -323,6 +323,7 @@  compute_stack_depth_worker (int start, int *need_tempvar,
 	  break;
 
 	case DW_OP_GNU_push_tls_address:
+	case DW_OP_form_tls_address:
 	  info[ndx].is_tls = 1;
 	  break;
 
diff --git a/gdb/dwarf2expr.c b/gdb/dwarf2expr.c
index 7bf3c78..7eb1982 100644
--- a/gdb/dwarf2expr.c
+++ b/gdb/dwarf2expr.c
@@ -1257,6 +1257,7 @@  execute_stack_op (struct dwarf_expr_context *ctx,
 	  break;
 
 	case DW_OP_GNU_push_tls_address:
+	case DW_OP_form_tls_address:
 	  /* Variable is at a constant offset in the thread-local
 	  storage block into the objfile for the current thread and
 	  the dynamic linker module containing this expression.  Here
@@ -1533,7 +1534,7 @@  ctx_no_get_frame_pc (void *baton)
 CORE_ADDR
 ctx_no_get_tls_address (void *baton, CORE_ADDR offset)
 {
-  error (_("%s is invalid in this context"), "DW_OP_GNU_push_tls_address");
+  error (_("%s is invalid in this context"), "DW_OP_form_tls_address");
 }
 
 /* Stub dwarf_expr_context_funcs.dwarf_call implementation.  */
diff --git a/gdb/dwarf2expr.h b/gdb/dwarf2expr.h
index 3d7ca00..cbab45b 100644
--- a/gdb/dwarf2expr.h
+++ b/gdb/dwarf2expr.h
@@ -56,7 +56,7 @@  struct dwarf_expr_context_funcs
   CORE_ADDR (*get_frame_pc) (void *baton);
 
   /* Return the thread-local storage address for
-     DW_OP_GNU_push_tls_address.  */
+     DW_OP_GNU_push_tls_address or DW_OP_form_tls_address.  */
   CORE_ADDR (*get_tls_address) (void *baton, CORE_ADDR offset);
 
   /* Execute DW_AT_location expression for the DWARF expression subroutine in
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index e60475f..946ddf8 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -3571,6 +3571,7 @@  dwarf2_compile_expr_to_ax (struct agent_expr *expr, struct axs_value *loc,
 	  break;
 
 	case DW_OP_GNU_push_tls_address:
+	case DW_OP_form_tls_address:
 	  unimplemented (op);
 	  break;
 
@@ -3907,7 +3908,8 @@  locexpr_describe_location_piece (struct symbol *symbol, struct ui_file *stream,
 	   && (data[0] == DW_OP_addr
 	       || (addr_size == 4 && data[0] == DW_OP_const4u)
 	       || (addr_size == 8 && data[0] == DW_OP_const8u))
-	   && data[1 + addr_size] == DW_OP_GNU_push_tls_address
+	   && (data[1 + addr_size] == DW_OP_GNU_push_tls_address
+	       || data[1 + addr_size] == DW_OP_form_tls_address)
 	   && piece_end_p (data + 2 + addr_size, end))
     {
       ULONGEST offset;
@@ -3930,7 +3932,8 @@  locexpr_describe_location_piece (struct symbol *symbol, struct ui_file *stream,
 	   && data + 1 + (leb128_size = skip_leb128 (data + 1, end)) < end
 	   && data[0] == DW_OP_GNU_const_index
 	   && leb128_size > 0
-	   && data[1 + leb128_size] == DW_OP_GNU_push_tls_address
+	   && (data[1 + leb128_size] == DW_OP_GNU_push_tls_address
+	       || data[1 + leb128_size] == DW_OP_form_tls_address)
 	   && piece_end_p (data + 2 + leb128_size, end))
     {
       uint64_t offset;
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 924b417..9ad2caa 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -20817,6 +20817,7 @@  decode_locdesc (struct dwarf_block *blk, struct dwarf2_cu *cu)
 	  break;
 
         case DW_OP_GNU_push_tls_address:
+	case DW_OP_form_tls_address:
 	  /* The top of the stack has the offset from the beginning
 	     of the thread control block at which the variable is located.  */
 	  /* Nothing should follow this operator, so the top of stack would