[PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Marcin Kościelnicki]

The struct tracepoint_action has an ops field, pointing to
a tracepoint_action_ops structure, containing send and download ops.
However, this field is only present when compiled in gdbserver, and not
when compiled in IPA.  When gdbserver is downloading tracepoint actions
to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
its struct tracepoint_action, to get to the part that corresponds to
IPA's struct tracepoint_action.

Unfortunately, this fails badly on ILP32 platforms where alignof(long long)
== 8.  Consider struct collect_memory_action layout in gdbserver:

0-3: base.ops
4: base.type
8-15: addr
16-23: len
24-27: basereg
sizeof == 32

and its layout in IPA:

0: base.type
8-15: addr
16-23: len
24-27: basereg
sizeof == 32

When gdbserver tries to download it to IPA, it skips the first 4 bytes
(base.ops), figuring the rest will match what IPA expects - which is
not true, since addr is aligned to 8 bytes and will be at a different
relative position to base.type.

The problem went unnoticed on the currently supported platforms, since
aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte
alignment for long long.

There are a few possible ways around this problem.  I decided on removing
ops altogether, since they can be easily inlined in their (only) places
of use - in fact allowing us share the code between 'L' and 'R'.  Any
approach where struct tracepoint_action is different between IPA and
gdbserver is just asking for trouble.

Found on s390.  Tested on x86_64, s390, s390x.

gdb/gdbserver/ChangeLog:

	* tracepoint.c (struct tracepoint_action_ops): Removed.
	(struct tracepoint_action): Removed ops field.
	(struct collect_registers_action): Removed.
	(struct collect_static_trace_data_action): Removed.
	(m_tracepoint_action_download): Removed.
	(m_tracepoint_action_send): Removed.
	(r_tracepoint_action_download): Removed.
	(r_tracepoint_action_send): Removed.
	(x_tracepoint_action_download): Removed.
	(x_tracepoint_action_send): Removed.
	(l_tracepoint_action_download): Removed.
	(l_tracepoint_action_send): Removed.
	(add_tracepoint_action): Remove ops setting.
	(download_tracepoint_1): Download actions inline instead of using ops.
	(tracepoint_send_agent): Send actions inline instead of using ops.
---
 gdb/gdbserver/ChangeLog    |  18 ++++
 gdb/gdbserver/tracepoint.c | 251 +++++++++++++++------------------------------
 2 files changed, 98 insertions(+), 171 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index a25dc9b..ec35f44 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,23 @@
 2016-01-23  Marcin Kościelnicki  <koriakin@0x04.net>
 
+	* tracepoint.c (struct tracepoint_action_ops): Removed.
+	(struct tracepoint_action): Removed ops field.
+	(struct collect_registers_action): Removed.
+	(struct collect_static_trace_data_action): Removed.
+	(m_tracepoint_action_download): Removed.
+	(m_tracepoint_action_send): Removed.
+	(r_tracepoint_action_download): Removed.
+	(r_tracepoint_action_send): Removed.
+	(x_tracepoint_action_download): Removed.
+	(x_tracepoint_action_send): Removed.
+	(l_tracepoint_action_download): Removed.
+	(l_tracepoint_action_send): Removed.
+	(add_tracepoint_action): Remove ops setting.
+	(download_tracepoint_1): Download actions inline instead of using ops.
+	(tracepoint_send_agent): Send actions inline instead of using ops.
+
+2016-01-23  Marcin Kościelnicki  <koriakin@0x04.net>
+
 	* gdb.trace/pending.exp: Fix expected message on continue.
 
 2016-01-22  Marcin Kościelnicki  <koriakin@0x04.net>
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 0671999..3b871dd 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -464,26 +464,10 @@ static int write_inferior_data_ptr (CORE_ADDR where, CORE_ADDR ptr);
 
 /* Operations on various types of tracepoint actions.  */
 
-struct tracepoint_action;
-
-struct tracepoint_action_ops
-{
-  /* Download tracepoint action ACTION to IPA.  Return the address of action
-     in IPA/inferior.  */
-  CORE_ADDR (*download) (const struct tracepoint_action *action);
-
-  /* Send ACTION to agent via command buffer started from BUFFER.  Return
-     updated head of command buffer.  */
-  char* (*send) (char *buffer, const struct tracepoint_action *action);
-};
-
 /* Base action.  Concrete actions inherit this.  */
 
 struct tracepoint_action
 {
-#ifndef IN_PROCESS_AGENT
-  const struct tracepoint_action_ops *ops;
-#endif
   char type;
 };
 
@@ -497,13 +481,6 @@ struct collect_memory_action
   int32_t basereg;
 };
 
-/* An 'R' (collect registers) action.  */
-
-struct collect_registers_action
-{
-  struct tracepoint_action base;
-};
-
 /* An 'X' (evaluate expression) action.  */
 
 struct eval_expr_action
@@ -513,89 +490,9 @@ struct eval_expr_action
   struct agent_expr *expr;
 };
 
-/* An 'L' (collect static trace data) action.  */
-struct collect_static_trace_data_action
-{
-  struct tracepoint_action base;
-};
-
 #ifndef IN_PROCESS_AGENT
-static CORE_ADDR
-m_tracepoint_action_download (const struct tracepoint_action *action)
-{
-  int size_in_ipa = (sizeof (struct collect_memory_action)
-		     - offsetof (struct tracepoint_action, type));
-  CORE_ADDR ipa_action = target_malloc (size_in_ipa);
-
-  write_inferior_memory (ipa_action, (unsigned char *) &action->type,
-			 size_in_ipa);
-
-  return ipa_action;
-}
-static char *
-m_tracepoint_action_send (char *buffer, const struct tracepoint_action *action)
-{
-  struct collect_memory_action *maction
-    = (struct collect_memory_action *) action;
-
-  COPY_FIELD_TO_BUF (buffer, maction, addr);
-  COPY_FIELD_TO_BUF (buffer, maction, len);
-  COPY_FIELD_TO_BUF (buffer, maction, basereg);
-
-  return buffer;
-}
-
-static const struct tracepoint_action_ops m_tracepoint_action_ops =
-{
-  m_tracepoint_action_download,
-  m_tracepoint_action_send,
-};
-
-static CORE_ADDR
-r_tracepoint_action_download (const struct tracepoint_action *action)
-{
-  int size_in_ipa = (sizeof (struct collect_registers_action)
-		     - offsetof (struct tracepoint_action, type));
-  CORE_ADDR ipa_action  = target_malloc (size_in_ipa);
-
-  write_inferior_memory (ipa_action, (unsigned char *) &action->type,
-			size_in_ipa);
-
-  return ipa_action;
-}
-
-static char *
-r_tracepoint_action_send (char *buffer, const struct tracepoint_action *action)
-{
-  return buffer;
-}
-
-static const struct tracepoint_action_ops r_tracepoint_action_ops =
-{
-  r_tracepoint_action_download,
-  r_tracepoint_action_send,
-};
-
 static CORE_ADDR download_agent_expr (struct agent_expr *expr);
 
-static CORE_ADDR
-x_tracepoint_action_download (const struct tracepoint_action *action)
-{
-  int size_in_ipa = (sizeof (struct eval_expr_action)
-		     - offsetof (struct tracepoint_action, type));
-  CORE_ADDR ipa_action = target_malloc (size_in_ipa);
-  CORE_ADDR expr;
-
-  write_inferior_memory (ipa_action, (unsigned char *) &action->type,
-			 size_in_ipa);
-  expr = download_agent_expr (((struct eval_expr_action *)action)->expr);
-  write_inferior_data_ptr (ipa_action + offsetof (struct eval_expr_action, expr)
-			   - offsetof (struct tracepoint_action, type),
-			   expr);
-
-  return ipa_action;
-}
-
 /* Copy agent expression AEXPR to buffer pointed by P.  If AEXPR is NULL,
    copy 0 to P.  Return updated header of buffer.  */
 
@@ -619,45 +516,6 @@ agent_expr_send (char *p, const struct agent_expr *aexpr)
     }
   return p;
 }
