gdbserver: Handle 'v' packet while processing qSymbol.
Commit Message
On powerpc64, qSymbol query may require gdb to read a function
descriptor, sending a vFile packet to gdbserver. Thus, we need
to handle 'v' packet in look_up_one_symbol.
vFile replies may be quite long, and require reallocating own_buf.
Since handle_v_requests assumes the buffer is the static global own_buf
from server.c and reallocates it, we need to make own_buf global and
use it from look_up_one_symbol instead of using our own auto variable.
I've also done the same change in relocate_instruction, just in case.
On gdb side, in remote_check_symbols, rs->buf may be clobbered by vFile
handling, yet we need its contents for the reply (the symbol name is
stored there). Allocate a new buffer instead.
This broke fast tracepoints on powerpc64, due to errors in reading IPA
symbols.
gdb/ChangeLog:
* remote.c (xfreep): New function.
(remote_check_symbols): Allocate own buffer for reply.
gdbserver/ChangeLog:
* remote-utils.c (look_up_one_symbol): Remove own_buf, handle 'v'
packets.
(relocate_instruction): Remove own_buf.
* server.c (own_buf): Make global.
(handle_v_requests): Make global.
* server.h (own_buf): New declaration.
(handle_v_requests): New prototype.
---
gdb/ChangeLog | 5 +++++
gdb/gdbserver/ChangeLog | 10 ++++++++++
gdb/gdbserver/remote-utils.c | 41 +++++++++++++++++++++++++++--------------
gdb/gdbserver/server.c | 4 ++--
gdb/gdbserver/server.h | 4 ++++
gdb/remote.c | 22 +++++++++++++++++-----
6 files changed, 65 insertions(+), 21 deletions(-)
Comments
Ping.
On 12/03/16 16:02, Marcin Kościelnicki wrote:
> On powerpc64, qSymbol query may require gdb to read a function
> descriptor, sending a vFile packet to gdbserver. Thus, we need
> to handle 'v' packet in look_up_one_symbol.
>
> vFile replies may be quite long, and require reallocating own_buf.
> Since handle_v_requests assumes the buffer is the static global own_buf
> from server.c and reallocates it, we need to make own_buf global and
> use it from look_up_one_symbol instead of using our own auto variable.
> I've also done the same change in relocate_instruction, just in case.
>
> On gdb side, in remote_check_symbols, rs->buf may be clobbered by vFile
> handling, yet we need its contents for the reply (the symbol name is
> stored there). Allocate a new buffer instead.
>
> This broke fast tracepoints on powerpc64, due to errors in reading IPA
> symbols.
>
> gdb/ChangeLog:
>
> * remote.c (xfreep): New function.
> (remote_check_symbols): Allocate own buffer for reply.
>
> gdbserver/ChangeLog:
>
> * remote-utils.c (look_up_one_symbol): Remove own_buf, handle 'v'
> packets.
> (relocate_instruction): Remove own_buf.
> * server.c (own_buf): Make global.
> (handle_v_requests): Make global.
> * server.h (own_buf): New declaration.
> (handle_v_requests): New prototype.
> ---
>
> gdb/ChangeLog | 5 +++++
> gdb/gdbserver/ChangeLog | 10 ++++++++++
> gdb/gdbserver/remote-utils.c | 41 +++++++++++++++++++++++++++--------------
> gdb/gdbserver/server.c | 4 ++--
> gdb/gdbserver/server.h | 4 ++++
> gdb/remote.c | 22 +++++++++++++++++-----
> 6 files changed, 65 insertions(+), 21 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index febe960..41f826e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-03-12 Marcin Kościelnicki <koriakin@0x04.net>
> +
> + * remote.c (xfreep): New function.
> + (remote_check_symbols): Allocate own buffer for reply.
> +
> 2016-03-11 Marcin Kościelnicki <koriakin@0x04.net>
>
> * s390-linux-tdep.c (s390_ax_pseudo_register_collect): New function.
> diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
> index dda87b0..7ce2690 100644
> --- a/gdb/gdbserver/ChangeLog
> +++ b/gdb/gdbserver/ChangeLog
> @@ -1,3 +1,13 @@
> +2016-03-12 Marcin Kościelnicki <koriakin@0x04.net>
> +
> + * remote-utils.c (look_up_one_symbol): Remove own_buf, handle 'v'
> + packets.
> + (relocate_instruction): Remove own_buf.
> + * server.c (own_buf): Make global.
> + (handle_v_requests): Make global.
> + * server.h (own_buf): New declaration.
> + (handle_v_requests): New prototype.
> +
> 2016-03-09 Marcin Kościelnicki <koriakin@0x04.net>
>
> * linux-ppc-low.c (ppc_supports_tracepoints): New function.
> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
> index e751473..24f92c2 100644
> --- a/gdb/gdbserver/remote-utils.c
> +++ b/gdb/gdbserver/remote-utils.c
> @@ -1462,7 +1462,7 @@ clear_symbol_cache (struct sym_cache **symcache_p)
> int
> look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
> {
> - char own_buf[266], *p, *q;
> + char *p, *q;
> int len;
> struct sym_cache *sym;
> struct process_info *proc;
> @@ -1499,21 +1499,35 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
> main loop. For now, this is an adequate approximation; allow
> GDB to read from memory while it figures out the address of the
> symbol. */
> - while (own_buf[0] == 'm')
> + while (1)
> {
> - CORE_ADDR mem_addr;
> - unsigned char *mem_buf;
> - unsigned int mem_len;
> + if (own_buf[0] == 'm')
> + {
> + CORE_ADDR mem_addr;
> + unsigned char *mem_buf;
> + unsigned int mem_len;
>
> - decode_m_packet (&own_buf[1], &mem_addr, &mem_len);
> - mem_buf = (unsigned char *) xmalloc (mem_len);
> - if (read_inferior_memory (mem_addr, mem_buf, mem_len) == 0)
> - bin2hex (mem_buf, own_buf, mem_len);
> + decode_m_packet (&own_buf[1], &mem_addr, &mem_len);
> + mem_buf = (unsigned char *) xmalloc (mem_len);
> + if (read_inferior_memory (mem_addr, mem_buf, mem_len) == 0)
> + bin2hex (mem_buf, own_buf, mem_len);
> + else
> + write_enn (own_buf);
> + free (mem_buf);
> + if (putpkt (own_buf) < 0)
> + return -1;
> + }
> + else if (own_buf[0] == 'v')
> + {
> + int new_len = -1;
> + handle_v_requests (own_buf, len, &new_len);
> + if (new_len != -1)
> + putpkt_binary (own_buf, new_len);
> + else
> + putpkt (own_buf);
> + }
> else
> - write_enn (own_buf);
> - free (mem_buf);
> - if (putpkt (own_buf) < 0)
> - return -1;
> + break;
> len = getpkt (own_buf);
> if (len < 0)
> return -1;
> @@ -1561,7 +1575,6 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
> int
> relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
> {
> - char own_buf[266];
> int len;
> ULONGEST written = 0;
>
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index ef715e7..9c50929 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -119,7 +119,7 @@ int disable_packet_qfThreadInfo;
> static struct target_waitstatus last_status;
> static ptid_t last_ptid;
>
> -static char *own_buf;
> +char *own_buf;
> static unsigned char *mem_buf;
>
> /* A sub-class of 'struct notif_event' for stop, holding information
> @@ -2935,7 +2935,7 @@ handle_v_kill (char *own_buf)
> }
>
> /* Handle all of the extended 'v' packets. */
> -static void
> +void
> handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
> {
> if (!disable_packet_vCont)
> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index 3d78fb3..51b2191 100644
> --- a/gdb/gdbserver/server.h
> +++ b/gdb/gdbserver/server.h
> @@ -82,6 +82,8 @@ extern int disable_packet_Tthread;
> extern int disable_packet_qC;
> extern int disable_packet_qfThreadInfo;
>
> +extern char *own_buf;
> +
> extern int run_once;
> extern int multi_process;
> extern int report_fork_events;
> @@ -113,6 +115,8 @@ typedef int gdb_fildes_t;
> #include "event-loop.h"
>
> /* Functions from server.c. */
> +extern void handle_v_requests (char *own_buf, int packet_len,
> + int *new_packet_len);
> extern int handle_serial_event (int err, gdb_client_data client_data);
> extern int handle_target_event (int err, gdb_client_data client_data);
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index f09a06e..5f3d9fd 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -4321,6 +4321,16 @@ init_all_packet_configs (void)
> }
> }
>
> +/* Helper function for cleanup below. Argument is a pointer to a void *
> + pointing to a buffer that needs to be freed. */
> +
> +static void
> +xfreep (void *arg)
> +{
> + void **pptr = (void **) arg;
> + xfree (*pptr);
> +}
> +
> /* Symbol look-up. */
>
> static void
> @@ -4329,6 +4339,7 @@ remote_check_symbols (void)
> struct remote_state *rs = get_remote_state ();
> char *msg, *reply, *tmp;
> int end;
> + long reply_size;
> struct cleanup *old_chain;
>
> /* The remote side has no concept of inferiors that aren't running
> @@ -4350,13 +4361,15 @@ remote_check_symbols (void)
> because we need both at the same time. */
> msg = (char *) xmalloc (get_remote_packet_size ());
> old_chain = make_cleanup (xfree, msg);
> + reply = (char *) xmalloc (get_remote_packet_size ());
> + make_cleanup (xfreep, &reply);
> + reply_size = get_remote_packet_size ();
>
> /* Invite target to request symbol lookups. */
>
> putpkt ("qSymbol::");
> - getpkt (&rs->buf, &rs->buf_size, 0);
> - packet_ok (rs->buf, &remote_protocol_packets[PACKET_qSymbol]);
> - reply = rs->buf;
> + getpkt (&reply, &reply_size, 0);
> + packet_ok (reply, &remote_protocol_packets[PACKET_qSymbol]);
>
> while (startswith (reply, "qSymbol:"))
> {
> @@ -4384,8 +4397,7 @@ remote_check_symbols (void)
> }
>
> putpkt (msg);
> - getpkt (&rs->buf, &rs->buf_size, 0);
> - reply = rs->buf;
> + getpkt (&reply, &reply_size, 0);
> }
>
> do_cleanups (old_chain);
>
On 03/12/2016 03:02 PM, Marcin Kościelnicki wrote:
> gdb/ChangeLog | 5 +++++
> gdb/gdbserver/ChangeLog | 10 ++++++++++
> gdb/gdbserver/remote-utils.c | 41 +++++++++++++++++++++++++++--------------
> gdb/gdbserver/server.c | 4 ++--
> gdb/gdbserver/server.h | 4 ++++
> gdb/remote.c | 22 +++++++++++++++++-----
> 6 files changed, 65 insertions(+), 21 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index febe960..41f826e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-03-12 Marcin Kościelnicki <koriakin@0x04.net>
> +
> + * remote.c (xfreep): New function.
You can use free_current_contents instead.
> + (remote_check_symbols): Allocate own buffer for reply.
> +
> @@ -1499,21 +1499,35 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
> main loop. For now, this is an adequate approximation; allow
> GDB to read from memory while it figures out the address of the
> symbol. */
> - while (own_buf[0] == 'm')
> + while (1)
Comment above is stale.
OK with those addressed.
Thanks,
Pedro Alves
On 30/03/16 00:00, Pedro Alves wrote:
> On 03/12/2016 03:02 PM, Marcin Kościelnicki wrote:
>
>> gdb/ChangeLog | 5 +++++
>> gdb/gdbserver/ChangeLog | 10 ++++++++++
>> gdb/gdbserver/remote-utils.c | 41 +++++++++++++++++++++++++++--------------
>> gdb/gdbserver/server.c | 4 ++--
>> gdb/gdbserver/server.h | 4 ++++
>> gdb/remote.c | 22 +++++++++++++++++-----
>> 6 files changed, 65 insertions(+), 21 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index febe960..41f826e 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2016-03-12 Marcin Kościelnicki <koriakin@0x04.net>
>> +
>> + * remote.c (xfreep): New function.
>
> You can use free_current_contents instead.
Whoops, I should've guessed there's something like that already...
>
>> + (remote_check_symbols): Allocate own buffer for reply.
>> +
>
>> @@ -1499,21 +1499,35 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
>> main loop. For now, this is an adequate approximation; allow
>> GDB to read from memory while it figures out the address of the
>> symbol. */
>> - while (own_buf[0] == 'm')
>> + while (1)
>
> Comment above is stale.
>
> OK with those addressed.
Thanks, fixed and pushed.
>
> Thanks,
> Pedro Alves
>
@@ -1,3 +1,8 @@
+2016-03-12 Marcin Kościelnicki <koriakin@0x04.net>
+
+ * remote.c (xfreep): New function.
+ (remote_check_symbols): Allocate own buffer for reply.
+
2016-03-11 Marcin Kościelnicki <koriakin@0x04.net>
* s390-linux-tdep.c (s390_ax_pseudo_register_collect): New function.
@@ -1,3 +1,13 @@
+2016-03-12 Marcin Kościelnicki <koriakin@0x04.net>
+
+ * remote-utils.c (look_up_one_symbol): Remove own_buf, handle 'v'
+ packets.
+ (relocate_instruction): Remove own_buf.
+ * server.c (own_buf): Make global.
+ (handle_v_requests): Make global.
+ * server.h (own_buf): New declaration.
+ (handle_v_requests): New prototype.
+
2016-03-09 Marcin Kościelnicki <koriakin@0x04.net>
* linux-ppc-low.c (ppc_supports_tracepoints): New function.
@@ -1462,7 +1462,7 @@ clear_symbol_cache (struct sym_cache **symcache_p)
int
look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
{
- char own_buf[266], *p, *q;
+ char *p, *q;
int len;
struct sym_cache *sym;
struct process_info *proc;
@@ -1499,21 +1499,35 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
main loop. For now, this is an adequate approximation; allow
GDB to read from memory while it figures out the address of the
symbol. */
- while (own_buf[0] == 'm')
+ while (1)
{
- CORE_ADDR mem_addr;
- unsigned char *mem_buf;
- unsigned int mem_len;
+ if (own_buf[0] == 'm')
+ {
+ CORE_ADDR mem_addr;
+ unsigned char *mem_buf;
+ unsigned int mem_len;
- decode_m_packet (&own_buf[1], &mem_addr, &mem_len);
- mem_buf = (unsigned char *) xmalloc (mem_len);
- if (read_inferior_memory (mem_addr, mem_buf, mem_len) == 0)
- bin2hex (mem_buf, own_buf, mem_len);
+ decode_m_packet (&own_buf[1], &mem_addr, &mem_len);
+ mem_buf = (unsigned char *) xmalloc (mem_len);
+ if (read_inferior_memory (mem_addr, mem_buf, mem_len) == 0)
+ bin2hex (mem_buf, own_buf, mem_len);
+ else
+ write_enn (own_buf);
+ free (mem_buf);
+ if (putpkt (own_buf) < 0)
+ return -1;
+ }
+ else if (own_buf[0] == 'v')
+ {
+ int new_len = -1;
+ handle_v_requests (own_buf, len, &new_len);
+ if (new_len != -1)
+ putpkt_binary (own_buf, new_len);
+ else
+ putpkt (own_buf);
+ }
else
- write_enn (own_buf);
- free (mem_buf);
- if (putpkt (own_buf) < 0)
- return -1;
+ break;
len = getpkt (own_buf);
if (len < 0)
return -1;
@@ -1561,7 +1575,6 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
int
relocate_instruction (CORE_ADDR *to, CORE_ADDR oldloc)
{
- char own_buf[266];
int len;
ULONGEST written = 0;
@@ -119,7 +119,7 @@ int disable_packet_qfThreadInfo;
static struct target_waitstatus last_status;
static ptid_t last_ptid;
-static char *own_buf;
+char *own_buf;
static unsigned char *mem_buf;
/* A sub-class of 'struct notif_event' for stop, holding information
@@ -2935,7 +2935,7 @@ handle_v_kill (char *own_buf)
}
/* Handle all of the extended 'v' packets. */
-static void
+void
handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
{
if (!disable_packet_vCont)
@@ -82,6 +82,8 @@ extern int disable_packet_Tthread;
extern int disable_packet_qC;
extern int disable_packet_qfThreadInfo;
+extern char *own_buf;
+
extern int run_once;
extern int multi_process;
extern int report_fork_events;
@@ -113,6 +115,8 @@ typedef int gdb_fildes_t;
#include "event-loop.h"
/* Functions from server.c. */
+extern void handle_v_requests (char *own_buf, int packet_len,
+ int *new_packet_len);
extern int handle_serial_event (int err, gdb_client_data client_data);
extern int handle_target_event (int err, gdb_client_data client_data);
@@ -4321,6 +4321,16 @@ init_all_packet_configs (void)
}
}
+/* Helper function for cleanup below. Argument is a pointer to a void *
+ pointing to a buffer that needs to be freed. */
+
+static void
+xfreep (void *arg)
+{
+ void **pptr = (void **) arg;
+ xfree (*pptr);
+}
+
/* Symbol look-up. */
static void
@@ -4329,6 +4339,7 @@ remote_check_symbols (void)
struct remote_state *rs = get_remote_state ();
char *msg, *reply, *tmp;
int end;
+ long reply_size;
struct cleanup *old_chain;
/* The remote side has no concept of inferiors that aren't running
@@ -4350,13 +4361,15 @@ remote_check_symbols (void)
because we need both at the same time. */
msg = (char *) xmalloc (get_remote_packet_size ());
old_chain = make_cleanup (xfree, msg);
+ reply = (char *) xmalloc (get_remote_packet_size ());
+ make_cleanup (xfreep, &reply);
+ reply_size = get_remote_packet_size ();
/* Invite target to request symbol lookups. */
putpkt ("qSymbol::");
- getpkt (&rs->buf, &rs->buf_size, 0);
- packet_ok (rs->buf, &remote_protocol_packets[PACKET_qSymbol]);
- reply = rs->buf;
+ getpkt (&reply, &reply_size, 0);
+ packet_ok (reply, &remote_protocol_packets[PACKET_qSymbol]);
while (startswith (reply, "qSymbol:"))
{
@@ -4384,8 +4397,7 @@ remote_check_symbols (void)
}
putpkt (msg);
- getpkt (&rs->buf, &rs->buf_size, 0);
- reply = rs->buf;
+ getpkt (&reply, &reply_size, 0);
}
do_cleanups (old_chain);