[2/4] Use the target architecture when encoding tracepoint actions

Message ID 1452188697-23870-3-git-send-email-antoine.tremblay@ericsson.com
State New, archived
Headers

Commit Message

Antoine Tremblay Jan. 7, 2016, 5:44 p.m. UTC
  This patch uses the target architecture rather then the objfile
architecture when encoding tracepoint actions.

The target architecture may contain additional registers. E.g. ARM VFP
registers. This information is needed to allow their collection. Since we
can never know whether the registers numbers in the target match the
binary's we have to use tdesc here.

One note about combined debuggers / multi-inferior from Pedro Alves:

In the combined debugger case taking Cell as the practical example that
gdb supports currently:

In that case, the main target_gdbarch() will be powerpc, but you may have set a
tracepoint on _spu_ code, which has a different gdbarch.  so for that case,
target_gdbarch would be wrong.  I think that in that case, we'd need to
find __the_ target/tdesc gdbarch that is (bfd) compatible with the
objfile's gdbarch.

I think cell/spu gdbserver doesn't support tracepoints, so we can ignore
this for now.

The multi-inferior/process case is somewhat related, but its simpler.
each inferior has its own gdbarch.

That is, target_gdbarch depends on the current inferior selected.
In fact, that just returns inferior->gdbarch nowaways.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* tracepoint.c (encode_actions_1): Use target_gdbarch () rather
	than loc->gdbarch.
---
 gdb/tracepoint.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
  

Comments

Marcin Koƛcielnicki Feb. 6, 2016, 8:58 p.m. UTC | #1
On 07/01/16 18:44, Antoine Tremblay wrote:
> This patch uses the target architecture rather then the objfile
> architecture when encoding tracepoint actions.
>
> The target architecture may contain additional registers. E.g. ARM VFP
> registers. This information is needed to allow their collection. Since we
> can never know whether the registers numbers in the target match the
> binary's we have to use tdesc here.
>
> One note about combined debuggers / multi-inferior from Pedro Alves:
>
> In the combined debugger case taking Cell as the practical example that
> gdb supports currently:
>
> In that case, the main target_gdbarch() will be powerpc, but you may have set a
> tracepoint on _spu_ code, which has a different gdbarch.  so for that case,
> target_gdbarch would be wrong.  I think that in that case, we'd need to
> find __the_ target/tdesc gdbarch that is (bfd) compatible with the
> objfile's gdbarch.
>
> I think cell/spu gdbserver doesn't support tracepoints, so we can ignore
> this for now.
>
> The multi-inferior/process case is somewhat related, but its simpler.
> each inferior has its own gdbarch.
>
> That is, target_gdbarch depends on the current inferior selected.
> In fact, that just returns inferior->gdbarch nowaways.
>
> No regressions, tested on ubuntu 14.04 ARMv7 and x86.
> With gdbserver-{native,extended} / { -marm -mthumb }
>
> gdb/ChangeLog:
>
> 	* tracepoint.c (encode_actions_1): Use target_gdbarch () rather
> 	than loc->gdbarch.
 > [...]

Hey, could we get that one pushed soon?  I've made a patchset that adds 
tdesc information to tfile format, making it work properly for 
multiple-tdesc architectures 
(https://sourceware.org/ml/gdb-patches/2016-02/msg00161.html), and made 
x86_64 work with collecting AVX registers 
(https://sourceware.org/ml/gdb-patches/2016-02/msg00167.html), so that 
patch could do a lot of good already.

Marcin Koƛcielnicki
  
Pedro Alves Feb. 11, 2016, 1:02 p.m. UTC | #2
On 01/07/2016 05:44 PM, Antoine Tremblay wrote:

> gdb/ChangeLog:
> 
> 	* tracepoint.c (encode_actions_1): Use target_gdbarch () rather
> 	than loc->gdbarch.

OK, please push.

Thanks,
Pedro Alves
  
Antoine Tremblay Feb. 11, 2016, 1:20 p.m. UTC | #3
Pedro Alves writes:

> On 01/07/2016 05:44 PM, Antoine Tremblay wrote:
>
>> gdb/ChangeLog:
>> 
>> 	* tracepoint.c (encode_actions_1): Use target_gdbarch () rather
>> 	than loc->gdbarch.
>
> OK, please push.
>
> Thanks,
> Pedro Alves

Pushed in.

Thanks,
Antoine
  

Patch

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 55b2e8e..f7f5736 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1428,14 +1428,14 @@  encode_actions_1 (struct command_line *action,
 
 	      if (0 == strncasecmp ("$reg", action_exp, 4))
 		{
-		  for (i = 0; i < gdbarch_num_regs (tloc->gdbarch); i++)
+		  for (i = 0; i < gdbarch_num_regs (target_gdbarch ()); i++)
 		    add_register (collect, i);
 		  action_exp = strchr (action_exp, ',');	/* more? */
 		}
 	      else if (0 == strncasecmp ("$arg", action_exp, 4))
 		{
 		  add_local_symbols (collect,
-				     tloc->gdbarch,
+				     target_gdbarch (),
 				     tloc->address,
 				     frame_reg,
 				     frame_offset,
@@ -1446,7 +1446,7 @@  encode_actions_1 (struct command_line *action,
 	      else if (0 == strncasecmp ("$loc", action_exp, 4))
 		{
 		  add_local_symbols (collect,
-				     tloc->gdbarch,
+				     target_gdbarch (),
 				     tloc->address,
 				     frame_reg,
 				     frame_offset,
@@ -1459,7 +1459,7 @@  encode_actions_1 (struct command_line *action,
 		  struct cleanup *old_chain1 = NULL;
 
 		  aexpr = gen_trace_for_return_address (tloc->address,
-							tloc->gdbarch,
+							target_gdbarch (),
 							trace_string);
 
 		  old_chain1 = make_cleanup_free_agent_expr (aexpr);
@@ -1513,7 +1513,7 @@  encode_actions_1 (struct command_line *action,
 		      {
 			const char *name = &exp->elts[2].string;
 
-			i = user_reg_map_name_to_regnum (tloc->gdbarch,
+			i = user_reg_map_name_to_regnum (target_gdbarch (),
 							 name, strlen (name));
 			if (i == -1)
 			  internal_error (__FILE__, __LINE__,
@@ -1543,7 +1543,7 @@  encode_actions_1 (struct command_line *action,
 
 			collect_symbol (collect,
 					exp->elts[2].symbol,
-					tloc->gdbarch,
+					target_gdbarch (),
 					frame_reg,
 					frame_offset,
 					tloc->address,