-
-static char *
-x_tracepoint_action_send ( char *buffer, const struct tracepoint_action *action)
-{
-  struct eval_expr_action *eaction = (struct eval_expr_action *) action;
-
-  return agent_expr_send (buffer, eaction->expr);
-}
-
-static const struct tracepoint_action_ops x_tracepoint_action_ops =
-{
-  x_tracepoint_action_download,
-  x_tracepoint_action_send,
-};
-
-static CORE_ADDR
-l_tracepoint_action_download (const struct tracepoint_action *action)
-{
-  int size_in_ipa = (sizeof (struct collect_static_trace_data_action)
-		     - offsetof (struct tracepoint_action, type));
-  CORE_ADDR ipa_action = target_malloc (size_in_ipa);
-
-  write_inferior_memory (ipa_action, (unsigned char *) &action->type,
-			 size_in_ipa);
-
-  return ipa_action;
-}
-
-static char *
-l_tracepoint_action_send (char *buffer, const struct tracepoint_action *action)
-{
-  return buffer;
-}
-
-static const struct tracepoint_action_ops l_tracepoint_action_ops =
-{
-  l_tracepoint_action_download,
-  l_tracepoint_action_send,
-};
 #endif
 
 /* This structure describes a piece of the source-level definition of
@@ -1944,7 +1802,6 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
 	    int is_neg;
 
 	    maction->base.type = *act;
-	    maction->base.ops = &m_tracepoint_action_ops;
 	    action = &maction->base;
 
 	    ++act;
@@ -1965,34 +1822,22 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
 	    break;
 	  }
 	case 'R':
-	  {
-	    struct collect_registers_action *raction =
-	      XNEW (struct collect_registers_action);
+	  action = XNEW (struct tracepoint_action);
+	  action->type = *act;
 
-	    raction->base.type = *act;
-	    raction->base.ops = &r_tracepoint_action_ops;
-	    action = &raction->base;
-
-	    trace_debug ("Want to collect registers");
+	  trace_debug ("Want to collect registers");
+	  ++act;
+	  /* skip past hex digits of mask for now */
+	  while (isxdigit(*act))
 	    ++act;
-	    /* skip past hex digits of mask for now */
-	    while (isxdigit(*act))
-	      ++act;
-	    break;
-	  }
+	  break;
 	case 'L':
-	  {
-	    struct collect_static_trace_data_action *raction =
-	      XNEW (struct collect_static_trace_data_action);
-
-	    raction->base.type = *act;
-	    raction->base.ops = &l_tracepoint_action_ops;
-	    action = &raction->base;
+	  action = XNEW (struct tracepoint_action);
+	  action->type = *act;
 
-	    trace_debug ("Want to collect static trace data");
-	    ++act;
-	    break;
-	  }
+	  trace_debug ("Want to collect static trace data");
+	  ++act;
+	  break;
 	case 'S':
 	  trace_debug ("Unexpected step action, ignoring");
 	  ++act;
@@ -2002,7 +1847,6 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
 	    struct eval_expr_action *xaction = XNEW (struct eval_expr_action);
 
 	    xaction->base.type = *act;
-	    xaction->base.ops = &x_tracepoint_action_ops;
 	    action = &xaction->base;
 
 	    trace_debug ("Want to evaluate expression");
@@ -6066,7 +5910,43 @@ download_tracepoint_1 (struct tracepoint *tpoint)
       for (i = 0; i < tpoint->numactions; i++)
 	{
 	  struct tracepoint_action *action = tpoint->actions[i];
-	  CORE_ADDR ipa_action = action->ops->download (action);
+	  CORE_ADDR ipa_action = 0;
+	  CORE_ADDR expr;
+	  int size_in_ipa = 0;
+
+	  switch (action->type)
+	    {
+	    case 'M':
+	      size_in_ipa = sizeof (struct collect_memory_action);
+	      ipa_action = target_malloc (size_in_ipa);
+	      write_inferior_memory (ipa_action, (unsigned char *) action,
+				     size_in_ipa);
+	      break;
+
+	    case 'R':
+	    case 'L':
+	      size_in_ipa = sizeof *action;
+	      ipa_action  = target_malloc (size_in_ipa);
+	      write_inferior_memory (ipa_action, (unsigned char *) action,
+				     size_in_ipa);
+	      break;
+
+	    case 'X':
+	      size_in_ipa = sizeof (struct eval_expr_action);
+	      ipa_action = target_malloc (size_in_ipa);
+	      write_inferior_memory (ipa_action, (unsigned char *) action,
+				     size_in_ipa);
+	      expr = download_agent_expr (((struct eval_expr_action *)action)->expr);
+	      write_inferior_data_ptr (ipa_action +
+				       offsetof (struct eval_expr_action, expr),
+				       expr);
+	      break;
+
+	    default:
+	      internal_error (__FILE__, __LINE__,
+			      "unknown tracepoint action '%c'", action->type);
+	      break;
+	    }
 
 	  if (ipa_action != 0)
 	    write_inferior_data_ptr
@@ -6115,8 +5995,37 @@ tracepoint_send_agent (struct tracepoint *tpoint)
     {
       struct tracepoint_action *action = tpoint->actions[i];
 
-      p[0] = action->type;
-      p = action->ops->send (&p[1], action);
+      *p++ = action->type;
+
+      switch (action->type)
+	{
+	  case 'M':
+	    {
+	      struct collect_memory_action *maction
+	        = (struct collect_memory_action *) action;
+
+	      COPY_FIELD_TO_BUF (p, maction, addr);
+	      COPY_FIELD_TO_BUF (p, maction, len);
+	      COPY_FIELD_TO_BUF (p, maction, basereg);
+	      break;
+	    }
+
+	  case 'R':
+	  case 'L':
+	    break;
+
+	  case 'X':
+	    {
+	      struct eval_expr_action *eaction = (struct eval_expr_action *) action;
+	      p = agent_expr_send (p, eaction->expr);
+	      break;
+	    }
+
+	  default:
+	    internal_error (__FILE__, __LINE__,
+			    "unknown tracepoint action '%c'", action->type);
+	    break;
+	}
     }
 
   get_jump_space_head ();
-- 
2.7.0

Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Pedro Alves]

On 01/23/2016 07:31 PM, Marcin Kościelnicki wrote:
> The struct tracepoint_action has an ops field, pointing to
> a tracepoint_action_ops structure, containing send and download ops.
> However, this field is only present when compiled in gdbserver, and not
> when compiled in IPA.  When gdbserver is downloading tracepoint actions
> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
> its struct tracepoint_action, to get to the part that corresponds to
> IPA's struct tracepoint_action.
> 
> Unfortunately, this fails badly on ILP32 platforms where alignof(long long)
> == 8.  Consider struct collect_memory_action layout in gdbserver:
> 
> 0-3: base.ops
> 4: base.type
> 8-15: addr
> 16-23: len
> 24-27: basereg
> sizeof == 32
> 
> and its layout in IPA:
> 
> 0: base.type
> 8-15: addr
> 16-23: len
> 24-27: basereg
> sizeof == 32
> 
> When gdbserver tries to download it to IPA, it skips the first 4 bytes
> (base.ops), figuring the rest will match what IPA expects - which is
> not true, since addr is aligned to 8 bytes and will be at a different
> relative position to base.type.
> 
> The problem went unnoticed on the currently supported platforms, since
> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte
> alignment for long long.
> 
> There are a few possible ways around this problem.  I decided on removing
> ops altogether, since they can be easily inlined in their (only) places
> of use - in fact allowing us share the code between 'L' and 'R'.  Any
> approach where struct tracepoint_action is different between IPA and
> gdbserver is just asking for trouble.
> 
> Found on s390.  Tested on x86_64, s390, s390x.

Hmm, this is essentially the same as:

 https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html

Right?

Seems that other patch inlines things a bit less though, which offhand
looks preferable.  WDYT?

Not sure what happened to that series.  I thought most of it (if not all)
had been approved already.

Thanks,
Pedro Alves

Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Marcin Kościelnicki]

On 25/01/16 12:53, Pedro Alves wrote:
> On 01/23/2016 07:31 PM, Marcin Kościelnicki wrote:
>> The struct tracepoint_action has an ops field, pointing to
>> a tracepoint_action_ops structure, containing send and download ops.
>> However, this field is only present when compiled in gdbserver, and not
>> when compiled in IPA.  When gdbserver is downloading tracepoint actions
>> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
>> its struct tracepoint_action, to get to the part that corresponds to
>> IPA's struct tracepoint_action.
>>
>> Unfortunately, this fails badly on ILP32 platforms where alignof(long long)
>> == 8.  Consider struct collect_memory_action layout in gdbserver:
>>
>> 0-3: base.ops
>> 4: base.type
>> 8-15: addr
>> 16-23: len
>> 24-27: basereg
>> sizeof == 32
>>
>> and its layout in IPA:
>>
>> 0: base.type
>> 8-15: addr
>> 16-23: len
>> 24-27: basereg
>> sizeof == 32
>>
>> When gdbserver tries to download it to IPA, it skips the first 4 bytes
>> (base.ops), figuring the rest will match what IPA expects - which is
>> not true, since addr is aligned to 8 bytes and will be at a different
>> relative position to base.type.
>>
>> The problem went unnoticed on the currently supported platforms, since
>> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte
>> alignment for long long.
>>
>> There are a few possible ways around this problem.  I decided on removing
>> ops altogether, since they can be easily inlined in their (only) places
>> of use - in fact allowing us share the code between 'L' and 'R'.  Any
>> approach where struct tracepoint_action is different between IPA and
>> gdbserver is just asking for trouble.
>>
>> Found on s390.  Tested on x86_64, s390, s390x.
>
> Hmm, this is essentially the same as:
>
>   https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html
>
> Right?
>
> Seems that other patch inlines things a bit less though, which offhand
> looks preferable.  WDYT?
>
> Not sure what happened to that series.  I thought most of it (if not all)
> had been approved already.
>
> Thanks,
> Pedro Alves
>

