diff mbox

PR debug/77315 - use DW_OP_form_tls_address

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

Commit Message

Tom Tromey Oct. 21, 2016, 5:17 p.m. UTC
This patch changes gcc to emit DW_OP_form_tls_address rather than
DW_OP_GNU_push_tls_address.  This is PR debug/77315.

DW_OP_form_tls_address was added in DWARF 3, and this patch uses the
DWARF version to decide which opcode to emit.

Note that gdb did not implement the DWARF 3 opcode until recently -- in
fact it isn't even in 7.12 (I thought I had put it in there but it seems
not).  I'm not sure if that means that this should wait a cycle.

Built and regtested on x86-64 Fedora 24.

2016-10-21  Tom Tromey  <tom@tromey.com>

	PR debug/77315:
	* dwarf2out.c (mem_loc_descriptor): Use DW_OP_form_tls_address.
	(resolve_args_picking_1): Move DW_OP_form_tls_address case next to
	DW_OP_GNU_push_tls_address case.
	(loc_list_from_tree_1): Use DW_OP_form_tls_address.
---
 gcc/ChangeLog   |  8 ++++++++
 gcc/dwarf2out.c | 12 ++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

Comments

Jakub Jelinek Oct. 21, 2016, 5:26 p.m. UTC | #1
On Fri, Oct 21, 2016 at 11:17:33AM -0600, Tom Tromey wrote:
> This patch changes gcc to emit DW_OP_form_tls_address rather than
> DW_OP_GNU_push_tls_address.  This is PR debug/77315.
> 
> DW_OP_form_tls_address was added in DWARF 3, and this patch uses the
> DWARF version to decide which opcode to emit.
> 
> Note that gdb did not implement the DWARF 3 opcode until recently -- in
> fact it isn't even in 7.12 (I thought I had put it in there but it seems
> not).  I'm not sure if that means that this should wait a cycle.
> 
> Built and regtested on x86-64 Fedora 24.

I admit I haven't looked at the GDB changes, but how will the debugger know
if it is an emutls emulation or normal ELF TLS, if the same op is used
in both cases?

Also, as this effectively requires the latest unreleased GDB under the
default options for something that has been working previously, I wonder
if it e.g. for some time shouldn't be guarded with dwarf_version >= 5
(which will require substantial changes in gdb anyway), and only be changed
back to dwarf_version >= 3 in 2 years or so when newer debugger will be much
more common.

