From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 49376 invoked by alias); 23 Jan 2016 19:32:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 49360 invoked by uid 89); 23 Jan 2016 19:32:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=downloading, Download, ACTION, 1,23 X-HELO: xyzzy.0x04.net Received: from xyzzy.0x04.net (HELO xyzzy.0x04.net) (109.74.193.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 23 Jan 2016 19:32:01 +0000 Received: from hogfather.0x04.net (89-65-66-135.dynamic.chello.pl [89.65.66.135]) by xyzzy.0x04.net (Postfix) with ESMTPS id A7D7040C53 for ; Sat, 23 Jan 2016 20:32:42 +0100 (CET) Received: by hogfather.0x04.net (Postfix, from userid 1000) id 4B848580092; Sat, 23 Jan 2016 20:31:58 +0100 (CET) From: =?UTF-8?q?Marcin=20Ko=C5=9Bcielnicki?= To: gdb-patches@sourceware.org Cc: =?UTF-8?q?Marcin=20Ko=C5=9Bcielnicki?= Subject: [PATCH] gdb.trace: Remove struct tracepoint_action_ops. Date: Sat, 23 Jan 2016 19:32:00 -0000 Message-Id: <1453577516-19252-1-git-send-email-koriakin@0x04.net> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00597.txt.bz2 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 + * 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 + * gdb.trace/pending.exp: Fix expected message on continue. 2016-01-22 Marcin Kościelnicki 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