Huh, I didn't know about that patch series.  Good to know, since I was 
going to try doing ppc tracepoints next, and had no idea that has 
already been attempted.  What happened to that patchset/author?  Kind of 
strange to abandon mostly-finished code when there's a $3k bounty on it.

The other patch looks fine too, I have no preference here.

Marcin Kościelnicki

Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Marcin Kościelnicki]

On 25/01/16 12:53, Pedro Alves wrote:
> On 01/23/2016 07:31 PM, Marcin Kościelnicki wrote:
>> The struct tracepoint_action has an ops field, pointing to
>> a tracepoint_action_ops structure, containing send and download ops.
>> However, this field is only present when compiled in gdbserver, and not
>> when compiled in IPA.  When gdbserver is downloading tracepoint actions
>> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
>> its struct tracepoint_action, to get to the part that corresponds to
>> IPA's struct tracepoint_action.
>>
>> Unfortunately, this fails badly on ILP32 platforms where alignof(long long)
>> == 8.  Consider struct collect_memory_action layout in gdbserver:
>>
>> 0-3: base.ops
>> 4: base.type
>> 8-15: addr
>> 16-23: len
>> 24-27: basereg
>> sizeof == 32
>>
>> and its layout in IPA:
>>
>> 0: base.type
>> 8-15: addr
>> 16-23: len
>> 24-27: basereg
>> sizeof == 32
>>
>> When gdbserver tries to download it to IPA, it skips the first 4 bytes
>> (base.ops), figuring the rest will match what IPA expects - which is
>> not true, since addr is aligned to 8 bytes and will be at a different
>> relative position to base.type.
>>
>> The problem went unnoticed on the currently supported platforms, since
>> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte
>> alignment for long long.
>>
>> There are a few possible ways around this problem.  I decided on removing
>> ops altogether, since they can be easily inlined in their (only) places
>> of use - in fact allowing us share the code between 'L' and 'R'.  Any
>> approach where struct tracepoint_action is different between IPA and
>> gdbserver is just asking for trouble.
>>
>> Found on s390.  Tested on x86_64, s390, s390x.
>
> Hmm, this is essentially the same as:
>
>   https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html
>
> Right?
>
> Seems that other patch inlines things a bit less though, which offhand
> looks preferable.  WDYT?
>
> Not sure what happened to that series.  I thought most of it (if not all)
> had been approved already.
>
> Thanks,
> Pedro Alves
>

So - what should I do to get these patches pushed?  The original 
developer seems not to be responding, but the work is mostly good, and 
some patches can be pushed unchanged.  What's the procedure for that? 
Do I commit as myself, but with the author's name in ChangeLog?  What if 
the patch needs some fixes?

Marcin Kościelnicki

Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Antoine Tremblay]

On 01/25/2016 07:17 AM, Marcin Kościelnicki wrote:
> On 25/01/16 12:53, Pedro Alves wrote:
>> On 01/23/2016 07:31 PM, Marcin Kościelnicki wrote:
>>> The struct tracepoint_action has an ops field, pointing to
>>> a tracepoint_action_ops structure, containing send and download ops.
>>> However, this field is only present when compiled in gdbserver, and not
>>> when compiled in IPA.  When gdbserver is downloading tracepoint actions
>>> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
>>> its struct tracepoint_action, to get to the part that corresponds to
>>> IPA's struct tracepoint_action.
>>>
>>> Unfortunately, this fails badly on ILP32 platforms where alignof(long
>>> long)
>>> == 8.  Consider struct collect_memory_action layout in gdbserver:
>>>
>>> 0-3: base.ops
>>> 4: base.type
>>> 8-15: addr
>>> 16-23: len
>>> 24-27: basereg
>>> sizeof == 32
>>>
>>> and its layout in IPA:
>>>
>>> 0: base.type
>>> 8-15: addr
>>> 16-23: len
>>> 24-27: basereg
>>> sizeof == 32
>>>
>>> When gdbserver tries to download it to IPA, it skips the first 4 bytes
>>> (base.ops), figuring the rest will match what IPA expects - which is
>>> not true, since addr is aligned to 8 bytes and will be at a different
>>> relative position to base.type.
>>>
>>> The problem went unnoticed on the currently supported platforms, since
>>> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte
>>> alignment for long long.
>>>
>>> There are a few possible ways around this problem.  I decided on
>>> removing
>>> ops altogether, since they can be easily inlined in their (only) places
>>> of use - in fact allowing us share the code between 'L' and 'R'.  Any
>>> approach where struct tracepoint_action is different between IPA and
>>> gdbserver is just asking for trouble.
>>>
>>> Found on s390.  Tested on x86_64, s390, s390x.
>>
>> Hmm, this is essentially the same as:
>>
>>   https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html
>>
>> Right?
>>
>> Seems that other patch inlines things a bit less though, which offhand
>> looks preferable.  WDYT?
>>
>> Not sure what happened to that series.  I thought most of it (if not all)
>> had been approved already.
>>
>> Thanks,
>> Pedro Alves
>>
>
> Huh, I didn't know about that patch series.  Good to know, since I was
> going to try doing ppc tracepoints next, and had no idea that has
> already been attempted.  What happened to that patchset/author?  Kind of
> strange to abandon mostly-finished code when there's a $3k bounty on it.
>
> The other patch looks fine too, I have no preference here.
>
> Marcin Kościelnicki

I had the same problem on ARM.

We could just keep the struct and pack it too, this is common for ABIs 
IMO...

It would avoid this kind of mistake in the future if we were going to 
reintroduce something similar...

I had this patch in the pipeline:

Author: Antoine Tremblay <antoine.tremblay@ericsson.com>
Date:   Thu Jan 28 13:03:10 2016 -0500

     Fix structure alignment problems with IPA protocol objects.

     While testing fast tracepoints on ARM I came across this problem :

     Program received signal SIGSEGV, Segmentation fault.
     0x4010b56c in ?? () from target:/lib/arm-linux-gnueabihf/libc.so.6
     (gdb) FAIL: gdb.trace/ftrace.exp: ond globvar > 7: continue

     The problem is that on GDBServer side struct tracepoint_action is 
aligned
     on 4 bytes, and collect_memory_action is aligned on 8. However in 
the IPA
     they are both aligned on 8 bytes.

     Thus when we copy the data from GDBServer's struct 
tracepoint_action->type
     offset to the ipa the alignement is wrong and the addr,len,basereg 
values
     are wrong also, causing a crash in the inferiror as it tries to read
     memory at a bogus address.

     This patch fixes this issue by setting the tracepoint_action and
     collect_memory_action as packed.

     This patch also fixes this issue in general by setting all IPA protocol
     object structures as packed.

     gdb/gdbserver/ChangeLog:

     	* tracepoint.c (ATTR_PACKED): Moved earlier in the file.
     	(struct tracepoint_action): Use ATTR_PACKED.
     	(struct collect_memory_action): Likewise.
     	(struct collect_registers_action): Likewise.
     	(struct eval_expr_action): Likewise.
     	(struct collect_static_trace_data_action): Likewise.

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 33f6120..35ce2ad 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -467,6 +467,14 @@ static int write_inferior_data_ptr (CORE_ADDR 
where, CORE_ADDR ptr);

  #endif

+#ifndef ATTR_PACKED
+#  if defined(__GNUC__)
+#    define ATTR_PACKED __attribute__ ((packed))
+#  else
+#    define ATTR_PACKED /* nothing */
+#  endif
+#endif
+
  /* Operations on various types of tracepoint actions.  */

  struct tracepoint_action;
@@ -490,7 +498,7 @@ struct tracepoint_action
    const struct tracepoint_action_ops *ops;
  #endif
    char type;