> 2016-10-21  Tom Tromey  <tom@tromey.com>
> 
> 	PR debug/77315:
> 	* dwarf2out.c (mem_loc_descriptor): Use DW_OP_form_tls_address.
> 	(resolve_args_picking_1): Move DW_OP_form_tls_address case next to
> 	DW_OP_GNU_push_tls_address case.
> 	(loc_list_from_tree_1): Use DW_OP_form_tls_address.
> ---
>  gcc/ChangeLog   |  8 ++++++++
>  gcc/dwarf2out.c | 12 ++++++++----
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 6102719..481a2a9 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,11 @@
> +2016-10-21  Tom Tromey  <tom@tromey.com>
> +
> +	PR debug/77315:
> +	* dwarf2out.c (mem_loc_descriptor): Use DW_OP_form_tls_address.
> +	(resolve_args_picking_1): Move DW_OP_form_tls_address case next to
> +	DW_OP_GNU_push_tls_address case.
> +	(loc_list_from_tree_1): Use DW_OP_form_tls_address.
> +
>  2016-10-21  Jakub Jelinek  <jakub@redhat.com>
>  
>  	* config/i386/adxintrin.h (_subborrow_u32, _addcarry_u32,
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index 4683e1c..6ab6ff8 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -13619,7 +13619,10 @@ mem_loc_descriptor (rtx rtl, machine_mode mode,
>  
>            temp = new_addr_loc_descr (rtl, dtprel_true);
>  
> -	  mem_loc_result = new_loc_descr (DW_OP_GNU_push_tls_address, 0, 0);
> +	  mem_loc_result = new_loc_descr ((dwarf_version > 2
> +					   ? DW_OP_form_tls_address
> +					   : DW_OP_GNU_push_tls_address),
> +					  0, 0);
>  	  add_loc_descr (&mem_loc_result, temp);
>  
>  	  break;
> @@ -15467,7 +15470,6 @@ resolve_args_picking_1 (dw_loc_descr_ref loc, unsigned initial_frame_offset,
>  	case DW_OP_piece:
>  	case DW_OP_deref_size:
>  	case DW_OP_nop:
> -	case DW_OP_form_tls_address:
>  	case DW_OP_bit_piece:
>  	case DW_OP_implicit_value:
>  	case DW_OP_stack_value:
> @@ -15595,6 +15597,7 @@ resolve_args_picking_1 (dw_loc_descr_ref loc, unsigned initial_frame_offset,
>  	    break;
>  	  }
>  
> +	case DW_OP_form_tls_address:
>  	case DW_OP_GNU_push_tls_address:
>  	case DW_OP_GNU_uninit:
>  	case DW_OP_GNU_encoded_addr:
> @@ -15924,8 +15927,9 @@ loc_list_from_tree_1 (tree loc, int want_address,
>  		  operand shouldn't be.  */
>  	      if (DECL_EXTERNAL (loc) && !targetm.binds_local_p (loc))
>  		return 0;
> -             dtprel = dtprel_true;
> -             tls_op = DW_OP_GNU_push_tls_address;
> +	      dtprel = dtprel_true;
> +	      tls_op = (dwarf_version > 2 ? DW_OP_form_tls_address
> +			: DW_OP_GNU_push_tls_address);
>  	    }
>  	  else
>  	    {
> -- 
> 2.7.4

	Jakub
Tom Tromey Oct. 21, 2016, 5:36 p.m. UTC | #2
>>>>> "Jakub" == Jakub Jelinek <jakub@redhat.com> writes:

Jakub> I admit I haven't looked at the GDB changes, but how will the debugger know
Jakub> if it is an emutls emulation or normal ELF TLS, if the same op is used
Jakub> in both cases?

Because gdb never implemented DW_OP_form_tls_address, that emultls case
has never worked.

My view is that, if it ought to work, then this has to be some sort of
platform-specific thing.  gdb already lets different platforms implement
TLS lookup differently; it can be done by the target or by the arch, see
target_translate_tls_address.

My suspicion is that the history here is just that
DW_OP_GNU_push_tls_address was introduced to solve this problem, then
formalized in DWARF, and then for some reason never followed up on.

Jakub> Also, as this effectively requires the latest unreleased GDB under the
Jakub> default options for something that has been working previously, I wonder
Jakub> if it e.g. for some time shouldn't be guarded with dwarf_version >= 5
Jakub> (which will require substantial changes in gdb anyway), and only be changed
Jakub> back to dwarf_version >= 3 in 2 years or so when newer debugger will be much
Jakub> more common.

I'd be amenable to that.  It did occur to me after sending that the
DWARF 5 work is going to require a new gdb anyhow.

Tom
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 6102719..481a2a9 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@ 
+2016-10-21  Tom Tromey  <tom@tromey.com>
+
+	PR debug/77315:
+	* dwarf2out.c (mem_loc_descriptor): Use DW_OP_form_tls_address.
+	(resolve_args_picking_1): Move DW_OP_form_tls_address case next to
+	DW_OP_GNU_push_tls_address case.
+	(loc_list_from_tree_1): Use DW_OP_form_tls_address.
+
 2016-10-21  Jakub Jelinek  <jakub@redhat.com>
 
 	* config/i386/adxintrin.h (_subborrow_u32, _addcarry_u32,
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 4683e1c..6ab6ff8 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -13619,7 +13619,10 @@  mem_loc_descriptor (rtx rtl, machine_mode mode,
 
           temp = new_addr_loc_descr (rtl, dtprel_true);
 
-	  mem_loc_result = new_loc_descr (DW_OP_GNU_push_tls_address, 0, 0);
+	  mem_loc_result = new_loc_descr ((dwarf_version > 2
+					   ? DW_OP_form_tls_address
+					   : DW_OP_GNU_push_tls_address),
+					  0, 0);
 	  add_loc_descr (&mem_loc_result, temp);
 
 	  break;
@@ -15467,7 +15470,6 @@  resolve_args_picking_1 (dw_loc_descr_ref loc, unsigned initial_frame_offset,
 	case DW_OP_piece:
 	case DW_OP_deref_size:
 	case DW_OP_nop:
-	case DW_OP_form_tls_address:
 	case DW_OP_bit_piece:
 	case DW_OP_implicit_value:
 	case DW_OP_stack_value:
@@ -15595,6 +15597,7 @@  resolve_args_picking_1 (dw_loc_descr_ref loc, unsigned initial_frame_offset,
 	    break;
 	  }
 
+	case DW_OP_form_tls_address:
 	case DW_OP_GNU_push_tls_address:
 	case DW_OP_GNU_uninit:
 	case DW_OP_GNU_encoded_addr:
@@ -15924,8 +15927,9 @@  loc_list_from_tree_1 (tree loc, int want_address,
 		  operand shouldn't be.  */
 	      if (DECL_EXTERNAL (loc) && !targetm.binds_local_p (loc))
 		return 0;
-             dtprel = dtprel_true;
-             tls_op = DW_OP_GNU_push_tls_address;
+	      dtprel = dtprel_true;
+	      tls_op = (dwarf_version > 2 ? DW_OP_form_tls_address
+			: DW_OP_GNU_push_tls_address);
 	    }
 	  else
 	    {