-};
+} ATTR_PACKED;

  /* An 'M' (collect memory) action.  */
  struct collect_memory_action
@@ -500,14 +508,14 @@ struct collect_memory_action
    ULONGEST addr;
    ULONGEST len;
    int32_t basereg;
-};
+} ATTR_PACKED;

  /* An 'R' (collect registers) action.  */

  struct collect_registers_action
  {
    struct tracepoint_action base;
-};
+} ATTR_PACKED;

  /* An 'X' (evaluate expression) action.  */

@@ -516,13 +524,13 @@ struct eval_expr_action
    struct tracepoint_action base;

    struct agent_expr *expr;
-};
+} ATTR_PACKED;

  /* An 'L' (collect static trace data) action.  */
  struct collect_static_trace_data_action
  {
    struct tracepoint_action base;
-};
+} ATTR_PACKED;

  #ifndef IN_PROCESS_AGENT
  static CORE_ADDR
@@ -828,14 +836,6 @@ IP_AGENT_EXPORT_VAR struct trace_state_variable 
*trace_state_variables;
     when the wrapped-around trace frame is the one being discarded; the
     free space ends up in two parts at opposite ends of the buffer.  */

-#ifndef ATTR_PACKED
-#  if defined(__GNUC__)
-#    define ATTR_PACKED __attribute__ ((packed))
-#  else
-#    define ATTR_PACKED /* nothing */
-#  endif
-#endif
-
  /* The data collected at a tracepoint hit.  This object should be as
     small as possible, since there may be a great many of them.  We do
     not need to keep a frame number, because they are all sequential


WDYT ?

Thanks,
Antoine

Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Marcin Kościelnicki]

On 29/01/16 11:14, Marcin Kościelnicki wrote:
> On 25/01/16 12:53, Pedro Alves wrote:
>> On 01/23/2016 07:31 PM, Marcin Kościelnicki wrote:
>>> The struct tracepoint_action has an ops field, pointing to
>>> a tracepoint_action_ops structure, containing send and download ops.
>>> However, this field is only present when compiled in gdbserver, and not
>>> when compiled in IPA.  When gdbserver is downloading tracepoint actions
>>> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
>>> its struct tracepoint_action, to get to the part that corresponds to
>>> IPA's struct tracepoint_action.
>>>
>>> Unfortunately, this fails badly on ILP32 platforms where alignof(long
>>> long)
>>> == 8.  Consider struct collect_memory_action layout in gdbserver:
>>>
>>> 0-3: base.ops
>>> 4: base.type
>>> 8-15: addr
>>> 16-23: len
>>> 24-27: basereg
>>> sizeof == 32
>>>
>>> and its layout in IPA:
>>>
>>> 0: base.type
>>> 8-15: addr
>>> 16-23: len
>>> 24-27: basereg
>>> sizeof == 32
>>>
>>> When gdbserver tries to download it to IPA, it skips the first 4 bytes
>>> (base.ops), figuring the rest will match what IPA expects - which is
>>> not true, since addr is aligned to 8 bytes and will be at a different
>>> relative position to base.type.
>>>
>>> The problem went unnoticed on the currently supported platforms, since
>>> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte
>>> alignment for long long.
>>>
>>> There are a few possible ways around this problem.  I decided on
>>> removing
>>> ops altogether, since they can be easily inlined in their (only) places
>>> of use - in fact allowing us share the code between 'L' and 'R'.  Any
>>> approach where struct tracepoint_action is different between IPA and
>>> gdbserver is just asking for trouble.
>>>
>>> Found on s390.  Tested on x86_64, s390, s390x.
>>
>> Hmm, this is essentially the same as:
>>
>>   https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html
>>
>> Right?
>>
>> Seems that other patch inlines things a bit less though, which offhand
>> looks preferable.  WDYT?
>>
>> Not sure what happened to that series.  I thought most of it (if not all)
>> had been approved already.
>>
>> Thanks,
>> Pedro Alves
>>
>
> So - what should I do to get these patches pushed?  The original
> developer seems not to be responding, but the work is mostly good, and
> some patches can be pushed unchanged.  What's the procedure for that? Do
> I commit as myself, but with the author's name in ChangeLog?  What if
> the patch needs some fixes?
>
> Marcin Kościelnicki
>


Ping.

Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Marcin Kościelnicki]

>
>
>
> On 01/25/2016 07:17 AM, Marcin Kościelnicki wrote: 
> > On 25/01/16 12:53, Pedro Alves wrote: 
> >> On 01/23/2016 07:31 PM, Marcin Kościelnicki wrote: 
> >>> The struct tracepoint_action has an ops field, pointing to 
> >>> a tracepoint_action_ops structure, containing send and download ops. 
> >>> However, this field is only present when compiled in gdbserver, and not 
> >>> when compiled in IPA.  When gdbserver is downloading tracepoint actions 
> >>> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from 
> >>> its struct tracepoint_action, to get to the part that corresponds to 
> >>> IPA's struct tracepoint_action. 
> >>> 
> >>> Unfortunately, this fails badly on ILP32 platforms where alignof(long 
> >>> long) 
> >>> == 8.  Consider struct collect_memory_action layout in gdbserver: 
> >>> 
> >>> 0-3: base.ops 
> >>> 4: base.type 
> >>> 8-15: addr 
> >>> 16-23: len 
> >>> 24-27: basereg 
> >>> sizeof == 32 
> >>> 
> >>> and its layout in IPA: 
> >>> 
> >>> 0: base.type 
> >>> 8-15: addr 
> >>> 16-23: len 
> >>> 24-27: basereg 
> >>> sizeof == 32 
> >>> 
> >>> When gdbserver tries to download it to IPA, it skips the first 4 bytes 
> >>> (base.ops), figuring the rest will match what IPA expects - which is 
> >>> not true, since addr is aligned to 8 bytes and will be at a different 
> >>> relative position to base.type. 
> >>> 
> >>> The problem went unnoticed on the currently supported platforms, since 
> >>> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte 
> >>> alignment for long long. 
> >>> 
> >>> There are a few possible ways around this problem.  I decided on 
> >>> removing 
> >>> ops altogether, since they can be easily inlined in their (only) places 
> >>> of use - in fact allowing us share the code between 'L' and 'R'.  Any 
> >>> approach where struct tracepoint_action is different between IPA and 
> >>> gdbserver is just asking for trouble. 
> >>> 
> >>> Found on s390.  Tested on x86_64, s390, s390x. 
> >> 
> >> Hmm, this is essentially the same as: 
> >> 
> >>   https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html 
> >> 
> >> Right? 
> >> 
> >> Seems that other patch inlines things a bit less though, which offhand 
> >> looks preferable.  WDYT? 
> >> 
> >> Not sure what happened to that series.  I thought most of it (if not all) 
> >> had been approved already. 
> >> 
> >> Thanks, 
> >> Pedro Alves 
> >> 
> > 
> > Huh, I didn't know about that patch series.  Good to know, since I was 
> > going to try doing ppc tracepoints next, and had no idea that has 
> > already been attempted.  What happened to that patchset/author?  Kind of 
> > strange to abandon mostly-finished code when there's a $3k bounty on it. 
> > 
> > The other patch looks fine too, I have no preference here. 
> > 
> > Marcin Kościelnicki 
>
> I had the same problem on ARM. 
>
> We could just keep the struct and pack it too, this is common for ABIs 
> IMO... 
>
> It would avoid this kind of mistake in the future if we were going to 
> reintroduce something similar... 
>
> I had this patch in the pipeline: 
>
> Author: Antoine Tremblay <antoine.tremblay@ericsson.com> 
> Date:   Thu Jan 28 13:03:10 2016 -0500 
>
>      Fix structure alignment problems with IPA protocol objects. 
>
>      While testing fast tracepoints on ARM I came across this problem : 
>
>      Program received signal SIGSEGV, Segmentation fault. 
>      0x4010b56c in ?? () from target:/lib/arm-linux-gnueabihf/libc.so.6 
>      (gdb) FAIL: gdb.trace/ftrace.exp: ond globvar > 7: continue 
>
>      The problem is that on GDBServer side struct tracepoint_action is 
> aligned 
>      on 4 bytes, and collect_memory_action is aligned on 8. However in 
> the IPA 
>      they are both aligned on 8 bytes. 
>
>      Thus when we copy the data from GDBServer's struct 
> tracepoint_action->type 
>      offset to the ipa the alignement is wrong and the addr,len,basereg 
> values 
>      are wrong also, causing a crash in the inferiror as it tries to read 
>      memory at a bogus address. 
>
>      This patch fixes this issue by setting the tracepoint_action and 
>      collect_memory_action as packed. 
>
>      This patch also fixes this issue in general by setting all IPA protocol 
>      object structures as packed. 
>
>      gdb/gdbserver/ChangeLog: 
>
>      * tracepoint.c (ATTR_PACKED): Moved earlier in the file. 
>      (struct tracepoint_action): Use ATTR_PACKED. 
>      (struct collect_memory_action): Likewise. 
>      (struct collect_registers_action): Likewise. 
>      (struct eval_expr_action): Likewise. 
>      (struct collect_static_trace_data_action): Likewise. 
>
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c 
> index 33f6120..35ce2ad 100644 
> --- a/gdb/gdbserver/tracepoint.c 
> +++ b/gdb/gdbserver/tracepoint.c 
> @@ -467,6 +467,14 @@ static int write_inferior_data_ptr (CORE_ADDR 
> where, CORE_ADDR ptr); 
>
>   #endif 
>
> +#ifndef ATTR_PACKED 
> +#  if defined(__GNUC__) 
> +#    define ATTR_PACKED __attribute__ ((packed)) 
> +#  else 
> +#    define ATTR_PACKED /* nothing */ 
> +#  endif 
> +#endif 
> + 
>   /* Operations on various types of tracepoint actions.  */ 
>
>   struct tracepoint_action; 
> @@ -490,7 +498,7 @@ struct tracepoint_action 
>     const struct tracepoint_action_ops *ops; 
>   #endif 
>     char type; 
> -}; 
> +} ATTR_PACKED; 
>
>   /* An 'M' (collect memory) action.  */ 
>   struct collect_memory_action 
> @@ -500,14 +508,14 @@ struct collect_memory_action 
>     ULONGEST addr; 
>     ULONGEST len; 
>     int32_t basereg; 
> -}; 
> +} ATTR_PACKED; 
>
>   /* An 'R' (collect registers) action.  */ 
>
>   struct collect_registers_action 
>   { 
>     struct tracepoint_action base; 
> -}; 
> +} ATTR_PACKED; 
>
>   /* An 'X' (evaluate expression) action.  */ 
>
> @@ -516,13 +524,13 @@ struct eval_expr_action 
>     struct tracepoint_action base; 
>
>     struct agent_expr *expr; 
> -}; 
> +} ATTR_PACKED; 
>
>   /* An 'L' (collect static trace data) action.  */ 
>   struct collect_static_trace_data_action 
>   { 
>     struct tracepoint_action base; 
> -}; 
> +} ATTR_PACKED; 
>
>   #ifndef IN_PROCESS_AGENT 
>   static CORE_ADDR 
> @@ -828,14 +836,6 @@ IP_AGENT_EXPORT_VAR struct trace_state_variable 
> *trace_state_variables; 
>      when the wrapped-around trace frame is the one being discarded; the 
>      free space ends up in two parts at opposite ends of the buffer.  */ 
>
> -#ifndef ATTR_PACKED 
> -#  if defined(__GNUC__) 
> -#    define ATTR_PACKED __attribute__ ((packed)) 
> -#  else 
> -#    define ATTR_PACKED /* nothing */ 
> -#  endif 
> -#endif 
> - 
>   /* The data collected at a tracepoint hit.  This object should be as 
>      small as possible, since there may be a great many of them.  We do 
>      not need to keep a frame number, because they are all sequential 
>
>
> WDYT ? 
>
> Thanks, 
> Antoine 
>
>

Well, that'd work too, but I still think having a struct with different layout between ipa and gdbserver is unnecessary and asking for trouble.  We should be consistent - if the encoding paths go through the ops structure, why does the actual execution of actions use a switch?  Removing the ops, by Wei-cheng's patch or mine, simplifies the code and makes it consistent with the switch-using paths.

Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Antoine Tremblay]

On 02/08/2016 06:55 AM, Marcin Kościelnicki wrote:
>
>>
>>
>>
>> On 01/25/2016 07:17 AM, Marcin Kościelnicki wrote:
>>> On 25/01/16 12:53, Pedro Alves wrote:
>>>> On 01/23/2016 07:31 PM, Marcin Kościelnicki wrote:
>>>>> The struct tracepoint_action has an ops field, pointing to
>>>>> a tracepoint_action_ops structure, containing send and download ops.
>>>>> However, this field is only present when compiled in gdbserver, and not
>>>>> when compiled in IPA.  When gdbserver is downloading tracepoint actions
>>>>> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
>>>>> its struct tracepoint_action, to get to the part that corresponds to
>>>>> IPA's struct tracepoint_action.
>>>>>
>>>>> Unfortunately, this fails badly on ILP32 platforms where alignof(long
>>>>> long)
>>>>> == 8.  Consider struct collect_memory_action layout in gdbserver:
>>>>>
>>>>> 0-3: base.ops
>>>>> 4: base.type
>>>>> 8-15: addr
>>>>> 16-23: len
>>>>> 24-27: basereg
>>>>> sizeof == 32
>>>>>
>>>>> and its layout in IPA:
>>>>>
>>>>> 0: base.type
>>>>> 8-15: addr
>>>>> 16-23: len
>>>>> 24-27: basereg
>>>>> sizeof == 32
>>>>>
>>>>> When gdbserver tries to download it to IPA, it skips the first 4 bytes
>>>>> (base.ops), figuring the rest will match what IPA expects - which is
>>>>> not true, since addr is aligned to 8 bytes and will be at a different
>>>>> relative position to base.type.
>>>>>
>>>>> The problem went unnoticed on the currently supported platforms, since
>>>>> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only 4-byte
>>>>> alignment for long long.
>>>>>
>>>>> There are a few possible ways around this problem.  I decided on
>>>>> removing
>>>>> ops altogether, since they can be easily inlined in their (only) places
>>>>> of use - in fact allowing us share the code between 'L' and 'R'.  Any
>>>>> approach where struct tracepoint_action is different between IPA and
>>>>> gdbserver is just asking for trouble.
>>>>>
>>>>> Found on s390.  Tested on x86_64, s390, s390x.
>>>>
>>>> Hmm, this is essentially the same as:
>>>>
>>>>     https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html
>>>>
>>>> Right?
>>>>
>>>> Seems that other patch inlines things a bit less though, which offhand
>>>> looks preferable.  WDYT?
>>>>
>>>> Not sure what happened to that series.  I thought most of it (if not all)
>>>> had been approved already.
>>>>
>>>> Thanks,
>>>> Pedro Alves
>>>>
>>>
>>> Huh, I didn't know about that patch series.  Good to know, since I was
>>> going to try doing ppc tracepoints next, and had no idea that has
>>> already been attempted.  What happened to that patchset/author?  Kind of
>>> strange to abandon mostly-finished code when there's a $3k bounty on it.
>>>
>>> The other patch looks fine too, I have no preference here.
>>>
>>> Marcin Kościelnicki
>>
>> I had the same problem on ARM.
>>
>> We could just keep the struct and pack it too, this is common for ABIs
>> IMO...
>>
>> It would avoid this kind of mistake in the future if we were going to
>> reintroduce something similar...
>>
>> I had this patch in the pipeline:
>>
>> Author: Antoine Tremblay <antoine.tremblay@ericsson.com>
>> Date:   Thu Jan 28 13:03:10 2016 -0500
>>
>>       Fix structure alignment problems with IPA protocol objects.
>>
>>       While testing fast tracepoints on ARM I came across this problem :
>>
>>       Program received signal SIGSEGV, Segmentation fault.
>>       0x4010b56c in ?? () from target:/lib/arm-linux-gnueabihf/libc.so.6
>>       (gdb) FAIL: gdb.trace/ftrace.exp: ond globvar > 7: continue
>>
>>       The problem is that on GDBServer side struct tracepoint_action is
>> aligned
>>       on 4 bytes, and collect_memory_action is aligned on 8. However in
>> the IPA
>>       they are both aligned on 8 bytes.
>>
>>       Thus when we copy the data from GDBServer's struct
>> tracepoint_action->type
>>       offset to the ipa the alignement is wrong and the addr,len,basereg
>> values
>>       are wrong also, causing a crash in the inferiror as it tries to read
>>       memory at a bogus address.
>>
>>       This patch fixes this issue by setting the tracepoint_action and
>>       collect_memory_action as packed.
>>
>>       This patch also fixes this issue in general by setting all IPA protocol
>>       object structures as packed.
>>
>>       gdb/gdbserver/ChangeLog:
>>
>>       * tracepoint.c (ATTR_PACKED): Moved earlier in the file.
>>       (struct tracepoint_action): Use ATTR_PACKED.
>>       (struct collect_memory_action): Likewise.
>>       (struct collect_registers_action): Likewise.
>>       (struct eval_expr_action): Likewise.
>>       (struct collect_static_trace_data_action): Likewise.
>>
>> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
>> index 33f6120..35ce2ad 100644
>> --- a/gdb/gdbserver/tracepoint.c
>> +++ b/gdb/gdbserver/tracepoint.c
>> @@ -467,6 +467,14 @@ static int write_inferior_data_ptr (CORE_ADDR
>> where, CORE_ADDR ptr);
>>
>>    #endif
>>
>> +#ifndef ATTR_PACKED
>> +#  if defined(__GNUC__)
>> +#    define ATTR_PACKED __attribute__ ((packed))
>> +#  else
>> +#    define ATTR_PACKED /* nothing */
>> +#  endif
>> +#endif
>> +
>>    /* Operations on various types of tracepoint actions.  */
>>
>>    struct tracepoint_action;
>> @@ -490,7 +498,7 @@ struct tracepoint_action
>>      const struct tracepoint_action_ops *ops;
>>    #endif
>>      char type;
>> -};
>> +} ATTR_PACKED;
>>
>>    /* An 'M' (collect memory) action.  */
>>    struct collect_memory_action
>> @@ -500,14 +508,14 @@ struct collect_memory_action
>>      ULONGEST addr;
>>      ULONGEST len;
>>      int32_t basereg;
>> -};
>> +} ATTR_PACKED;
>>
>>    /* An 'R' (collect registers) action.  */
>>
>>    struct collect_registers_action
>>    {
>>      struct tracepoint_action base;
>> -};
>> +} ATTR_PACKED;
>>
>>    /* An 'X' (evaluate expression) action.  */
>>
>> @@ -516,13 +524,13 @@ struct eval_expr_action
>>      struct tracepoint_action base;
>>
>>      struct agent_expr *expr;
>> -};
>> +} ATTR_PACKED;
>>
>>    /* An 'L' (collect static trace data) action.  */
>>    struct collect_static_trace_data_action
>>    {
>>      struct tracepoint_action base;
>> -};
>> +} ATTR_PACKED;
>>
>>    #ifndef IN_PROCESS_AGENT
>>    static CORE_ADDR
>> @@ -828,14 +836,6 @@ IP_AGENT_EXPORT_VAR struct trace_state_variable
>> *trace_state_variables;
>>       when the wrapped-around trace frame is the one being discarded; the
>>       free space ends up in two parts at opposite ends of the buffer.  */
>>
>> -#ifndef ATTR_PACKED
>> -#  if defined(__GNUC__)
>> -#    define ATTR_PACKED __attribute__ ((packed))
>> -#  else
>> -#    define ATTR_PACKED /* nothing */
>> -#  endif
>> -#endif
>> -
>>    /* The data collected at a tracepoint hit.  This object should be as
>>       small as possible, since there may be a great many of them.  We do
>>       not need to keep a frame number, because they are all sequential
>>
>>
>> WDYT ?
>>
>> Thanks,
>> Antoine
>>
>>
>
> Well, that'd work too, but I still think having a struct with different layout between ipa and gdbserver is unnecessary and asking for trouble.  We should be consistent - if the encoding paths go through the ops structure, why does the actual execution of actions use a switch?  Removing the ops, by Wei-cheng's patch or mine, simplifies the code and makes it consistent with the switch-using paths.
>

Yes I agree about removing the ops it does make it cleaner. I'll still 
post this patch after just to avoid repeating this problem if we ever 
need to have diverging structs in the future and forget about the issue.

Regards,
Antoine

Re: [PATCH] gdb.trace: Remove struct tracepoint_action_ops.

[Antoine Tremblay]

On 02/05/2016 08:05 PM, Marcin Kościelnicki wrote:
> On 29/01/16 11:14, Marcin Kościelnicki wrote:
>> On 25/01/16 12:53, Pedro Alves wrote:
>>> On 01/23/2016 07:31 PM, Marcin Kościelnicki wrote:
>>>> The struct tracepoint_action has an ops field, pointing to
>>>> a tracepoint_action_ops structure, containing send and download ops.
>>>> However, this field is only present when compiled in gdbserver, and not
>>>> when compiled in IPA.  When gdbserver is downloading tracepoint actions
>>>> to IPA, it skips offsetof(struct tracepoint_action, type) bytes from
>>>> its struct tracepoint_action, to get to the part that corresponds to
>>>> IPA's struct tracepoint_action.
>>>>
>>>> Unfortunately, this fails badly on ILP32 platforms where alignof(long
>>>> long)
>>>> == 8.  Consider struct collect_memory_action layout in gdbserver:
>>>>
>>>> 0-3: base.ops
>>>> 4: base.type
>>>> 8-15: addr
>>>> 16-23: len
>>>> 24-27: basereg
>>>> sizeof == 32
>>>>
>>>> and its layout in IPA:
>>>>
>>>> 0: base.type
>>>> 8-15: addr
>>>> 16-23: len
>>>> 24-27: basereg
>>>> sizeof == 32
>>>>
>>>> When gdbserver tries to download it to IPA, it skips the first 4 bytes
>>>> (base.ops), figuring the rest will match what IPA expects - which is
>>>> not true, since addr is aligned to 8 bytes and will be at a different
>>>> relative position to base.type.
>>>>
>>>> The problem went unnoticed on the currently supported platforms, since
>>>> aarch64 and x86_64 have ops aligned to 8 bytes, and i386 has only
>>>> 4-byte
>>>> alignment for long long.
>>>>
>>>> There are a few possible ways around this problem.  I decided on
>>>> removing
>>>> ops altogether, since they can be easily inlined in their (only) places
>>>> of use - in fact allowing us share the code between 'L' and 'R'.  Any
>>>> approach where struct tracepoint_action is different between IPA and
>>>> gdbserver is just asking for trouble.
>>>>
>>>> Found on s390.  Tested on x86_64, s390, s390x.
>>>
>>> Hmm, this is essentially the same as:
>>>
>>>   https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html
>>>
>>> Right?
>>>
>>> Seems that other patch inlines things a bit less though, which offhand
>>> looks preferable.  WDYT?
>>>
>>> Not sure what happened to that series.  I thought most of it (if not
>>> all)
>>> had been approved already.
>>>
>>> Thanks,
>>> Pedro Alves
>>>
>>
>> So - what should I do to get these patches pushed?  The original
>> developer seems not to be responding, but the work is mostly good, and
>> some patches can be pushed unchanged.  What's the procedure for that? Do
>> I commit as myself, but with the author's name in ChangeLog?  What if
>> the patch needs some fixes?
>>
>> Marcin Kościelnicki
>>
>
>
> Ping.

I have a similar situation and was advised that I could commit someone 
else patch if he's in the Commit After Approval list, which Wei-cheng is.

You can keep the author of the patch to be the original author in the 
commit and have his name in the ChangeLog. (git commit -a 
--author=<orignal> if you need too)

If the patch needs some changes and you're willing to do them, you can 
do the same procedure as above and add your name to the ChangeLog.
Providing you've submitted the changes to the list and that they are 
approved of course...

Regards,
Antoine

[PATCH] gdbserver: Remove tracepoint_action ops.

[Marcin Kościelnicki]

This patch removes 'ops' in tracepoint, and uses helper functions to
call action handler instead.

The object layout of tracepoint_action may differ in gdbserver and
inferior depend on the alignment rule of target ABI, so gdbserver cannot
simply copy the object from its memory to inferior memory.

For example,

  struct collect_memory_action
  {
    struct tracepoint_action base;
    {
      #ifndef IN_PROCESS_AGENT
      const struct tracepoint_action_ops *ops;
      #if
  -   char type;
  | }
  | ULONGEST addr;
  | ULONGEST len;
  - int32_t basereg;
  };

and on PowerPC,

     Wihtout ops           with ops
      0   1   2   3         0   1   2   3
   0 |type| PADDING...    0 |ops-------------|
   4 .................    4 |type|PADDING....|
   8 |addr------------    8 |addr-------------
   c ----------------|    c -----------------|
  10 |len-------------   10 |len--------------
  14 ----------------|   14 -----------------|
  18 |basereg--------|   18 |basereg---------|

so we cannot directly copy the object.

In this patch, 'ops' is removed in order to make the objects identical.

gdbserver/ChangeLog:

2015-06-27  Wei-cheng Wang  <cole945@gmail.com>
2016-02-11  Marcin Kościelnicki  <koriakin@0x04.net>

	* tracepoint.c (struct tracepoint_action_ops): Removed.
	(struct tracepoint_action): Remove ops.
	(m_tracepoint_action_download, r_tracepoint_action_download,
	x_tracepoint_action_download, l_tracepoint_action_download): Adjust
	size and offset accordingly.
	(m_tracepoint_action_ops, r_tracepoint_action_ops,
	x_tracepoint_action_ops, l_tracepoint_action_ops): Delete
	(tracepoint_action_send, tracepoint_action_download): New functions.
	Helpers for tracetion action handlers.
	(add_tracepoint_action): Remove setup actions ops.
	(download_tracepoint_1, tracepoint_send_agent): Call helper functions.
---
Hey,

This is a fixed-up version of Wei-cheng's patch at
https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html .

This version, in addition to fixing a few trivial conflicts, also removes
the definition of struct tracepoint_action_ops, which is now unused, but
was left in by the original patch.

I'm also not sure if I got the changelog format right...

OK to push?

 gdb/gdbserver/tracepoint.c | 119 +++++++++++++++++++--------------------------
 1 file changed, 50 insertions(+), 69 deletions(-)

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 0671999..ca77487 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -462,28 +462,10 @@ static int write_inferior_data_ptr (CORE_ADDR where, CORE_ADDR ptr);
 
 #endif
 
-/* Operations on various types of tracepoint actions.  */
-
-struct tracepoint_action;
-
-struct tracepoint_action_ops
-{
-  /* Download tracepoint action ACTION to IPA.  Return the address of action
-     in IPA/inferior.  */
-  CORE_ADDR (*download) (const struct tracepoint_action *action);
-
-  /* Send ACTION to agent via command buffer started from BUFFER.  Return
-     updated head of command buffer.  */
-  char* (*send) (char *buffer, const struct tracepoint_action *action);
-};
-
 /* Base action.  Concrete actions inherit this.  */
 
 struct tracepoint_action
 {
-#ifndef IN_PROCESS_AGENT
-  const struct tracepoint_action_ops *ops;
-#endif
   char type;
 };
 
@@ -523,12 +505,10 @@ struct collect_static_trace_data_action
 static CORE_ADDR
 m_tracepoint_action_download (const struct tracepoint_action *action)
 {
-  int size_in_ipa = (sizeof (struct collect_memory_action)
-		     - offsetof (struct tracepoint_action, type));
-  CORE_ADDR ipa_action = target_malloc (size_in_ipa);
+  CORE_ADDR ipa_action = target_malloc (sizeof (struct collect_memory_action));
 
-  write_inferior_memory (ipa_action, (unsigned char *) &action->type,
-			 size_in_ipa);
+  write_inferior_memory (ipa_action, (unsigned char *) action,
+			 sizeof (struct collect_memory_action));
 
   return ipa_action;
 }
@@ -545,21 +525,13 @@ m_tracepoint_action_send (char *buffer, const struct tracepoint_action *action)
   return buffer;
 }
 
-static const struct tracepoint_action_ops m_tracepoint_action_ops =
-{
-  m_tracepoint_action_download,
-  m_tracepoint_action_send,
-};
-
 static CORE_ADDR
 r_tracepoint_action_download (const struct tracepoint_action *action)
 {
-  int size_in_ipa = (sizeof (struct collect_registers_action)
-		     - offsetof (struct tracepoint_action, type));
-  CORE_ADDR ipa_action  = target_malloc (size_in_ipa);
+  CORE_ADDR ipa_action  = target_malloc (sizeof (struct collect_registers_action));
 
-  write_inferior_memory (ipa_action, (unsigned char *) &action->type,
-			size_in_ipa);
+  write_inferior_memory (ipa_action, (unsigned char *) action,
+			 sizeof (struct collect_registers_action));
 
   return ipa_action;
 }
@@ -570,27 +542,19 @@ r_tracepoint_action_send (char *buffer, const struct tracepoint_action *action)
   return buffer;
 }
 
-static const struct tracepoint_action_ops r_tracepoint_action_ops =
-{
-  r_tracepoint_action_download,
-  r_tracepoint_action_send,
-};
-
 static CORE_ADDR download_agent_expr (struct agent_expr *expr);
 
 static CORE_ADDR
 x_tracepoint_action_download (const struct tracepoint_action *action)
 {
-  int size_in_ipa = (sizeof (struct eval_expr_action)
-		     - offsetof (struct tracepoint_action, type));
-  CORE_ADDR ipa_action = target_malloc (size_in_ipa);
+  CORE_ADDR ipa_action = target_malloc (sizeof (struct eval_expr_action));
   CORE_ADDR expr;
 
-  write_inferior_memory (ipa_action, (unsigned char *) &action->type,
-			 size_in_ipa);
-  expr = download_agent_expr (((struct eval_expr_action *)action)->expr);
-  write_inferior_data_ptr (ipa_action + offsetof (struct eval_expr_action, expr)
-			   - offsetof (struct tracepoint_action, type),
+  write_inferior_memory (ipa_action, (unsigned char *) action,
+			 sizeof (struct eval_expr_action));
+  expr = download_agent_expr (((struct eval_expr_action *) action)->expr);
+  write_inferior_data_ptr (ipa_action
+			   + offsetof (struct eval_expr_action, expr),
 			   expr);
 
   return ipa_action;
@@ -628,21 +592,14 @@ x_tracepoint_action_send ( char *buffer, const struct tracepoint_action *action)
   return agent_expr_send (buffer, eaction->expr);
 }
 
-static const struct tracepoint_action_ops x_tracepoint_action_ops =
-{
-  x_tracepoint_action_download,
-  x_tracepoint_action_send,
-};
-
 static CORE_ADDR
 l_tracepoint_action_download (const struct tracepoint_action *action)
 {
-  int size_in_ipa = (sizeof (struct collect_static_trace_data_action)
-		     - offsetof (struct tracepoint_action, type));
-  CORE_ADDR ipa_action = target_malloc (size_in_ipa);
+  CORE_ADDR ipa_action =
+    target_malloc (sizeof (struct collect_static_trace_data_action));
 
-  write_inferior_memory (ipa_action, (unsigned char *) &action->type,
-			 size_in_ipa);
+  write_inferior_memory (ipa_action, (unsigned char *) action,
+			 sizeof (struct collect_static_trace_data_action));
 
   return ipa_action;
 }
@@ -653,11 +610,39 @@ l_tracepoint_action_send (char *buffer, const struct tracepoint_action *action)
   return buffer;
 }
 
-static const struct tracepoint_action_ops l_tracepoint_action_ops =
+static char *
+tracepoint_action_send (char *buffer, const struct tracepoint_action *action)
+{
+  switch (action->type)
+    {
+    case 'M':
+      return m_tracepoint_action_send (buffer, action);
+    case 'R':
+      return r_tracepoint_action_send (buffer, action);
+    case 'X':
+      return x_tracepoint_action_send (buffer, action);
+    case 'L':
+      return l_tracepoint_action_send (buffer, action);
+    }
+  error ("Unknown trace action '%c'.", action->type);
+}
+
+static CORE_ADDR
+tracepoint_action_download (const struct tracepoint_action *action)
 {
-  l_tracepoint_action_download,
-  l_tracepoint_action_send,
-};
+  switch (action->type)
+    {
+    case 'M':
+      return m_tracepoint_action_download (action);
+    case 'R':
+      return r_tracepoint_action_download (action);
+    case 'X':
+      return x_tracepoint_action_download (action);
+    case 'L':
+      return l_tracepoint_action_download (action);
+    }
+  error ("Unknown trace action '%c'.", action->type);
+}
 #endif
 
 /* This structure describes a piece of the source-level definition of
@@ -1944,7 +1929,6 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
 	    int is_neg;
 
 	    maction->base.type = *act;
-	    maction->base.ops = &m_tracepoint_action_ops;
 	    action = &maction->base;
 
 	    ++act;
@@ -1970,7 +1954,6 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
 	      XNEW (struct collect_registers_action);
 
 	    raction->base.type = *act;
-	    raction->base.ops = &r_tracepoint_action_ops;
 	    action = &raction->base;
 
 	    trace_debug ("Want to collect registers");
@@ -1986,7 +1969,6 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
 	      XNEW (struct collect_static_trace_data_action);
 
 	    raction->base.type = *act;
-	    raction->base.ops = &l_tracepoint_action_ops;
 	    action = &raction->base;
 
 	    trace_debug ("Want to collect static trace data");
@@ -2002,7 +1984,6 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
 	    struct eval_expr_action *xaction = XNEW (struct eval_expr_action);
 
 	    xaction->base.type = *act;
-	    xaction->base.ops = &x_tracepoint_action_ops;
 	    action = &xaction->base;
 
 	    trace_debug ("Want to evaluate expression");
@@ -6066,7 +6047,7 @@ download_tracepoint_1 (struct tracepoint *tpoint)
       for (i = 0; i < tpoint->numactions; i++)
 	{
 	  struct tracepoint_action *action = tpoint->actions[i];
-	  CORE_ADDR ipa_action = action->ops->download (action);
+	  CORE_ADDR ipa_action = tracepoint_action_download (action);
 
 	  if (ipa_action != 0)
 	    write_inferior_data_ptr
@@ -6116,7 +6097,7 @@ tracepoint_send_agent (struct tracepoint *tpoint)
       struct tracepoint_action *action = tpoint->actions[i];
 
       p[0] = action->type;
-      p = action->ops->send (&p[1], action);
+      p = tracepoint_action_send (&p[1], action);
     }
 
   get_jump_space_head ();
-- 
2.7.0

Re: [PATCH] gdbserver: Remove tracepoint_action ops.

[Pedro Alves]

On 02/11/2016 04:30 PM, Marcin Kościelnicki wrote:

> 2015-06-27  Wei-cheng Wang  <cole945@gmail.com>
> 2016-02-11  Marcin Kościelnicki  <koriakin@0x04.net>

Only one date, and then align names:

2015-06-27  Wei-cheng Wang  <cole945@gmail.com>
	    Marcin Kościelnicki  <koriakin@0x04.net>

See previous examples:

$ grep -P "\t.*@"  gdb/ChangeLog-2015 -C 3 | less


> 
> 	* tracepoint.c (struct tracepoint_action_ops): Removed.

"Remove."

> 	(struct tracepoint_action): Remove ops.
> 	(m_tracepoint_action_download, r_tracepoint_action_download,
> 	x_tracepoint_action_download, l_tracepoint_action_download): Adjust
> 	size and offset accordingly.

Split lines with close/open ()s:

	(m_tracepoint_action_download, r_tracepoint_action_download)
	(x_tracepoint_action_download, l_tracepoint_action_download): Adjust
	size and offset accordingly.


> 	(m_tracepoint_action_ops, r_tracepoint_action_ops,
> 	x_tracepoint_action_ops, l_tracepoint_action_ops): Delete

Ditto.  Period at end:

	(m_tracepoint_action_ops, r_tracepoint_action_ops)
	(x_tracepoint_action_ops, l_tracepoint_action_ops): Delete.


> 	(tracepoint_action_send, tracepoint_action_download): New functions.
> 	Helpers for tracetion action handlers.

Typo "tracetion".


> 	(add_tracepoint_action): Remove setup actions ops.
> 	(download_tracepoint_1, tracepoint_send_agent): Call helper functions.
> ---
> Hey,
> 
> This is a fixed-up version of Wei-cheng's patch at
> https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html .
> 
> This version, in addition to fixing a few trivial conflicts, also removes
> the definition of struct tracepoint_action_ops, which is now unused, but
> was left in by the original patch.

Thanks.

> 
> I'm also not sure if I got the changelog format right...
> 
> OK to push?
> 

OK with fixes below.

> -
>  static CORE_ADDR
>  r_tracepoint_action_download (const struct tracepoint_action *action)
>  {
> -  int size_in_ipa = (sizeof (struct collect_registers_action)
> -		     - offsetof (struct tracepoint_action, type));
> -  CORE_ADDR ipa_action  = target_malloc (size_in_ipa);
> +  CORE_ADDR ipa_action  = target_malloc (sizeof (struct collect_registers_action));
>  

Remove spurious double-space before '=' while at it.


> @@ -628,21 +592,14 @@ x_tracepoint_action_send ( char *buffer, const struct tracepoint_action *action)
>    return agent_expr_send (buffer, eaction->expr);
>  }
>  
> -static const struct tracepoint_action_ops x_tracepoint_action_ops =
> -{
> -  x_tracepoint_action_download,
> -  x_tracepoint_action_send,
> -};
> -
>  static CORE_ADDR
>  l_tracepoint_action_download (const struct tracepoint_action *action)
>  {
> -  int size_in_ipa = (sizeof (struct collect_static_trace_data_action)
> -		     - offsetof (struct tracepoint_action, type));
> -  CORE_ADDR ipa_action = target_malloc (size_in_ipa);
> +  CORE_ADDR ipa_action =
> +    target_malloc (sizeof (struct collect_static_trace_data_action));

The '=' goes at the beginning of the next line.

OK with those fixed.

Thanks,
Pedro Alves

Re: [PATCH] gdbserver: Remove tracepoint_action ops.

[Marcin Kościelnicki]

On 11/02/16 17:50, Pedro Alves wrote:
> On 02/11/2016 04:30 PM, Marcin Kościelnicki wrote:
>
>> 2015-06-27  Wei-cheng Wang  <cole945@gmail.com>
>> 2016-02-11  Marcin Kościelnicki  <koriakin@0x04.net>
>
> Only one date, and then align names:
>
> 2015-06-27  Wei-cheng Wang  <cole945@gmail.com>
> 	    Marcin Kościelnicki  <koriakin@0x04.net>
>
> See previous examples:
>
> $ grep -P "\t.*@"  gdb/ChangeLog-2015 -C 3 | less
>
>
>>
>> 	* tracepoint.c (struct tracepoint_action_ops): Removed.
>
> "Remove."
>
>> 	(struct tracepoint_action): Remove ops.
>> 	(m_tracepoint_action_download, r_tracepoint_action_download,
>> 	x_tracepoint_action_download, l_tracepoint_action_download): Adjust
>> 	size and offset accordingly.
>
> Split lines with close/open ()s:
>
> 	(m_tracepoint_action_download, r_tracepoint_action_download)
> 	(x_tracepoint_action_download, l_tracepoint_action_download): Adjust
> 	size and offset accordingly.
>
>
>> 	(m_tracepoint_action_ops, r_tracepoint_action_ops,
>> 	x_tracepoint_action_ops, l_tracepoint_action_ops): Delete
>
> Ditto.  Period at end:
>
> 	(m_tracepoint_action_ops, r_tracepoint_action_ops)
> 	(x_tracepoint_action_ops, l_tracepoint_action_ops): Delete.
>
>
>> 	(tracepoint_action_send, tracepoint_action_download): New functions.
>> 	Helpers for tracetion action handlers.
>
> Typo "tracetion".
>
>
>> 	(add_tracepoint_action): Remove setup actions ops.
>> 	(download_tracepoint_1, tracepoint_send_agent): Call helper functions.
>> ---
>> Hey,
>>
>> This is a fixed-up version of Wei-cheng's patch at
>> https://sourceware.org/ml/gdb-patches/2015-03/msg00995.html .
>>
>> This version, in addition to fixing a few trivial conflicts, also removes
>> the definition of struct tracepoint_action_ops, which is now unused, but
>> was left in by the original patch.
>
> Thanks.
>
>>
>> I'm also not sure if I got the changelog format right...
>>
>> OK to push?
>>
>
> OK with fixes below.
>
>> -
>>   static CORE_ADDR
>>   r_tracepoint_action_download (const struct tracepoint_action *action)
>>   {
>> -  int size_in_ipa = (sizeof (struct collect_registers_action)
>> -		     - offsetof (struct tracepoint_action, type));
>> -  CORE_ADDR ipa_action  = target_malloc (size_in_ipa);
>> +  CORE_ADDR ipa_action  = target_malloc (sizeof (struct collect_registers_action));
>>
>
> Remove spurious double-space before '=' while at it.
>
>
>> @@ -628,21 +592,14 @@ x_tracepoint_action_send ( char *buffer, const struct tracepoint_action *action)
>>     return agent_expr_send (buffer, eaction->expr);
>>   }
>>
>> -static const struct tracepoint_action_ops x_tracepoint_action_ops =
>> -{
>> -  x_tracepoint_action_download,
>> -  x_tracepoint_action_send,
>> -};
>> -
>>   static CORE_ADDR
>>   l_tracepoint_action_download (const struct tracepoint_action *action)
>>   {
>> -  int size_in_ipa = (sizeof (struct collect_static_trace_data_action)
>> -		     - offsetof (struct tracepoint_action, type));
>> -  CORE_ADDR ipa_action = target_malloc (size_in_ipa);
>> +  CORE_ADDR ipa_action =
>> +    target_malloc (sizeof (struct collect_static_trace_data_action));
>
> The '=' goes at the beginning of the next line.
>
> OK with those fixed.
>
> Thanks,
> Pedro Alves
>

Thanks, fixed and pushed.