* [PATCH 1/4] Create private_inferior class hierarchy
2017-11-22 16:41 [PATCH 0/4] Poison XNEW and friends for non-POD types Simon Marchi
@ 2017-11-22 16:41 ` Simon Marchi
2017-11-22 22:20 ` Joel Brobecker
2017-11-23 14:09 ` Pedro Alves
2017-11-22 16:42 ` [PATCH 4/4] Poison XNEW and friends for types that should use new/delete Simon Marchi
` (2 subsequent siblings)
3 siblings, 2 replies; 19+ messages in thread
From: Simon Marchi @ 2017-11-22 16:41 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
There are currently multiple definitions of private_inferior, defined in
remote.c and darwin-nat.h. The patch that poisons XNEW and friends for
non-POD types trips on that, because private_inferior is freed in
~inferior(), where it is an opaque type. Since the compiler can't tell
whether the type is POD, it gives an error. Also, we can't start using
C++ features in these structures (make them non-POD) as long as there
are multiple definitions with the same name. For these reasons, this
patch makes a class hierarchy, with private_inferior being the abstract
base class, and darwin_inferior & remote_inferior inheriting from it.
Destruction is done through the virtual destructor.
I stumbled on some suspicious code in the darwin implementation though.
darwin_check_new_threads does an XCNEW(darwin_thread_t) when it finds a
new thread, allocating a new structure for it (darwin_thread_t is a
typedef for private_thread_info). It then VEC_safe_pushes it in a
vector defined as DEF_VEC_O (a vector of objects). This means that the
structure content gets copied in the vector. The thread_info object is
created with the XCNEW'ed structure as the private thread info, while
the rest of the code works with the instance in the vector. We have
therefore two distinct instances of darwin_thread_t/private_thread_info
for each thread. This is not really a problem in practice, because
thread_info::priv is not used in the darwin code. I still find it weird
and far from ideal, so I tried to fix it by changing the vector to be a
vector of pointers. There should now be a single instance of the
structure for each thread. The deallocation of the
darwin_thread_t/private_thread_info structure is done by the thread_info
destructor.
I am able to build on macOS, but not really test, since the port seems a
bit broken. I am not able to debug reliably on the machine I have
access to, which runs macOS 10.12.6.
gdb/ChangeLog:
* inferior.h (private_inferior): Define structure type, add
virtual pure destructor.
(inferior) <priv>: Change type to unique_ptr.
* inferior.c (private_inferior::~private_inferior): Provide
default implementation.
(inferior::~inferior): Don't free priv field.
(exit_inferior_1): Likewise.
* darwin-nat.h (struct darwin_exception_info): Initialize fields.
(darwin_exception_info): Remove typedef.
(DEF_VEC_O (darwin_thread_t)); Remove.
(private_inferior): Rename to ...
(darwin_private_inferior): ... this, extend private_inferior.
<threads>: Change type to std::vector of darwin_thread_t pointers.
* darwin-nat.c (darwin_check_new_threads): Adjust.
(find_inferior_task_it): Adjust.
(darwin_find_thread); Adjust.
(darwin_suspend_inferior): Adjust.
(darwin_resume_inferior): Adjust.
(darwin_find_new_inferior): Adjust.
(darwin_decode_notify_message): Adjust.
(darwin_send_reply): Adjust.
(darwin_resume_inferior_threads): Adjust.
(darwin_suspend_inferior_threads): Adjust.
(darwin_decode_message): Adjust.
(darwin_wait): Adjust.
(darwin_interrupt): Adjust.
(darwin_deallocate_threads): Adjust.
(darwin_mourn_inferior): Adjust, don't free private data.
(darwin_reply_to_all_pending_messages): Adjust.
(darwin_stop_inferior): Adjust.
(darwin_setup_exceptions): Adjust.
(darwin_kill_inferior): Adjust.
(darwin_setup_request_notification): Adjust.
(darwin_attach_pid): Adjust.
(darwin_init_thread_list): Adjust.
(darwin_setup_fake_stop_event): Adjust.
(darwin_attach): Adjust.
(darwin_detach): Adjust.
(darwin_xfer_partial): Adjust.
(set_enable_mach_exceptions): Adjust.
(darwin_pid_to_exec_file): Adjust.
(darwin_get_ada_task_ptid): Adjust.
* darwin-nat-info.c (get_task_from_args): Adjust.
(info_mach_ports_command): Adjust.
(info_mach_region_command): Adjust.
(info_mach_exceptions_command): Adjust.
* remote.c (private_inferior): Rename to ...
(remote_private_inferior): ... this, initialize fields.
(get_remote_inferior); New.
(remote_commit_resume): Use get_remote_inferior.
(check_pending_event_prevents_wildcard_vcont_callback): Likewise.
---
gdb/darwin-nat-info.c | 50 +++++----
gdb/darwin-nat.c | 287 ++++++++++++++++++++++++++------------------------
gdb/darwin-nat.h | 30 +++---
gdb/inferior.c | 4 +-
gdb/inferior.h | 9 +-
gdb/remote.c | 30 ++++--
6 files changed, 218 insertions(+), 192 deletions(-)
diff --git a/gdb/darwin-nat-info.c b/gdb/darwin-nat-info.c
index 44782bf..feab181 100644
--- a/gdb/darwin-nat-info.c
+++ b/gdb/darwin-nat-info.c
@@ -118,7 +118,11 @@ get_task_from_args (const char *args)
{
if (ptid_equal (inferior_ptid, null_ptid))
printf_unfiltered (_("No inferior running\n"));
- return current_inferior ()->priv->task;
+
+ darwin_inferior *priv
+ = (darwin_inferior *) current_inferior ()->priv.get ();
+
+ return priv->task;
}
if (strcmp (args, "gdb") == 0)
return mach_task_self ();
@@ -257,36 +261,31 @@ info_mach_ports_command (const char *args, int from_tty)
else if (!ptid_equal (inferior_ptid, null_ptid))
{
struct inferior *inf = current_inferior ();
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
- if (port == inf->priv->task)
+ if (port == priv->task)
printf_unfiltered (_(" inferior-task"));
- else if (port == inf->priv->notify_port)
+ else if (port == priv->notify_port)
printf_unfiltered (_(" inferior-notify"));
else
{
- int k;
- darwin_thread_t *t;
-
- for (k = 0; k < inf->priv->exception_info.count; k++)
- if (port == inf->priv->exception_info.ports[k])
+ for (int k = 0; k < priv->exception_info.count; k++)
+ if (port == priv->exception_info.ports[k])
{
printf_unfiltered (_(" inferior-excp-port"));
break;
}
- if (inf->priv->threads)
+ for (darwin_thread_t *t : priv->threads)
{
- for (k = 0;
- VEC_iterate(darwin_thread_t,
- inf->priv->threads, k, t);
- k++)
- if (port == t->gdb_port)
- {
- printf_unfiltered (_(" inferior-thread for 0x%x"),
- inf->priv->task);
- break;
- }
+ if (port == t->gdb_port)
+ {
+ printf_unfiltered (_(" inferior-thread for 0x%x"),
+ priv->task);
+ break;
+ }
}
+
}
}
}
@@ -738,7 +737,8 @@ info_mach_region_command (const char *exp, int from_tty)
error (_("Inferior not available"));
inf = current_inferior ();
- darwin_debug_region (inf->priv->task, address);
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+ darwin_debug_region (priv->task, address);
}
static void
@@ -807,7 +807,11 @@ info_mach_exceptions_command (const char *args, int from_tty)
{
if (ptid_equal (inferior_ptid, null_ptid))
printf_unfiltered (_("No inferior running\n"));
- disp_exception (¤t_inferior ()->priv->exception_info);
+
+ darwin_inferior *priv
+ = (darwin_inferior *) current_inferior ()->priv.get ();
+
+ disp_exception (&priv->exception_info);
return;
}
else if (strcmp (args, "host") == 0)
@@ -830,8 +834,10 @@ info_mach_exceptions_command (const char *args, int from_tty)
printf_unfiltered (_("No inferior running\n"));
inf = current_inferior ();
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
kret = task_get_exception_ports
- (inf->priv->task, EXC_MASK_ALL, info.masks,
+ (priv->task, EXC_MASK_ALL, info.masks,
&info.count, info.ports, info.behaviors, info.flavors);
MACH_CHECK_ERROR (kret);
disp_exception (&info);
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 6e420c4..f4ed455 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -279,13 +279,12 @@ static void
darwin_check_new_threads (struct inferior *inf)
{
kern_return_t kret;
- unsigned int i;
thread_array_t thread_list;
unsigned int new_nbr;
unsigned int old_nbr;
unsigned int new_ix, old_ix;
- darwin_inferior *darwin_inf = inf->priv;
- VEC (darwin_thread_t) *thread_vec;
+ darwin_inferior *darwin_inf = (darwin_inferior *) inf->priv.get ();
+ std::vector<darwin_thread_t *> new_thread_vec;
/* Get list of threads. */
kret = task_threads (darwin_inf->task, &thread_list, &new_nbr);
@@ -297,17 +296,15 @@ darwin_check_new_threads (struct inferior *inf)
if (new_nbr > 1)
qsort (thread_list, new_nbr, sizeof (thread_t), cmp_thread_t);
- if (darwin_inf->threads)
- old_nbr = VEC_length (darwin_thread_t, darwin_inf->threads);
- else
- old_nbr = 0;
+ old_nbr = darwin_inf->threads.size ();
/* Quick check for no changes. */
if (old_nbr == new_nbr)
{
+ size_t i;
+
for (i = 0; i < new_nbr; i++)
- if (thread_list[i]
- != VEC_index (darwin_thread_t, darwin_inf->threads, i)->gdb_port)
+ if (thread_list[i] != darwin_inf->threads[i]->gdb_port)
break;
if (i == new_nbr)
{
@@ -328,15 +325,15 @@ darwin_check_new_threads (struct inferior *inf)
}
/* Full handling: detect new threads, remove dead threads. */
- thread_vec = VEC_alloc (darwin_thread_t, new_nbr);
+
+ new_thread_vec.reserve (new_nbr);
for (new_ix = 0, old_ix = 0; new_ix < new_nbr || old_ix < old_nbr;)
{
- thread_t new_id = (new_ix < new_nbr) ?
- thread_list[new_ix] : THREAD_NULL;
- darwin_thread_t *old = (old_ix < old_nbr) ?
- VEC_index (darwin_thread_t, darwin_inf->threads, old_ix) : NULL;
- thread_t old_id = old ? old->gdb_port : THREAD_NULL;
+ thread_t new_id = (new_ix < new_nbr) ? thread_list[new_ix] : THREAD_NULL;
+ darwin_thread_t *old
+ = (old_ix < old_nbr) ? darwin_inf->threads[old_ix] : NULL;
+ thread_t old_id = old != NULL ? old->gdb_port : THREAD_NULL;
inferior_debug
(12, _(" new_ix:%d/%d, old_ix:%d/%d, new_id:0x%x old_id:0x%x\n"),
@@ -345,7 +342,7 @@ darwin_check_new_threads (struct inferior *inf)
if (old_id == new_id)
{
/* Thread still exist. */
- VEC_safe_push (darwin_thread_t, thread_vec, old);
+ new_thread_vec.push_back (old);
new_ix++;
old_ix++;
@@ -368,13 +365,13 @@ darwin_check_new_threads (struct inferior *inf)
/* A thread was created. */
struct private_thread_info *pti;
- pti = XCNEW (struct private_thread_info);
+ pti = XCNEW (darwin_thread_t);
pti->gdb_port = new_id;
pti->msg_state = DARWIN_RUNNING;
/* Add the new thread. */
add_thread_with_info (ptid_build (inf->pid, 0, new_id), pti);
- VEC_safe_push (darwin_thread_t, thread_vec, pti);
+ new_thread_vec.push_back (pti);
new_ix++;
continue;
}
@@ -390,9 +387,7 @@ darwin_check_new_threads (struct inferior *inf)
gdb_assert_not_reached ("unexpected thread case");
}
- if (darwin_inf->threads)
- VEC_free (darwin_thread_t, darwin_inf->threads);
- darwin_inf->threads = thread_vec;
+ darwin_inf->threads = std::move (new_thread_vec);
/* Deallocate the buffer. */
kret = vm_deallocate (gdb_task, (vm_address_t) thread_list,
@@ -403,7 +398,9 @@ darwin_check_new_threads (struct inferior *inf)
static int
find_inferior_task_it (struct inferior *inf, void *port_ptr)
{
- return inf->priv->task == *(task_t *)port_ptr;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
+ return priv->task == *(task_t *)port_ptr;
}
static int
@@ -430,14 +427,14 @@ darwin_find_inferior_by_pid (int pid)
static darwin_thread_t *
darwin_find_thread (struct inferior *inf, thread_t thread)
{
- darwin_thread_t *t;
- int k;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
+ for (darwin_thread_t *t : priv->threads)
+ {
+ if (t->gdb_port == thread)
+ return t;
+ }
- for (k = 0;
- VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
- k++)
- if (t->gdb_port == thread)
- return t;
return NULL;
}
@@ -446,14 +443,16 @@ darwin_find_thread (struct inferior *inf, thread_t thread)
static void
darwin_suspend_inferior (struct inferior *inf)
{
- if (!inf->priv->suspended)
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
+ if (!priv->suspended)
{
kern_return_t kret;
- kret = task_suspend (inf->priv->task);
+ kret = task_suspend (priv->task);
MACH_CHECK_ERROR (kret);
- inf->priv->suspended = 1;
+ priv->suspended = 1;
}
}
@@ -462,14 +461,16 @@ darwin_suspend_inferior (struct inferior *inf)
static void
darwin_resume_inferior (struct inferior *inf)
{
- if (inf->priv->suspended)
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
+ if (priv->suspended)
{
kern_return_t kret;
- kret = task_resume (inf->priv->task);
+ kret = task_resume (priv->task);
MACH_CHECK_ERROR (kret);
- inf->priv->suspended = 0;
+ priv->suspended = 0;
}
}
@@ -580,11 +581,13 @@ darwin_find_new_inferior (task_t task_port, thread_t thread_port)
if (inf == NULL)
return NULL;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
/* Deallocate saved exception ports. */
- darwin_deallocate_exception_ports (inf->priv);
+ darwin_deallocate_exception_ports (priv);
/* No need to remove dead_name notification, but still... */
- kret = mach_port_request_notification (gdb_task, inf->priv->task,
+ kret = mach_port_request_notification (gdb_task, priv->task,
MACH_NOTIFY_DEAD_NAME, 0,
MACH_PORT_NULL,
MACH_MSG_TYPE_MAKE_SEND_ONCE,
@@ -593,9 +596,9 @@ darwin_find_new_inferior (task_t task_port, thread_t thread_port)
MACH_CHECK_ERROR (kret);
/* Replace old task port. */
- kret = mach_port_deallocate (gdb_task, inf->priv->task);
+ kret = mach_port_deallocate (gdb_task, priv->task);
MACH_CHECK_ERROR (kret);
- inf->priv->task = task_port;
+ priv->task = task_port;
darwin_setup_request_notification (inf);
darwin_setup_exceptions (inf);
@@ -789,8 +792,10 @@ darwin_decode_notify_message (mach_msg_header_t *hdr, struct inferior **pinf)
inf = darwin_find_inferior_by_task (task_port);
*pinf = inf;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
/* Check message destination. */
- if (inf != NULL && hdr->msgh_local_port != inf->priv->notify_port)
+ if (inf != NULL && hdr->msgh_local_port != priv->notify_port)
return -4;
return 0;
@@ -817,6 +822,7 @@ darwin_send_reply (struct inferior *inf, darwin_thread_t *thread)
{
kern_return_t kret;
mig_reply_error_t reply;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
darwin_encode_reply (&reply, &thread->event.header, KERN_SUCCESS);
@@ -826,7 +832,7 @@ darwin_send_reply (struct inferior *inf, darwin_thread_t *thread)
MACH_PORT_NULL);
MACH_CHECK_ERROR (kret);
- inf->priv->pending_messages--;
+ priv->pending_messages--;
}
static void
@@ -889,12 +895,9 @@ darwin_resume_thread (struct inferior *inf, darwin_thread_t *thread,
static void
darwin_resume_inferior_threads (struct inferior *inf, int step, int nsignal)
{
- darwin_thread_t *thread;
- int k;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
- for (k = 0;
- VEC_iterate (darwin_thread_t, inf->priv->threads, k, thread);
- k++)
+ for (darwin_thread_t *thread : priv->threads)
darwin_resume_thread (inf, thread, step, nsignal);
}
@@ -920,24 +923,24 @@ darwin_resume_inferior_threads_it (struct inferior *inf, void *param)
static void
darwin_suspend_inferior_threads (struct inferior *inf)
{
- darwin_thread_t *thread;
- kern_return_t kret;
- int k;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
- for (k = 0;
- VEC_iterate (darwin_thread_t, inf->priv->threads, k, thread);
- k++)
- switch (thread->msg_state)
- {
- case DARWIN_STOPPED:
- case DARWIN_MESSAGE:
- break;
- case DARWIN_RUNNING:
- kret = thread_suspend (thread->gdb_port);
- MACH_CHECK_ERROR (kret);
- thread->msg_state = DARWIN_STOPPED;
- break;
- }
+ for (darwin_thread_t *thread : priv->threads)
+ {
+ switch (thread->msg_state)
+ {
+ case DARWIN_STOPPED:
+ case DARWIN_MESSAGE:
+ break;
+ case DARWIN_RUNNING:
+ {
+ kern_return_t kret = thread_suspend (thread->gdb_port);
+ MACH_CHECK_ERROR (kret);
+ thread->msg_state = DARWIN_STOPPED;
+ break;
+ }
+ }
+ }
}
static void
@@ -1045,7 +1048,10 @@ darwin_decode_message (mach_msg_header_t *hdr,
}
*pinf = inf;
*pthread = thread;
- inf->priv->pending_messages++;
+
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
+ priv->pending_messages++;
status->kind = TARGET_WAITKIND_STOPPED;
thread->msg_state = DARWIN_MESSAGE;
@@ -1129,7 +1135,9 @@ darwin_decode_message (mach_msg_header_t *hdr,
if (inf != NULL)
{
- if (!inf->priv->no_ptrace)
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
+ if (!priv->no_ptrace)
{
pid_t res;
int wstatus;
@@ -1232,9 +1240,11 @@ darwin_wait (ptid_t ptid, struct target_waitstatus *status)
inf = darwin_inf_fake_stop;
darwin_inf_fake_stop = NULL;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
status->kind = TARGET_WAITKIND_STOPPED;
status->value.sig = GDB_SIGNAL_TRAP;
- thread = VEC_index (darwin_thread_t, inf->priv->threads, 0);
+ thread = priv->threads[0];
thread->msg_state = DARWIN_STOPPED;
return ptid_build (inf->pid, 0, thread->gdb_port);
}
@@ -1336,9 +1346,10 @@ static void
darwin_interrupt (struct target_ops *self, ptid_t t)
{
struct inferior *inf = current_inferior ();
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
/* FIXME: handle in no_ptrace mode. */
- gdb_assert (!inf->priv->no_ptrace);
+ gdb_assert (!priv->no_ptrace);
kill (inf->pid, SIGINT);
}
@@ -1347,27 +1358,22 @@ darwin_interrupt (struct target_ops *self, ptid_t t)
static void
darwin_deallocate_threads (struct inferior *inf)
{
- if (inf->priv->threads)
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
+ for (darwin_thread_t *t : priv->threads)
{
- kern_return_t kret;
- int k;
- darwin_thread_t *t;
- for (k = 0;
- VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
- k++)
- {
- kret = mach_port_deallocate (gdb_task, t->gdb_port);
- MACH_CHECK_ERROR (kret);
- }
- VEC_free (darwin_thread_t, inf->priv->threads);
- inf->priv->threads = NULL;
+ kern_return_t kret = mach_port_deallocate (gdb_task, t->gdb_port);
+ MACH_CHECK_ERROR (kret);
}
+
+ priv->threads.clear ();
}
static void
darwin_mourn_inferior (struct target_ops *ops)
{
struct inferior *inf = current_inferior ();
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
kern_return_t kret;
mach_port_t prev;
int i;
@@ -1377,18 +1383,18 @@ darwin_mourn_inferior (struct target_ops *ops)
/* Remove notify_port from darwin_port_set. */
kret = mach_port_move_member (gdb_task,
- inf->priv->notify_port, MACH_PORT_NULL);
+ priv->notify_port, MACH_PORT_NULL);
MACH_CHECK_ERROR (kret);
/* Remove task port dead_name notification. */
- kret = mach_port_request_notification (gdb_task, inf->priv->task,
+ kret = mach_port_request_notification (gdb_task, priv->task,
MACH_NOTIFY_DEAD_NAME, 0,
MACH_PORT_NULL,
MACH_MSG_TYPE_MAKE_SEND_ONCE,
&prev);
/* This can fail if the task is dead. */
inferior_debug (4, "task=0x%x, prev=0x%x, notify_port=0x%x\n",
- inf->priv->task, prev, inf->priv->notify_port);
+ priv->task, prev, priv->notify_port);
if (kret == KERN_SUCCESS)
{
@@ -1397,17 +1403,16 @@ darwin_mourn_inferior (struct target_ops *ops)
}
/* Destroy notify_port. */
- kret = mach_port_destroy (gdb_task, inf->priv->notify_port);
+ kret = mach_port_destroy (gdb_task, priv->notify_port);
MACH_CHECK_ERROR (kret);
/* Deallocate saved exception ports. */
- darwin_deallocate_exception_ports (inf->priv);
+ darwin_deallocate_exception_ports (priv);
/* Deallocate task port. */
- kret = mach_port_deallocate (gdb_task, inf->priv->task);
+ kret = mach_port_deallocate (gdb_task, priv->task);
MACH_CHECK_ERROR (kret);
- xfree (inf->priv);
inf->priv = NULL;
inf_child_mourn_inferior (ops);
@@ -1416,12 +1421,9 @@ darwin_mourn_inferior (struct target_ops *ops)
static void
darwin_reply_to_all_pending_messages (struct inferior *inf)
{
- int k;
- darwin_thread_t *t;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
- for (k = 0;
- VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
- k++)
+ for (darwin_thread_t *t : priv->threads)
{
if (t->msg_state == DARWIN_MESSAGE)
darwin_resume_thread (inf, t, 0, 0);
@@ -1436,6 +1438,7 @@ darwin_stop_inferior (struct inferior *inf)
kern_return_t kret;
int status;
int res;
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
gdb_assert (inf != NULL);
@@ -1443,7 +1446,7 @@ darwin_stop_inferior (struct inferior *inf)
darwin_reply_to_all_pending_messages (inf);
- if (inf->priv->no_ptrace)
+ if (priv->no_ptrace)
return;
res = kill (inf->pid, SIGSTOP);
@@ -1512,11 +1515,12 @@ darwin_deallocate_exception_ports (darwin_inferior *inf)
static void
darwin_setup_exceptions (struct inferior *inf)
{
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
kern_return_t kret;
int traps_expected;
exception_mask_t mask;
- kret = darwin_save_exception_ports (inf->priv);
+ kret = darwin_save_exception_ports (priv);
if (kret != KERN_SUCCESS)
error (_("Unable to save exception ports, task_get_exception_ports"
"returned: %d"),
@@ -1527,7 +1531,7 @@ darwin_setup_exceptions (struct inferior *inf)
mask = EXC_MASK_ALL;
else
mask = EXC_MASK_SOFTWARE | EXC_MASK_BREAKPOINT;
- kret = task_set_exception_ports (inf->priv->task, mask, darwin_ex_port,
+ kret = task_set_exception_ports (priv->task, mask, darwin_ex_port,
EXCEPTION_DEFAULT, THREAD_STATE_NONE);
if (kret != KERN_SUCCESS)
error (_("Unable to set exception ports, task_set_exception_ports"
@@ -1539,6 +1543,7 @@ static void
darwin_kill_inferior (struct target_ops *ops)
{
struct inferior *inf = current_inferior ();
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
struct target_waitstatus wstatus;
ptid_t ptid;
kern_return_t kret;
@@ -1550,7 +1555,7 @@ darwin_kill_inferior (struct target_ops *ops)
gdb_assert (inf != NULL);
- kret = darwin_restore_exception_ports (inf->priv);
+ kret = darwin_restore_exception_ports (priv);
MACH_CHECK_ERROR (kret);
darwin_reply_to_all_pending_messages (inf);
@@ -1573,12 +1578,13 @@ darwin_kill_inferior (struct target_ops *ops)
static void
darwin_setup_request_notification (struct inferior *inf)
{
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
kern_return_t kret;
mach_port_t prev_not;
- kret = mach_port_request_notification (gdb_task, inf->priv->task,
+ kret = mach_port_request_notification (gdb_task, priv->task,
MACH_NOTIFY_DEAD_NAME, 0,
- inf->priv->notify_port,
+ priv->notify_port,
MACH_MSG_TYPE_MAKE_SEND_ONCE,
&prev_not);
if (kret != KERN_SUCCESS)
@@ -1607,9 +1613,10 @@ darwin_attach_pid (struct inferior *inf)
mach_port_t prev_not;
exception_mask_t mask;
- inf->priv = XCNEW (darwin_inferior);
+ darwin_inferior *priv = new darwin_inferior;
+ inf->priv.reset (priv);
- kret = task_for_pid (gdb_task, inf->pid, &inf->priv->task);
+ kret = task_for_pid (gdb_task, inf->pid, &priv->task);
if (kret != KERN_SUCCESS)
{
int status;
@@ -1626,7 +1633,7 @@ darwin_attach_pid (struct inferior *inf)
}
inferior_debug (2, _("inferior task: 0x%x, pid: %d\n"),
- inf->priv->task, inf->pid);
+ priv->task, inf->pid);
if (darwin_ex_port == MACH_PORT_NULL)
{
@@ -1663,14 +1670,14 @@ darwin_attach_pid (struct inferior *inf)
/* Create a port to be notified when the child task terminates. */
kret = mach_port_allocate (gdb_task, MACH_PORT_RIGHT_RECEIVE,
- &inf->priv->notify_port);
+ &priv->notify_port);
if (kret != KERN_SUCCESS)
error (_("Unable to create notification port, mach_port_allocate "
"returned: %d"),
kret);
kret = mach_port_move_member (gdb_task,
- inf->priv->notify_port, darwin_port_set);
+ priv->notify_port, darwin_port_set);
if (kret != KERN_SUCCESS)
error (_("Unable to move notification port into new port set, "
"mach_port_move_member\n"
@@ -1708,11 +1715,11 @@ darwin_init_thread_list (struct inferior *inf)
{
darwin_check_new_threads (inf);
- gdb_assert (inf->priv->threads != NULL);
- gdb_assert (VEC_length (darwin_thread_t, inf->priv->threads) > 0);
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
- private_thread_info *first_pti
- = VEC_index (darwin_thread_t, inf->priv->threads, 0);
+ gdb_assert (!priv->threads.empty ());
+
+ private_thread_info *first_pti = priv->threads.front ();
struct thread_info *first_thread
= thread_info_from_private_thread_info (first_pti);
@@ -1849,6 +1856,7 @@ darwin_create_inferior (struct target_ops *ops,
static void
darwin_setup_fake_stop_event (struct inferior *inf)
{
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
darwin_thread_t *thread;
kern_return_t kret;
@@ -1861,7 +1869,7 @@ darwin_setup_fake_stop_event (struct inferior *inf)
as well. Otherwise, we'll try resuming it when resuming the
inferior, and get a warning because the thread's suspend count
is already zero, making the resume request useless. */
- thread = VEC_index (darwin_thread_t, inf->priv->threads, 0);
+ thread = priv->threads[0];
kret = thread_suspend (thread->gdb_port);
MACH_CHECK_ERROR (kret);
}
@@ -1912,11 +1920,13 @@ darwin_attach (struct target_ops *ops, const char *args, int from_tty)
darwin_init_thread_list (inf);
- darwin_check_osabi (inf->priv, ptid_get_tid (inferior_ptid));
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
+
+ darwin_check_osabi (priv, ptid_get_tid (inferior_ptid));
darwin_setup_fake_stop_event (inf);
- inf->priv->no_ptrace = 1;
+ priv->no_ptrace = 1;
}
/* Take a program previously attached to and detaches it.
@@ -1931,6 +1941,7 @@ darwin_detach (struct target_ops *ops, const char *args, int from_tty)
{
pid_t pid = ptid_get_pid (inferior_ptid);
struct inferior *inf = current_inferior ();
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
kern_return_t kret;
int res;
@@ -1938,13 +1949,13 @@ darwin_detach (struct target_ops *ops, const char *args, int from_tty)
target_announce_detach (from_tty);
/* If ptrace() is in use, stop the process. */
- if (!inf->priv->no_ptrace)
+ if (!priv->no_ptrace)
darwin_stop_inferior (inf);
- kret = darwin_restore_exception_ports (inf->priv);
+ kret = darwin_restore_exception_ports (priv);
MACH_CHECK_ERROR (kret);
- if (!inf->priv->no_ptrace)
+ if (!priv->no_ptrace)
{
res = PTRACE (PT_DETACH, inf->pid, 0, 0);
if (res != 0)
@@ -1957,7 +1968,7 @@ darwin_detach (struct target_ops *ops, const char *args, int from_tty)
/* When using ptrace, we have just performed a PT_DETACH, which
resumes the inferior. On the other hand, when we are not using
ptrace, we need to resume its execution ourselves. */
- if (inf->priv->no_ptrace)
+ if (priv->no_ptrace)
darwin_resume_inferior (inf);
darwin_mourn_inferior (ops);
@@ -2189,6 +2200,7 @@ darwin_xfer_partial (struct target_ops *ops,
ULONGEST offset, ULONGEST len, ULONGEST *xfered_len)
{
struct inferior *inf = current_inferior ();
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
inferior_debug
(8, _("darwin_xfer_partial(%s, %s, rbuf=%s, wbuf=%s) pid=%u\n"),
@@ -2200,7 +2212,7 @@ darwin_xfer_partial (struct target_ops *ops,
{
case TARGET_OBJECT_MEMORY:
{
- int l = darwin_read_write_inferior (inf->priv->task, offset,
+ int l = darwin_read_write_inferior (priv->task, offset,
readbuf, writebuf, len);
if (l == 0)
@@ -2219,7 +2231,7 @@ darwin_xfer_partial (struct target_ops *ops,
/* Support only read. */
return TARGET_XFER_E_IO;
}
- return darwin_read_dyld_info (inf->priv->task, offset, readbuf, len,
+ return darwin_read_dyld_info (priv->task, offset, readbuf, len,
xfered_len);
#endif
default:
@@ -2235,6 +2247,7 @@ set_enable_mach_exceptions (const char *args, int from_tty,
if (!ptid_equal (inferior_ptid, null_ptid))
{
struct inferior *inf = current_inferior ();
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
exception_mask_t mask;
kern_return_t kret;
@@ -2242,10 +2255,10 @@ set_enable_mach_exceptions (const char *args, int from_tty,
mask = EXC_MASK_ALL;
else
{
- darwin_restore_exception_ports (inf->priv);
+ darwin_restore_exception_ports (priv);
mask = EXC_MASK_SOFTWARE | EXC_MASK_BREAKPOINT;
}
- kret = task_set_exception_ports (inf->priv->task, mask, darwin_ex_port,
+ kret = task_set_exception_ports (priv->task, mask, darwin_ex_port,
EXCEPTION_DEFAULT, THREAD_STATE_NONE);
MACH_CHECK_ERROR (kret);
}
@@ -2267,10 +2280,8 @@ darwin_pid_to_exec_file (struct target_ops *self, int pid)
static ptid_t
darwin_get_ada_task_ptid (struct target_ops *self, long lwp, long thread)
{
- int i;
- darwin_thread_t *t;
- int k;
struct inferior *inf = current_inferior ();
+ darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
kern_return_t kret;
mach_port_name_array_t names;
mach_msg_type_number_t names_count;
@@ -2279,16 +2290,16 @@ darwin_get_ada_task_ptid (struct target_ops *self, long lwp, long thread)
long res = 0;
/* First linear search. */
- for (k = 0;
- VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
- k++)
- if (t->inf_port == lwp)
- return ptid_build (ptid_get_pid (inferior_ptid), 0, t->gdb_port);
+ for (darwin_thread_t *t : priv->threads)
+ {
+ if (t->inf_port == lwp)
+ return ptid_build (ptid_get_pid (inferior_ptid), 0, t->gdb_port);
+ }
/* Maybe the port was never extract. Do it now. */
/* First get inferior port names. */
- kret = mach_port_names (inf->priv->task, &names, &names_count, &types,
+ kret = mach_port_names (priv->task, &names, &names_count, &types,
&types_count);
MACH_CHECK_ERROR (kret);
if (kret != KERN_SUCCESS)
@@ -2297,29 +2308,29 @@ darwin_get_ada_task_ptid (struct target_ops *self, long lwp, long thread)
/* For each name, copy the right in the gdb space and then compare with
our view of the inferior threads. We don't forget to deallocate the
right. */
- for (i = 0; i < names_count; i++)
+ for (int i = 0; i < names_count; i++)
{
mach_port_t local_name;
mach_msg_type_name_t local_type;
/* We just need to know the corresponding name in gdb name space.
So extract and deallocate the right. */
- kret = mach_port_extract_right (inf->priv->task, names[i],
+ kret = mach_port_extract_right (priv->task, names[i],
MACH_MSG_TYPE_COPY_SEND,
&local_name, &local_type);
if (kret != KERN_SUCCESS)
continue;
mach_port_deallocate (gdb_task, local_name);
- for (k = 0;
- VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
- k++)
- if (t->gdb_port == local_name)
- {
- t->inf_port = names[i];
- if (names[i] == lwp)
- res = t->gdb_port;
- }
+ for (darwin_thread_t *t : priv->threads)
+ {
+ if (t->gdb_port == local_name)
+ {
+ t->inf_port = names[i];
+ if (names[i] == lwp)
+ res = t->gdb_port;
+ }
+ }
}
vm_deallocate (gdb_task, (vm_address_t) names,
diff --git a/gdb/darwin-nat.h b/gdb/darwin-nat.h
index 77feb0e..24c10b5 100644
--- a/gdb/darwin-nat.h
+++ b/gdb/darwin-nat.h
@@ -26,21 +26,20 @@
struct darwin_exception_info
{
/* Exceptions handled by the port. */
- exception_mask_t masks[EXC_TYPES_COUNT];
+ exception_mask_t masks[EXC_TYPES_COUNT] {};
/* Ports receiving exception messages. */
- mach_port_t ports[EXC_TYPES_COUNT];
+ mach_port_t ports[EXC_TYPES_COUNT] {};
/* Type of messages sent. */
- exception_behavior_t behaviors[EXC_TYPES_COUNT];
+ exception_behavior_t behaviors[EXC_TYPES_COUNT] {};
/* Type of state to be sent. */
- thread_state_flavor_t flavors[EXC_TYPES_COUNT];
+ thread_state_flavor_t flavors[EXC_TYPES_COUNT] {};
/* Number of elements set. */
- mach_msg_type_number_t count;
+ mach_msg_type_number_t count = 0;
};
-typedef struct darwin_exception_info darwin_exception_info;
struct darwin_exception_msg
{
@@ -95,36 +94,31 @@ struct private_thread_info
};
typedef struct private_thread_info darwin_thread_t;
-/* Define the threads vector type. */
-DEF_VEC_O (darwin_thread_t);
-
-
/* Describe an inferior. */
-struct private_inferior
+struct darwin_inferior : public private_inferior
{
/* Corresponding task port. */
- task_t task;
+ task_t task = 0;
/* Port which will receive the dead-name notification for the task port.
This is used to detect the death of the task. */
- mach_port_t notify_port;
+ mach_port_t notify_port = 0;
/* Initial exception handling. */
darwin_exception_info exception_info;
/* Number of messages that have been received but not yet replied. */
- unsigned int pending_messages;
+ unsigned int pending_messages = 0;
/* Set if inferior is not controlled by ptrace(2) but through Mach. */
- unsigned char no_ptrace;
+ bool no_ptrace = false;
/* True if this task is suspended. */
- unsigned char suspended;
+ bool suspended = false;
/* Sorted vector of known threads. */
- VEC(darwin_thread_t) *threads;
+ std::vector<darwin_thread_t *> threads;
};
-typedef struct private_inferior darwin_inferior;
/* Exception port. */
extern mach_port_t darwin_ex_port;
diff --git a/gdb/inferior.c b/gdb/inferior.c
index bcac981..9b3043d 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -71,6 +71,8 @@ set_current_inferior (struct inferior *inf)
current_inferior_ = inf;
}
+private_inferior::~private_inferior () = default;
+
inferior::~inferior ()
{
inferior *inf = this;
@@ -80,7 +82,6 @@ inferior::~inferior ()
xfree (inf->args);
xfree (inf->terminal);
target_desc_info_free (inf->tdesc_info);
- xfree (inf->priv);
}
inferior::inferior (int pid_)
@@ -209,7 +210,6 @@ exit_inferior_1 (struct inferior *inftoex, int silent)
inf->pid = 0;
inf->fake_pid_p = 0;
- xfree (inf->priv);
inf->priv = NULL;
if (inf->vfork_parent != NULL)
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 37252a6..0705dd9 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -263,7 +263,12 @@ enum stop_kind
#define ON_STACK 1
#define AT_ENTRY_POINT 4
-struct private_inferior;
+/* Base class for target-specific inferior data. */
+
+struct private_inferior
+{
+ virtual ~private_inferior () = 0;
+};
/* Inferior process specific part of `struct infcall_control_state'.
@@ -403,7 +408,7 @@ public:
bool needs_setup = false;
/* Private data used by the target vector implementation. */
- private_inferior *priv = NULL;
+ std::unique_ptr<private_inferior> priv;
/* HAS_EXIT_CODE is true if the inferior exited with an exit code.
In this case, the EXIT_CODE field is also valid. */
diff --git a/gdb/remote.c b/gdb/remote.c
index 62ac055..f328080 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5826,12 +5826,22 @@ static int is_pending_fork_parent_thread (struct thread_info *thread);
/* Private per-inferior info for target remote processes. */
-struct private_inferior
+struct remote_inferior : public private_inferior
{
/* Whether we can send a wildcard vCont for this process. */
- int may_wildcard_vcont;
+ bool may_wildcard_vcont = true;
};
+/* Get the remote private inferior data associated to INF. */
+
+static remote_inferior *get_remote_inferior (inferior *inf)
+{
+ if (inf->priv == NULL)
+ inf->priv.reset (new remote_inferior);
+
+ return (remote_inferior *) inf->priv.get ();
+}
+
/* Structure used to track the construction of a vCont packet in the
outgoing packet buffer. This is used to send multiple vCont
packets if we have more actions than would fit a single packet. */
@@ -5993,9 +6003,9 @@ remote_commit_resume (struct target_ops *ops)
/* And assume every process is individually wildcard-able too. */
ALL_NON_EXITED_INFERIORS (inf)
{
- if (inf->priv == NULL)
- inf->priv = XNEW (struct private_inferior);
- inf->priv->may_wildcard_vcont = 1;
+ remote_inferior *priv = get_remote_inferior (inf);
+
+ priv->may_wildcard_vcont = true;
}
/* Check for any pending events (not reported or processed yet) and
@@ -6008,7 +6018,7 @@ remote_commit_resume (struct target_ops *ops)
can't wildcard that process. */
if (!tp->executing)
{
- tp->inf->priv->may_wildcard_vcont = 0;
+ get_remote_inferior (tp->inf)->may_wildcard_vcont = false;
/* And if we can't wildcard a process, we can't wildcard
everything either. */
@@ -6042,7 +6052,7 @@ remote_commit_resume (struct target_ops *ops)
if (!remote_thr->last_resume_step
&& remote_thr->last_resume_sig == GDB_SIGNAL_0
- && tp->inf->priv->may_wildcard_vcont)
+ && get_remote_inferior (tp->inf)->may_wildcard_vcont)
{
/* We'll send a wildcard resume instead. */
remote_thr->vcont_resumed = 1;
@@ -6062,7 +6072,7 @@ remote_commit_resume (struct target_ops *ops)
ALL_NON_EXITED_INFERIORS (inf)
{
- if (inf->priv->may_wildcard_vcont)
+ if (get_remote_inferior (inf)->may_wildcard_vcont)
{
any_process_wildcard = 1;
break;
@@ -6083,7 +6093,7 @@ remote_commit_resume (struct target_ops *ops)
{
ALL_NON_EXITED_INFERIORS (inf)
{
- if (inf->priv->may_wildcard_vcont)
+ if (get_remote_inferior (inf)->may_wildcard_vcont)
{
vcont_builder_push_action (&vcont_builder,
pid_to_ptid (inf->pid),
@@ -6579,7 +6589,7 @@ check_pending_event_prevents_wildcard_vcont_callback
we'd resume this process too. */
*may_global_wildcard_vcont = 0;
if (inf != NULL)
- inf->priv->may_wildcard_vcont = 0;
+ get_remote_inferior (inf)->may_wildcard_vcont = false;
return 1;
}
--
2.7.4
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/4] Create private_inferior class hierarchy
2017-11-22 16:41 ` [PATCH 1/4] Create private_inferior class hierarchy Simon Marchi
@ 2017-11-22 22:20 ` Joel Brobecker
2017-11-24 15:46 ` Simon Marchi
2017-11-23 14:09 ` Pedro Alves
1 sibling, 1 reply; 19+ messages in thread
From: Joel Brobecker @ 2017-11-22 22:20 UTC (permalink / raw)
To: Xavier Roirand; +Cc: gdb-patches, Simon Marchi, Jerome Guitton
Hey Xavier,
Could you look at Simon's message below, and maybe review/test
his changes?
Thank you!
On Wed, Nov 22, 2017 at 11:41:04AM -0500, Simon Marchi wrote:
> There are currently multiple definitions of private_inferior, defined in
> remote.c and darwin-nat.h. The patch that poisons XNEW and friends for
> non-POD types trips on that, because private_inferior is freed in
> ~inferior(), where it is an opaque type. Since the compiler can't tell
> whether the type is POD, it gives an error. Also, we can't start using
> C++ features in these structures (make them non-POD) as long as there
> are multiple definitions with the same name. For these reasons, this
> patch makes a class hierarchy, with private_inferior being the abstract
> base class, and darwin_inferior & remote_inferior inheriting from it.
> Destruction is done through the virtual destructor.
>
> I stumbled on some suspicious code in the darwin implementation though.
> darwin_check_new_threads does an XCNEW(darwin_thread_t) when it finds a
> new thread, allocating a new structure for it (darwin_thread_t is a
> typedef for private_thread_info). It then VEC_safe_pushes it in a
> vector defined as DEF_VEC_O (a vector of objects). This means that the
> structure content gets copied in the vector. The thread_info object is
> created with the XCNEW'ed structure as the private thread info, while
> the rest of the code works with the instance in the vector. We have
> therefore two distinct instances of darwin_thread_t/private_thread_info
> for each thread. This is not really a problem in practice, because
> thread_info::priv is not used in the darwin code. I still find it weird
> and far from ideal, so I tried to fix it by changing the vector to be a
> vector of pointers. There should now be a single instance of the
> structure for each thread. The deallocation of the
> darwin_thread_t/private_thread_info structure is done by the thread_info
> destructor.
>
> I am able to build on macOS, but not really test, since the port seems a
> bit broken. I am not able to debug reliably on the machine I have
> access to, which runs macOS 10.12.6.
>
> gdb/ChangeLog:
>
> * inferior.h (private_inferior): Define structure type, add
> virtual pure destructor.
> (inferior) <priv>: Change type to unique_ptr.
> * inferior.c (private_inferior::~private_inferior): Provide
> default implementation.
> (inferior::~inferior): Don't free priv field.
> (exit_inferior_1): Likewise.
> * darwin-nat.h (struct darwin_exception_info): Initialize fields.
> (darwin_exception_info): Remove typedef.
> (DEF_VEC_O (darwin_thread_t)); Remove.
> (private_inferior): Rename to ...
> (darwin_private_inferior): ... this, extend private_inferior.
> <threads>: Change type to std::vector of darwin_thread_t pointers.
> * darwin-nat.c (darwin_check_new_threads): Adjust.
> (find_inferior_task_it): Adjust.
> (darwin_find_thread); Adjust.
> (darwin_suspend_inferior): Adjust.
> (darwin_resume_inferior): Adjust.
> (darwin_find_new_inferior): Adjust.
> (darwin_decode_notify_message): Adjust.
> (darwin_send_reply): Adjust.
> (darwin_resume_inferior_threads): Adjust.
> (darwin_suspend_inferior_threads): Adjust.
> (darwin_decode_message): Adjust.
> (darwin_wait): Adjust.
> (darwin_interrupt): Adjust.
> (darwin_deallocate_threads): Adjust.
> (darwin_mourn_inferior): Adjust, don't free private data.
> (darwin_reply_to_all_pending_messages): Adjust.
> (darwin_stop_inferior): Adjust.
> (darwin_setup_exceptions): Adjust.
> (darwin_kill_inferior): Adjust.
> (darwin_setup_request_notification): Adjust.
> (darwin_attach_pid): Adjust.
> (darwin_init_thread_list): Adjust.
> (darwin_setup_fake_stop_event): Adjust.
> (darwin_attach): Adjust.
> (darwin_detach): Adjust.
> (darwin_xfer_partial): Adjust.
> (set_enable_mach_exceptions): Adjust.
> (darwin_pid_to_exec_file): Adjust.
> (darwin_get_ada_task_ptid): Adjust.
> * darwin-nat-info.c (get_task_from_args): Adjust.
> (info_mach_ports_command): Adjust.
> (info_mach_region_command): Adjust.
> (info_mach_exceptions_command): Adjust.
> * remote.c (private_inferior): Rename to ...
> (remote_private_inferior): ... this, initialize fields.
> (get_remote_inferior); New.
> (remote_commit_resume): Use get_remote_inferior.
> (check_pending_event_prevents_wildcard_vcont_callback): Likewise.
> ---
> gdb/darwin-nat-info.c | 50 +++++----
> gdb/darwin-nat.c | 287 ++++++++++++++++++++++++++------------------------
> gdb/darwin-nat.h | 30 +++---
> gdb/inferior.c | 4 +-
> gdb/inferior.h | 9 +-
> gdb/remote.c | 30 ++++--
> 6 files changed, 218 insertions(+), 192 deletions(-)
>
> diff --git a/gdb/darwin-nat-info.c b/gdb/darwin-nat-info.c
> index 44782bf..feab181 100644
> --- a/gdb/darwin-nat-info.c
> +++ b/gdb/darwin-nat-info.c
> @@ -118,7 +118,11 @@ get_task_from_args (const char *args)
> {
> if (ptid_equal (inferior_ptid, null_ptid))
> printf_unfiltered (_("No inferior running\n"));
> - return current_inferior ()->priv->task;
> +
> + darwin_inferior *priv
> + = (darwin_inferior *) current_inferior ()->priv.get ();
> +
> + return priv->task;
> }
> if (strcmp (args, "gdb") == 0)
> return mach_task_self ();
> @@ -257,36 +261,31 @@ info_mach_ports_command (const char *args, int from_tty)
> else if (!ptid_equal (inferior_ptid, null_ptid))
> {
> struct inferior *inf = current_inferior ();
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
>
> - if (port == inf->priv->task)
> + if (port == priv->task)
> printf_unfiltered (_(" inferior-task"));
> - else if (port == inf->priv->notify_port)
> + else if (port == priv->notify_port)
> printf_unfiltered (_(" inferior-notify"));
> else
> {
> - int k;
> - darwin_thread_t *t;
> -
> - for (k = 0; k < inf->priv->exception_info.count; k++)
> - if (port == inf->priv->exception_info.ports[k])
> + for (int k = 0; k < priv->exception_info.count; k++)
> + if (port == priv->exception_info.ports[k])
> {
> printf_unfiltered (_(" inferior-excp-port"));
> break;
> }
>
> - if (inf->priv->threads)
> + for (darwin_thread_t *t : priv->threads)
> {
> - for (k = 0;
> - VEC_iterate(darwin_thread_t,
> - inf->priv->threads, k, t);
> - k++)
> - if (port == t->gdb_port)
> - {
> - printf_unfiltered (_(" inferior-thread for 0x%x"),
> - inf->priv->task);
> - break;
> - }
> + if (port == t->gdb_port)
> + {
> + printf_unfiltered (_(" inferior-thread for 0x%x"),
> + priv->task);
> + break;
> + }
> }
> +
> }
> }
> }
> @@ -738,7 +737,8 @@ info_mach_region_command (const char *exp, int from_tty)
> error (_("Inferior not available"));
>
> inf = current_inferior ();
> - darwin_debug_region (inf->priv->task, address);
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> + darwin_debug_region (priv->task, address);
> }
>
> static void
> @@ -807,7 +807,11 @@ info_mach_exceptions_command (const char *args, int from_tty)
> {
> if (ptid_equal (inferior_ptid, null_ptid))
> printf_unfiltered (_("No inferior running\n"));
> - disp_exception (¤t_inferior ()->priv->exception_info);
> +
> + darwin_inferior *priv
> + = (darwin_inferior *) current_inferior ()->priv.get ();
> +
> + disp_exception (&priv->exception_info);
> return;
> }
> else if (strcmp (args, "host") == 0)
> @@ -830,8 +834,10 @@ info_mach_exceptions_command (const char *args, int from_tty)
> printf_unfiltered (_("No inferior running\n"));
> inf = current_inferior ();
>
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> kret = task_get_exception_ports
> - (inf->priv->task, EXC_MASK_ALL, info.masks,
> + (priv->task, EXC_MASK_ALL, info.masks,
> &info.count, info.ports, info.behaviors, info.flavors);
> MACH_CHECK_ERROR (kret);
> disp_exception (&info);
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 6e420c4..f4ed455 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -279,13 +279,12 @@ static void
> darwin_check_new_threads (struct inferior *inf)
> {
> kern_return_t kret;
> - unsigned int i;
> thread_array_t thread_list;
> unsigned int new_nbr;
> unsigned int old_nbr;
> unsigned int new_ix, old_ix;
> - darwin_inferior *darwin_inf = inf->priv;
> - VEC (darwin_thread_t) *thread_vec;
> + darwin_inferior *darwin_inf = (darwin_inferior *) inf->priv.get ();
> + std::vector<darwin_thread_t *> new_thread_vec;
>
> /* Get list of threads. */
> kret = task_threads (darwin_inf->task, &thread_list, &new_nbr);
> @@ -297,17 +296,15 @@ darwin_check_new_threads (struct inferior *inf)
> if (new_nbr > 1)
> qsort (thread_list, new_nbr, sizeof (thread_t), cmp_thread_t);
>
> - if (darwin_inf->threads)
> - old_nbr = VEC_length (darwin_thread_t, darwin_inf->threads);
> - else
> - old_nbr = 0;
> + old_nbr = darwin_inf->threads.size ();
>
> /* Quick check for no changes. */
> if (old_nbr == new_nbr)
> {
> + size_t i;
> +
> for (i = 0; i < new_nbr; i++)
> - if (thread_list[i]
> - != VEC_index (darwin_thread_t, darwin_inf->threads, i)->gdb_port)
> + if (thread_list[i] != darwin_inf->threads[i]->gdb_port)
> break;
> if (i == new_nbr)
> {
> @@ -328,15 +325,15 @@ darwin_check_new_threads (struct inferior *inf)
> }
>
> /* Full handling: detect new threads, remove dead threads. */
> - thread_vec = VEC_alloc (darwin_thread_t, new_nbr);
> +
> + new_thread_vec.reserve (new_nbr);
>
> for (new_ix = 0, old_ix = 0; new_ix < new_nbr || old_ix < old_nbr;)
> {
> - thread_t new_id = (new_ix < new_nbr) ?
> - thread_list[new_ix] : THREAD_NULL;
> - darwin_thread_t *old = (old_ix < old_nbr) ?
> - VEC_index (darwin_thread_t, darwin_inf->threads, old_ix) : NULL;
> - thread_t old_id = old ? old->gdb_port : THREAD_NULL;
> + thread_t new_id = (new_ix < new_nbr) ? thread_list[new_ix] : THREAD_NULL;
> + darwin_thread_t *old
> + = (old_ix < old_nbr) ? darwin_inf->threads[old_ix] : NULL;
> + thread_t old_id = old != NULL ? old->gdb_port : THREAD_NULL;
>
> inferior_debug
> (12, _(" new_ix:%d/%d, old_ix:%d/%d, new_id:0x%x old_id:0x%x\n"),
> @@ -345,7 +342,7 @@ darwin_check_new_threads (struct inferior *inf)
> if (old_id == new_id)
> {
> /* Thread still exist. */
> - VEC_safe_push (darwin_thread_t, thread_vec, old);
> + new_thread_vec.push_back (old);
> new_ix++;
> old_ix++;
>
> @@ -368,13 +365,13 @@ darwin_check_new_threads (struct inferior *inf)
> /* A thread was created. */
> struct private_thread_info *pti;
>
> - pti = XCNEW (struct private_thread_info);
> + pti = XCNEW (darwin_thread_t);
> pti->gdb_port = new_id;
> pti->msg_state = DARWIN_RUNNING;
>
> /* Add the new thread. */
> add_thread_with_info (ptid_build (inf->pid, 0, new_id), pti);
> - VEC_safe_push (darwin_thread_t, thread_vec, pti);
> + new_thread_vec.push_back (pti);
> new_ix++;
> continue;
> }
> @@ -390,9 +387,7 @@ darwin_check_new_threads (struct inferior *inf)
> gdb_assert_not_reached ("unexpected thread case");
> }
>
> - if (darwin_inf->threads)
> - VEC_free (darwin_thread_t, darwin_inf->threads);
> - darwin_inf->threads = thread_vec;
> + darwin_inf->threads = std::move (new_thread_vec);
>
> /* Deallocate the buffer. */
> kret = vm_deallocate (gdb_task, (vm_address_t) thread_list,
> @@ -403,7 +398,9 @@ darwin_check_new_threads (struct inferior *inf)
> static int
> find_inferior_task_it (struct inferior *inf, void *port_ptr)
> {
> - return inf->priv->task == *(task_t *)port_ptr;
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> + return priv->task == *(task_t *)port_ptr;
> }
>
> static int
> @@ -430,14 +427,14 @@ darwin_find_inferior_by_pid (int pid)
> static darwin_thread_t *
> darwin_find_thread (struct inferior *inf, thread_t thread)
> {
> - darwin_thread_t *t;
> - int k;
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> + for (darwin_thread_t *t : priv->threads)
> + {
> + if (t->gdb_port == thread)
> + return t;
> + }
>
> - for (k = 0;
> - VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
> - k++)
> - if (t->gdb_port == thread)
> - return t;
> return NULL;
> }
>
> @@ -446,14 +443,16 @@ darwin_find_thread (struct inferior *inf, thread_t thread)
> static void
> darwin_suspend_inferior (struct inferior *inf)
> {
> - if (!inf->priv->suspended)
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> + if (!priv->suspended)
> {
> kern_return_t kret;
>
> - kret = task_suspend (inf->priv->task);
> + kret = task_suspend (priv->task);
> MACH_CHECK_ERROR (kret);
>
> - inf->priv->suspended = 1;
> + priv->suspended = 1;
> }
> }
>
> @@ -462,14 +461,16 @@ darwin_suspend_inferior (struct inferior *inf)
> static void
> darwin_resume_inferior (struct inferior *inf)
> {
> - if (inf->priv->suspended)
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> + if (priv->suspended)
> {
> kern_return_t kret;
>
> - kret = task_resume (inf->priv->task);
> + kret = task_resume (priv->task);
> MACH_CHECK_ERROR (kret);
>
> - inf->priv->suspended = 0;
> + priv->suspended = 0;
> }
> }
>
> @@ -580,11 +581,13 @@ darwin_find_new_inferior (task_t task_port, thread_t thread_port)
> if (inf == NULL)
> return NULL;
>
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> /* Deallocate saved exception ports. */
> - darwin_deallocate_exception_ports (inf->priv);
> + darwin_deallocate_exception_ports (priv);
>
> /* No need to remove dead_name notification, but still... */
> - kret = mach_port_request_notification (gdb_task, inf->priv->task,
> + kret = mach_port_request_notification (gdb_task, priv->task,
> MACH_NOTIFY_DEAD_NAME, 0,
> MACH_PORT_NULL,
> MACH_MSG_TYPE_MAKE_SEND_ONCE,
> @@ -593,9 +596,9 @@ darwin_find_new_inferior (task_t task_port, thread_t thread_port)
> MACH_CHECK_ERROR (kret);
>
> /* Replace old task port. */
> - kret = mach_port_deallocate (gdb_task, inf->priv->task);
> + kret = mach_port_deallocate (gdb_task, priv->task);
> MACH_CHECK_ERROR (kret);
> - inf->priv->task = task_port;
> + priv->task = task_port;
>
> darwin_setup_request_notification (inf);
> darwin_setup_exceptions (inf);
> @@ -789,8 +792,10 @@ darwin_decode_notify_message (mach_msg_header_t *hdr, struct inferior **pinf)
> inf = darwin_find_inferior_by_task (task_port);
> *pinf = inf;
>
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> /* Check message destination. */
> - if (inf != NULL && hdr->msgh_local_port != inf->priv->notify_port)
> + if (inf != NULL && hdr->msgh_local_port != priv->notify_port)
> return -4;
>
> return 0;
> @@ -817,6 +822,7 @@ darwin_send_reply (struct inferior *inf, darwin_thread_t *thread)
> {
> kern_return_t kret;
> mig_reply_error_t reply;
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
>
> darwin_encode_reply (&reply, &thread->event.header, KERN_SUCCESS);
>
> @@ -826,7 +832,7 @@ darwin_send_reply (struct inferior *inf, darwin_thread_t *thread)
> MACH_PORT_NULL);
> MACH_CHECK_ERROR (kret);
>
> - inf->priv->pending_messages--;
> + priv->pending_messages--;
> }
>
> static void
> @@ -889,12 +895,9 @@ darwin_resume_thread (struct inferior *inf, darwin_thread_t *thread,
> static void
> darwin_resume_inferior_threads (struct inferior *inf, int step, int nsignal)
> {
> - darwin_thread_t *thread;
> - int k;
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
>
> - for (k = 0;
> - VEC_iterate (darwin_thread_t, inf->priv->threads, k, thread);
> - k++)
> + for (darwin_thread_t *thread : priv->threads)
> darwin_resume_thread (inf, thread, step, nsignal);
> }
>
> @@ -920,24 +923,24 @@ darwin_resume_inferior_threads_it (struct inferior *inf, void *param)
> static void
> darwin_suspend_inferior_threads (struct inferior *inf)
> {
> - darwin_thread_t *thread;
> - kern_return_t kret;
> - int k;
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
>
> - for (k = 0;
> - VEC_iterate (darwin_thread_t, inf->priv->threads, k, thread);
> - k++)
> - switch (thread->msg_state)
> - {
> - case DARWIN_STOPPED:
> - case DARWIN_MESSAGE:
> - break;
> - case DARWIN_RUNNING:
> - kret = thread_suspend (thread->gdb_port);
> - MACH_CHECK_ERROR (kret);
> - thread->msg_state = DARWIN_STOPPED;
> - break;
> - }
> + for (darwin_thread_t *thread : priv->threads)
> + {
> + switch (thread->msg_state)
> + {
> + case DARWIN_STOPPED:
> + case DARWIN_MESSAGE:
> + break;
> + case DARWIN_RUNNING:
> + {
> + kern_return_t kret = thread_suspend (thread->gdb_port);
> + MACH_CHECK_ERROR (kret);
> + thread->msg_state = DARWIN_STOPPED;
> + break;
> + }
> + }
> + }
> }
>
> static void
> @@ -1045,7 +1048,10 @@ darwin_decode_message (mach_msg_header_t *hdr,
> }
> *pinf = inf;
> *pthread = thread;
> - inf->priv->pending_messages++;
> +
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> + priv->pending_messages++;
>
> status->kind = TARGET_WAITKIND_STOPPED;
> thread->msg_state = DARWIN_MESSAGE;
> @@ -1129,7 +1135,9 @@ darwin_decode_message (mach_msg_header_t *hdr,
>
> if (inf != NULL)
> {
> - if (!inf->priv->no_ptrace)
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> + if (!priv->no_ptrace)
> {
> pid_t res;
> int wstatus;
> @@ -1232,9 +1240,11 @@ darwin_wait (ptid_t ptid, struct target_waitstatus *status)
> inf = darwin_inf_fake_stop;
> darwin_inf_fake_stop = NULL;
>
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> status->kind = TARGET_WAITKIND_STOPPED;
> status->value.sig = GDB_SIGNAL_TRAP;
> - thread = VEC_index (darwin_thread_t, inf->priv->threads, 0);
> + thread = priv->threads[0];
> thread->msg_state = DARWIN_STOPPED;
> return ptid_build (inf->pid, 0, thread->gdb_port);
> }
> @@ -1336,9 +1346,10 @@ static void
> darwin_interrupt (struct target_ops *self, ptid_t t)
> {
> struct inferior *inf = current_inferior ();
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
>
> /* FIXME: handle in no_ptrace mode. */
> - gdb_assert (!inf->priv->no_ptrace);
> + gdb_assert (!priv->no_ptrace);
> kill (inf->pid, SIGINT);
> }
>
> @@ -1347,27 +1358,22 @@ darwin_interrupt (struct target_ops *self, ptid_t t)
> static void
> darwin_deallocate_threads (struct inferior *inf)
> {
> - if (inf->priv->threads)
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> + for (darwin_thread_t *t : priv->threads)
> {
> - kern_return_t kret;
> - int k;
> - darwin_thread_t *t;
> - for (k = 0;
> - VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
> - k++)
> - {
> - kret = mach_port_deallocate (gdb_task, t->gdb_port);
> - MACH_CHECK_ERROR (kret);
> - }
> - VEC_free (darwin_thread_t, inf->priv->threads);
> - inf->priv->threads = NULL;
> + kern_return_t kret = mach_port_deallocate (gdb_task, t->gdb_port);
> + MACH_CHECK_ERROR (kret);
> }
> +
> + priv->threads.clear ();
> }
>
> static void
> darwin_mourn_inferior (struct target_ops *ops)
> {
> struct inferior *inf = current_inferior ();
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> kern_return_t kret;
> mach_port_t prev;
> int i;
> @@ -1377,18 +1383,18 @@ darwin_mourn_inferior (struct target_ops *ops)
>
> /* Remove notify_port from darwin_port_set. */
> kret = mach_port_move_member (gdb_task,
> - inf->priv->notify_port, MACH_PORT_NULL);
> + priv->notify_port, MACH_PORT_NULL);
> MACH_CHECK_ERROR (kret);
>
> /* Remove task port dead_name notification. */
> - kret = mach_port_request_notification (gdb_task, inf->priv->task,
> + kret = mach_port_request_notification (gdb_task, priv->task,
> MACH_NOTIFY_DEAD_NAME, 0,
> MACH_PORT_NULL,
> MACH_MSG_TYPE_MAKE_SEND_ONCE,
> &prev);
> /* This can fail if the task is dead. */
> inferior_debug (4, "task=0x%x, prev=0x%x, notify_port=0x%x\n",
> - inf->priv->task, prev, inf->priv->notify_port);
> + priv->task, prev, priv->notify_port);
>
> if (kret == KERN_SUCCESS)
> {
> @@ -1397,17 +1403,16 @@ darwin_mourn_inferior (struct target_ops *ops)
> }
>
> /* Destroy notify_port. */
> - kret = mach_port_destroy (gdb_task, inf->priv->notify_port);
> + kret = mach_port_destroy (gdb_task, priv->notify_port);
> MACH_CHECK_ERROR (kret);
>
> /* Deallocate saved exception ports. */
> - darwin_deallocate_exception_ports (inf->priv);
> + darwin_deallocate_exception_ports (priv);
>
> /* Deallocate task port. */
> - kret = mach_port_deallocate (gdb_task, inf->priv->task);
> + kret = mach_port_deallocate (gdb_task, priv->task);
> MACH_CHECK_ERROR (kret);
>
> - xfree (inf->priv);
> inf->priv = NULL;
>
> inf_child_mourn_inferior (ops);
> @@ -1416,12 +1421,9 @@ darwin_mourn_inferior (struct target_ops *ops)
> static void
> darwin_reply_to_all_pending_messages (struct inferior *inf)
> {
> - int k;
> - darwin_thread_t *t;
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
>
> - for (k = 0;
> - VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
> - k++)
> + for (darwin_thread_t *t : priv->threads)
> {
> if (t->msg_state == DARWIN_MESSAGE)
> darwin_resume_thread (inf, t, 0, 0);
> @@ -1436,6 +1438,7 @@ darwin_stop_inferior (struct inferior *inf)
> kern_return_t kret;
> int status;
> int res;
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
>
> gdb_assert (inf != NULL);
>
> @@ -1443,7 +1446,7 @@ darwin_stop_inferior (struct inferior *inf)
>
> darwin_reply_to_all_pending_messages (inf);
>
> - if (inf->priv->no_ptrace)
> + if (priv->no_ptrace)
> return;
>
> res = kill (inf->pid, SIGSTOP);
> @@ -1512,11 +1515,12 @@ darwin_deallocate_exception_ports (darwin_inferior *inf)
> static void
> darwin_setup_exceptions (struct inferior *inf)
> {
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> kern_return_t kret;
> int traps_expected;
> exception_mask_t mask;
>
> - kret = darwin_save_exception_ports (inf->priv);
> + kret = darwin_save_exception_ports (priv);
> if (kret != KERN_SUCCESS)
> error (_("Unable to save exception ports, task_get_exception_ports"
> "returned: %d"),
> @@ -1527,7 +1531,7 @@ darwin_setup_exceptions (struct inferior *inf)
> mask = EXC_MASK_ALL;
> else
> mask = EXC_MASK_SOFTWARE | EXC_MASK_BREAKPOINT;
> - kret = task_set_exception_ports (inf->priv->task, mask, darwin_ex_port,
> + kret = task_set_exception_ports (priv->task, mask, darwin_ex_port,
> EXCEPTION_DEFAULT, THREAD_STATE_NONE);
> if (kret != KERN_SUCCESS)
> error (_("Unable to set exception ports, task_set_exception_ports"
> @@ -1539,6 +1543,7 @@ static void
> darwin_kill_inferior (struct target_ops *ops)
> {
> struct inferior *inf = current_inferior ();
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> struct target_waitstatus wstatus;
> ptid_t ptid;
> kern_return_t kret;
> @@ -1550,7 +1555,7 @@ darwin_kill_inferior (struct target_ops *ops)
>
> gdb_assert (inf != NULL);
>
> - kret = darwin_restore_exception_ports (inf->priv);
> + kret = darwin_restore_exception_ports (priv);
> MACH_CHECK_ERROR (kret);
>
> darwin_reply_to_all_pending_messages (inf);
> @@ -1573,12 +1578,13 @@ darwin_kill_inferior (struct target_ops *ops)
> static void
> darwin_setup_request_notification (struct inferior *inf)
> {
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> kern_return_t kret;
> mach_port_t prev_not;
>
> - kret = mach_port_request_notification (gdb_task, inf->priv->task,
> + kret = mach_port_request_notification (gdb_task, priv->task,
> MACH_NOTIFY_DEAD_NAME, 0,
> - inf->priv->notify_port,
> + priv->notify_port,
> MACH_MSG_TYPE_MAKE_SEND_ONCE,
> &prev_not);
> if (kret != KERN_SUCCESS)
> @@ -1607,9 +1613,10 @@ darwin_attach_pid (struct inferior *inf)
> mach_port_t prev_not;
> exception_mask_t mask;
>
> - inf->priv = XCNEW (darwin_inferior);
> + darwin_inferior *priv = new darwin_inferior;
> + inf->priv.reset (priv);
>
> - kret = task_for_pid (gdb_task, inf->pid, &inf->priv->task);
> + kret = task_for_pid (gdb_task, inf->pid, &priv->task);
> if (kret != KERN_SUCCESS)
> {
> int status;
> @@ -1626,7 +1633,7 @@ darwin_attach_pid (struct inferior *inf)
> }
>
> inferior_debug (2, _("inferior task: 0x%x, pid: %d\n"),
> - inf->priv->task, inf->pid);
> + priv->task, inf->pid);
>
> if (darwin_ex_port == MACH_PORT_NULL)
> {
> @@ -1663,14 +1670,14 @@ darwin_attach_pid (struct inferior *inf)
>
> /* Create a port to be notified when the child task terminates. */
> kret = mach_port_allocate (gdb_task, MACH_PORT_RIGHT_RECEIVE,
> - &inf->priv->notify_port);
> + &priv->notify_port);
> if (kret != KERN_SUCCESS)
> error (_("Unable to create notification port, mach_port_allocate "
> "returned: %d"),
> kret);
>
> kret = mach_port_move_member (gdb_task,
> - inf->priv->notify_port, darwin_port_set);
> + priv->notify_port, darwin_port_set);
> if (kret != KERN_SUCCESS)
> error (_("Unable to move notification port into new port set, "
> "mach_port_move_member\n"
> @@ -1708,11 +1715,11 @@ darwin_init_thread_list (struct inferior *inf)
> {
> darwin_check_new_threads (inf);
>
> - gdb_assert (inf->priv->threads != NULL);
> - gdb_assert (VEC_length (darwin_thread_t, inf->priv->threads) > 0);
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
>
> - private_thread_info *first_pti
> - = VEC_index (darwin_thread_t, inf->priv->threads, 0);
> + gdb_assert (!priv->threads.empty ());
> +
> + private_thread_info *first_pti = priv->threads.front ();
> struct thread_info *first_thread
> = thread_info_from_private_thread_info (first_pti);
>
> @@ -1849,6 +1856,7 @@ darwin_create_inferior (struct target_ops *ops,
> static void
> darwin_setup_fake_stop_event (struct inferior *inf)
> {
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> darwin_thread_t *thread;
> kern_return_t kret;
>
> @@ -1861,7 +1869,7 @@ darwin_setup_fake_stop_event (struct inferior *inf)
> as well. Otherwise, we'll try resuming it when resuming the
> inferior, and get a warning because the thread's suspend count
> is already zero, making the resume request useless. */
> - thread = VEC_index (darwin_thread_t, inf->priv->threads, 0);
> + thread = priv->threads[0];
> kret = thread_suspend (thread->gdb_port);
> MACH_CHECK_ERROR (kret);
> }
> @@ -1912,11 +1920,13 @@ darwin_attach (struct target_ops *ops, const char *args, int from_tty)
>
> darwin_init_thread_list (inf);
>
> - darwin_check_osabi (inf->priv, ptid_get_tid (inferior_ptid));
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> +
> + darwin_check_osabi (priv, ptid_get_tid (inferior_ptid));
>
> darwin_setup_fake_stop_event (inf);
>
> - inf->priv->no_ptrace = 1;
> + priv->no_ptrace = 1;
> }
>
> /* Take a program previously attached to and detaches it.
> @@ -1931,6 +1941,7 @@ darwin_detach (struct target_ops *ops, const char *args, int from_tty)
> {
> pid_t pid = ptid_get_pid (inferior_ptid);
> struct inferior *inf = current_inferior ();
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> kern_return_t kret;
> int res;
>
> @@ -1938,13 +1949,13 @@ darwin_detach (struct target_ops *ops, const char *args, int from_tty)
> target_announce_detach (from_tty);
>
> /* If ptrace() is in use, stop the process. */
> - if (!inf->priv->no_ptrace)
> + if (!priv->no_ptrace)
> darwin_stop_inferior (inf);
>
> - kret = darwin_restore_exception_ports (inf->priv);
> + kret = darwin_restore_exception_ports (priv);
> MACH_CHECK_ERROR (kret);
>
> - if (!inf->priv->no_ptrace)
> + if (!priv->no_ptrace)
> {
> res = PTRACE (PT_DETACH, inf->pid, 0, 0);
> if (res != 0)
> @@ -1957,7 +1968,7 @@ darwin_detach (struct target_ops *ops, const char *args, int from_tty)
> /* When using ptrace, we have just performed a PT_DETACH, which
> resumes the inferior. On the other hand, when we are not using
> ptrace, we need to resume its execution ourselves. */
> - if (inf->priv->no_ptrace)
> + if (priv->no_ptrace)
> darwin_resume_inferior (inf);
>
> darwin_mourn_inferior (ops);
> @@ -2189,6 +2200,7 @@ darwin_xfer_partial (struct target_ops *ops,
> ULONGEST offset, ULONGEST len, ULONGEST *xfered_len)
> {
> struct inferior *inf = current_inferior ();
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
>
> inferior_debug
> (8, _("darwin_xfer_partial(%s, %s, rbuf=%s, wbuf=%s) pid=%u\n"),
> @@ -2200,7 +2212,7 @@ darwin_xfer_partial (struct target_ops *ops,
> {
> case TARGET_OBJECT_MEMORY:
> {
> - int l = darwin_read_write_inferior (inf->priv->task, offset,
> + int l = darwin_read_write_inferior (priv->task, offset,
> readbuf, writebuf, len);
>
> if (l == 0)
> @@ -2219,7 +2231,7 @@ darwin_xfer_partial (struct target_ops *ops,
> /* Support only read. */
> return TARGET_XFER_E_IO;
> }
> - return darwin_read_dyld_info (inf->priv->task, offset, readbuf, len,
> + return darwin_read_dyld_info (priv->task, offset, readbuf, len,
> xfered_len);
> #endif
> default:
> @@ -2235,6 +2247,7 @@ set_enable_mach_exceptions (const char *args, int from_tty,
> if (!ptid_equal (inferior_ptid, null_ptid))
> {
> struct inferior *inf = current_inferior ();
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> exception_mask_t mask;
> kern_return_t kret;
>
> @@ -2242,10 +2255,10 @@ set_enable_mach_exceptions (const char *args, int from_tty,
> mask = EXC_MASK_ALL;
> else
> {
> - darwin_restore_exception_ports (inf->priv);
> + darwin_restore_exception_ports (priv);
> mask = EXC_MASK_SOFTWARE | EXC_MASK_BREAKPOINT;
> }
> - kret = task_set_exception_ports (inf->priv->task, mask, darwin_ex_port,
> + kret = task_set_exception_ports (priv->task, mask, darwin_ex_port,
> EXCEPTION_DEFAULT, THREAD_STATE_NONE);
> MACH_CHECK_ERROR (kret);
> }
> @@ -2267,10 +2280,8 @@ darwin_pid_to_exec_file (struct target_ops *self, int pid)
> static ptid_t
> darwin_get_ada_task_ptid (struct target_ops *self, long lwp, long thread)
> {
> - int i;
> - darwin_thread_t *t;
> - int k;
> struct inferior *inf = current_inferior ();
> + darwin_inferior *priv = (darwin_inferior *) inf->priv.get ();
> kern_return_t kret;
> mach_port_name_array_t names;
> mach_msg_type_number_t names_count;
> @@ -2279,16 +2290,16 @@ darwin_get_ada_task_ptid (struct target_ops *self, long lwp, long thread)
> long res = 0;
>
> /* First linear search. */
> - for (k = 0;
> - VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
> - k++)
> - if (t->inf_port == lwp)
> - return ptid_build (ptid_get_pid (inferior_ptid), 0, t->gdb_port);
> + for (darwin_thread_t *t : priv->threads)
> + {
> + if (t->inf_port == lwp)
> + return ptid_build (ptid_get_pid (inferior_ptid), 0, t->gdb_port);
> + }
>
> /* Maybe the port was never extract. Do it now. */
>
> /* First get inferior port names. */
> - kret = mach_port_names (inf->priv->task, &names, &names_count, &types,
> + kret = mach_port_names (priv->task, &names, &names_count, &types,
> &types_count);
> MACH_CHECK_ERROR (kret);
> if (kret != KERN_SUCCESS)
> @@ -2297,29 +2308,29 @@ darwin_get_ada_task_ptid (struct target_ops *self, long lwp, long thread)
> /* For each name, copy the right in the gdb space and then compare with
> our view of the inferior threads. We don't forget to deallocate the
> right. */
> - for (i = 0; i < names_count; i++)
> + for (int i = 0; i < names_count; i++)
> {
> mach_port_t local_name;
> mach_msg_type_name_t local_type;
>
> /* We just need to know the corresponding name in gdb name space.
> So extract and deallocate the right. */
> - kret = mach_port_extract_right (inf->priv->task, names[i],
> + kret = mach_port_extract_right (priv->task, names[i],
> MACH_MSG_TYPE_COPY_SEND,
> &local_name, &local_type);
> if (kret != KERN_SUCCESS)
> continue;
> mach_port_deallocate (gdb_task, local_name);
>
> - for (k = 0;
> - VEC_iterate (darwin_thread_t, inf->priv->threads, k, t);
> - k++)
> - if (t->gdb_port == local_name)
> - {
> - t->inf_port = names[i];
> - if (names[i] == lwp)
> - res = t->gdb_port;
> - }
> + for (darwin_thread_t *t : priv->threads)
> + {
> + if (t->gdb_port == local_name)
> + {
> + t->inf_port = names[i];
> + if (names[i] == lwp)
> + res = t->gdb_port;
> + }
> + }
> }
>
> vm_deallocate (gdb_task, (vm_address_t) names,
> diff --git a/gdb/darwin-nat.h b/gdb/darwin-nat.h
> index 77feb0e..24c10b5 100644
> --- a/gdb/darwin-nat.h
> +++ b/gdb/darwin-nat.h
> @@ -26,21 +26,20 @@
> struct darwin_exception_info
> {
> /* Exceptions handled by the port. */
> - exception_mask_t masks[EXC_TYPES_COUNT];
> + exception_mask_t masks[EXC_TYPES_COUNT] {};
>
> /* Ports receiving exception messages. */
> - mach_port_t ports[EXC_TYPES_COUNT];
> + mach_port_t ports[EXC_TYPES_COUNT] {};
>
> /* Type of messages sent. */
> - exception_behavior_t behaviors[EXC_TYPES_COUNT];
> + exception_behavior_t behaviors[EXC_TYPES_COUNT] {};
>
> /* Type of state to be sent. */
> - thread_state_flavor_t flavors[EXC_TYPES_COUNT];
> + thread_state_flavor_t flavors[EXC_TYPES_COUNT] {};
>
> /* Number of elements set. */
> - mach_msg_type_number_t count;
> + mach_msg_type_number_t count = 0;
> };
> -typedef struct darwin_exception_info darwin_exception_info;
>
> struct darwin_exception_msg
> {
> @@ -95,36 +94,31 @@ struct private_thread_info
> };
> typedef struct private_thread_info darwin_thread_t;
>
> -/* Define the threads vector type. */
> -DEF_VEC_O (darwin_thread_t);
> -
> -
> /* Describe an inferior. */
> -struct private_inferior
> +struct darwin_inferior : public private_inferior
> {
> /* Corresponding task port. */
> - task_t task;
> + task_t task = 0;
>
> /* Port which will receive the dead-name notification for the task port.
> This is used to detect the death of the task. */
> - mach_port_t notify_port;
> + mach_port_t notify_port = 0;
>
> /* Initial exception handling. */
> darwin_exception_info exception_info;
>
> /* Number of messages that have been received but not yet replied. */
> - unsigned int pending_messages;
> + unsigned int pending_messages = 0;
>
> /* Set if inferior is not controlled by ptrace(2) but through Mach. */
> - unsigned char no_ptrace;
> + bool no_ptrace = false;
>
> /* True if this task is suspended. */
> - unsigned char suspended;
> + bool suspended = false;
>
> /* Sorted vector of known threads. */
> - VEC(darwin_thread_t) *threads;
> + std::vector<darwin_thread_t *> threads;
> };
> -typedef struct private_inferior darwin_inferior;
>
> /* Exception port. */
> extern mach_port_t darwin_ex_port;
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index bcac981..9b3043d 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -71,6 +71,8 @@ set_current_inferior (struct inferior *inf)
> current_inferior_ = inf;
> }
>
> +private_inferior::~private_inferior () = default;
> +
> inferior::~inferior ()
> {
> inferior *inf = this;
> @@ -80,7 +82,6 @@ inferior::~inferior ()
> xfree (inf->args);
> xfree (inf->terminal);
> target_desc_info_free (inf->tdesc_info);
> - xfree (inf->priv);
> }
>
> inferior::inferior (int pid_)
> @@ -209,7 +210,6 @@ exit_inferior_1 (struct inferior *inftoex, int silent)
>
> inf->pid = 0;
> inf->fake_pid_p = 0;
> - xfree (inf->priv);
> inf->priv = NULL;
>
> if (inf->vfork_parent != NULL)
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 37252a6..0705dd9 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -263,7 +263,12 @@ enum stop_kind
> #define ON_STACK 1
> #define AT_ENTRY_POINT 4
>
> -struct private_inferior;
> +/* Base class for target-specific inferior data. */
> +
> +struct private_inferior
> +{
> + virtual ~private_inferior () = 0;
> +};
>
> /* Inferior process specific part of `struct infcall_control_state'.
>
> @@ -403,7 +408,7 @@ public:
> bool needs_setup = false;
>
> /* Private data used by the target vector implementation. */
> - private_inferior *priv = NULL;
> + std::unique_ptr<private_inferior> priv;
>
> /* HAS_EXIT_CODE is true if the inferior exited with an exit code.
> In this case, the EXIT_CODE field is also valid. */
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 62ac055..f328080 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -5826,12 +5826,22 @@ static int is_pending_fork_parent_thread (struct thread_info *thread);
>
> /* Private per-inferior info for target remote processes. */
>
> -struct private_inferior
> +struct remote_inferior : public private_inferior
> {
> /* Whether we can send a wildcard vCont for this process. */
> - int may_wildcard_vcont;
> + bool may_wildcard_vcont = true;
> };
>
> +/* Get the remote private inferior data associated to INF. */
> +
> +static remote_inferior *get_remote_inferior (inferior *inf)
> +{
> + if (inf->priv == NULL)
> + inf->priv.reset (new remote_inferior);
> +
> + return (remote_inferior *) inf->priv.get ();
> +}
> +
> /* Structure used to track the construction of a vCont packet in the
> outgoing packet buffer. This is used to send multiple vCont
> packets if we have more actions than would fit a single packet. */
> @@ -5993,9 +6003,9 @@ remote_commit_resume (struct target_ops *ops)
> /* And assume every process is individually wildcard-able too. */
> ALL_NON_EXITED_INFERIORS (inf)
> {
> - if (inf->priv == NULL)
> - inf->priv = XNEW (struct private_inferior);
> - inf->priv->may_wildcard_vcont = 1;
> + remote_inferior *priv = get_remote_inferior (inf);
> +
> + priv->may_wildcard_vcont = true;
> }
>
> /* Check for any pending events (not reported or processed yet) and
> @@ -6008,7 +6018,7 @@ remote_commit_resume (struct target_ops *ops)
> can't wildcard that process. */
> if (!tp->executing)
> {
> - tp->inf->priv->may_wildcard_vcont = 0;
> + get_remote_inferior (tp->inf)->may_wildcard_vcont = false;
>
> /* And if we can't wildcard a process, we can't wildcard
> everything either. */
> @@ -6042,7 +6052,7 @@ remote_commit_resume (struct target_ops *ops)
>
> if (!remote_thr->last_resume_step
> && remote_thr->last_resume_sig == GDB_SIGNAL_0
> - && tp->inf->priv->may_wildcard_vcont)
> + && get_remote_inferior (tp->inf)->may_wildcard_vcont)
> {
> /* We'll send a wildcard resume instead. */
> remote_thr->vcont_resumed = 1;
> @@ -6062,7 +6072,7 @@ remote_commit_resume (struct target_ops *ops)
>
> ALL_NON_EXITED_INFERIORS (inf)
> {
> - if (inf->priv->may_wildcard_vcont)
> + if (get_remote_inferior (inf)->may_wildcard_vcont)
> {
> any_process_wildcard = 1;
> break;
> @@ -6083,7 +6093,7 @@ remote_commit_resume (struct target_ops *ops)
> {
> ALL_NON_EXITED_INFERIORS (inf)
> {
> - if (inf->priv->may_wildcard_vcont)
> + if (get_remote_inferior (inf)->may_wildcard_vcont)
> {
> vcont_builder_push_action (&vcont_builder,
> pid_to_ptid (inf->pid),
> @@ -6579,7 +6589,7 @@ check_pending_event_prevents_wildcard_vcont_callback
> we'd resume this process too. */
> *may_global_wildcard_vcont = 0;
> if (inf != NULL)
> - inf->priv->may_wildcard_vcont = 0;
> + get_remote_inferior (inf)->may_wildcard_vcont = false;
>
> return 1;
> }
> --
> 2.7.4
--
Joel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/4] Create private_inferior class hierarchy
2017-11-22 22:20 ` Joel Brobecker
@ 2017-11-24 15:46 ` Simon Marchi
2017-11-24 21:31 ` Joel Brobecker
0 siblings, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-11-24 15:46 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Xavier Roirand, gdb-patches, Simon Marchi, Jerome Guitton
On 2017-11-22 17:20, Joel Brobecker wrote:
> Hey Xavier,
>
> Could you look at Simon's message below, and maybe review/test
> his changes?
Hi Joel and Xavier,
I pushed this patch (or rather, v2 of this patch), but testing on macOS
would still be relevant (Joel, I assume you were referring to the darwin
changes?).
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] Create private_inferior class hierarchy
2017-11-24 15:46 ` Simon Marchi
@ 2017-11-24 21:31 ` Joel Brobecker
0 siblings, 0 replies; 19+ messages in thread
From: Joel Brobecker @ 2017-11-24 21:31 UTC (permalink / raw)
To: Simon Marchi; +Cc: Xavier Roirand, gdb-patches, Simon Marchi, Jerome Guitton
> I pushed this patch (or rather, v2 of this patch), but testing on macOS
> would still be relevant (Joel, I assume you were referring to the darwin
> changes?).
Yes, that's correct. We have a machine running MacOS, so we can
do some testing....
--
Joel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] Create private_inferior class hierarchy
2017-11-22 16:41 ` [PATCH 1/4] Create private_inferior class hierarchy Simon Marchi
2017-11-22 22:20 ` Joel Brobecker
@ 2017-11-23 14:09 ` Pedro Alves
2017-11-23 16:17 ` Simon Marchi
1 sibling, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2017-11-23 14:09 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 11/22/2017 04:41 PM, Simon Marchi wrote:
> diff --git a/gdb/darwin-nat-info.c b/gdb/darwin-nat-info.c
> index 44782bf..feab181 100644
> --- a/gdb/darwin-nat-info.c
> +++ b/gdb/darwin-nat-info.c
> @@ -118,7 +118,11 @@ get_task_from_args (const char *args)
> {
> if (ptid_equal (inferior_ptid, null_ptid))
> printf_unfiltered (_("No inferior running\n"));
> - return current_inferior ()->priv->task;
> +
> + darwin_inferior *priv
> + = (darwin_inferior *) current_inferior ()->priv.get ();
> +
How about adding a function like this:
static darwin_inferior *
get_darwin_inferior (inferior *inf)
{
return static_cast<darwin_inferior *> (inf->priv.get ());
}
and then using it throughout instead of the casts.
>
> +/* Get the remote private inferior data associated to INF. */
> +
> +static remote_inferior *get_remote_inferior (inferior *inf)
Ah, you've added something like it to remote.c
Missing line break before get_, though.
> +{
> + if (inf->priv == NULL)
> + inf->priv.reset (new remote_inferior);
> +
> + return (remote_inferior *) inf->priv.get ();
static_cast would ensure that remote_inferior actually
inherits private_inferior.
> +}
> +
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/4] Create private_inferior class hierarchy
2017-11-23 14:09 ` Pedro Alves
@ 2017-11-23 16:17 ` Simon Marchi
0 siblings, 0 replies; 19+ messages in thread
From: Simon Marchi @ 2017-11-23 16:17 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-11-23 09:09, Pedro Alves wrote:
> On 11/22/2017 04:41 PM, Simon Marchi wrote:
>
>> diff --git a/gdb/darwin-nat-info.c b/gdb/darwin-nat-info.c
>> index 44782bf..feab181 100644
>> --- a/gdb/darwin-nat-info.c
>> +++ b/gdb/darwin-nat-info.c
>> @@ -118,7 +118,11 @@ get_task_from_args (const char *args)
>> {
>> if (ptid_equal (inferior_ptid, null_ptid))
>> printf_unfiltered (_("No inferior running\n"));
>> - return current_inferior ()->priv->task;
>> +
>> + darwin_inferior *priv
>> + = (darwin_inferior *) current_inferior ()->priv.get ();
>> +
>
> How about adding a function like this:
>
> static darwin_inferior *
> get_darwin_inferior (inferior *inf)
> {
> return static_cast<darwin_inferior *> (inf->priv.get ());
> }
>
> and then using it throughout instead of the casts.
Good idea.
>>
>> +/* Get the remote private inferior data associated to INF. */
>> +
>> +static remote_inferior *get_remote_inferior (inferior *inf)
>
> Ah, you've added something like it to remote.c
> Missing line break before get_, though.
Oops, thanks.
>> +{
>> + if (inf->priv == NULL)
>> + inf->priv.reset (new remote_inferior);
>> +
>> + return (remote_inferior *) inf->priv.get ();
>
> static_cast would ensure that remote_inferior actually
> inherits private_inferior.
Didn't know static_cast checked that, cool! I'll use that.
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/4] Poison XNEW and friends for types that should use new/delete
2017-11-22 16:41 [PATCH 0/4] Poison XNEW and friends for non-POD types Simon Marchi
2017-11-22 16:41 ` [PATCH 1/4] Create private_inferior class hierarchy Simon Marchi
@ 2017-11-22 16:42 ` Simon Marchi
2017-11-23 15:02 ` Pedro Alves
2017-11-22 16:42 ` [PATCH 2/4] remote: C++ify thread_item and threads_listing_context Simon Marchi
2017-11-22 16:42 ` [PATCH 3/4] Create private_thread_info hierarchy Simon Marchi
3 siblings, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-11-22 16:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@polymtl.ca>
This patch (finally!) makes it so that trying to use XNEW with a type
that requires "new" will cause a compilation error. The criterion I
initially used to allow a type to use XNEW (which calls malloc in the
end) was std::is_trivially_constructible, but then realized that gcc 4.8
did not have it. Instead, I went with:
using IsMallocatable = std::is_pod<T>;
which is just a bit more strict, which doesn't hurt. A similar thing is
done for macros that free instead of allocated, the criterion is:
using IsFreeable = gdb::Or<std::is_trivially_destructible<T>, std::is_void<T>>;
For simplicity, we could also do for std::is_pod for IsFreeable as well,
if you prefer.
I chose to put static_assert in the functions, instead of using
gdb::Requires in the template as SFINAE, because it allows to put a
message, which I think makes the compiler error more understandable.
With gdb::Requires, the error is:
In file included from /home/simark/src/binutils-gdb/gdb/common/common-utils.h:26:0,
from /home/simark/src/binutils-gdb/gdb/common/common-defs.h:78,
from /home/simark/src/binutils-gdb/gdb/defs.h:28,
from /home/simark/src/binutils-gdb/gdb/lala.c:1:
/home/simark/src/binutils-gdb/gdb/lala.c: In function ‘void foo()’:
/home/simark/src/binutils-gdb/gdb/common/poison.h:108:25: error: no matching function for call to ‘xnew<bar>()’
#define XNEW(T) xnew<T>()
^
/home/simark/src/binutils-gdb/gdb/lala.c:13:3: note: in expansion of macro ‘XNEW’
XNEW(bar);
^~~~
/home/simark/src/binutils-gdb/gdb/common/poison.h:101:1: note: candidate: template<class T, typename std::enable_if<std::is_trivially_constructible<_Tp>::value, void>::type <anonymous> > T* xnew()
xnew ()
^~~~
/home/simark/src/binutils-gdb/gdb/common/poison.h:101:1: note: template argument deduction/substitution failed:
/home/simark/src/binutils-gdb/gdb/common/poison.h:108:25: note: couldn't deduce template parameter ‘<anonymous>’
#define XNEW(T) xnew<T>()
^
/home/simark/src/binutils-gdb/gdb/lala.c:13:3: note: in expansion of macro ‘XNEW’
XNEW(bar);
^~~~
and with static_assert:
In file included from /home/simark/src/binutils-gdb/gdb/common/common-utils.h:26:0,
from /home/simark/src/binutils-gdb/gdb/common/common-defs.h:78,
from /home/simark/src/binutils-gdb/gdb/defs.h:28,
from /home/simark/src/binutils-gdb/gdb/lala.c:1:
/home/simark/src/binutils-gdb/gdb/common/poison.h: In instantiation of ‘T* xnew() [with T = bar]’:
/home/simark/src/binutils-gdb/gdb/lala.c:13:3: required from here
/home/simark/src/binutils-gdb/gdb/common/poison.h:103:3: error: static assertion failed: Trying to use XNEW with a non-POD data type. Use operator new instead.
static_assert (IsMallocatable<T>::value, "Trying to use XNEW with a non-POD\
^~~~~~~~~~~~~
I think the first one is more likely to just make people yell at their
screen, especially those less used to C++.
Generated-code-wise, it adds one more function call (xnew<T>) when using
XNEW and building with -O0, but it all goes away with optimizations
enabled.
gdb/ChangeLog:
* common/common-utils.h: Include poison.h.
(xfree): Remove declaration, add definition with static_assert.
* common/common-utils.c (xfree): Remove.
* common/poison.h (IsMallocatable): Define.
(IsFreeable): Define.
(free): Delete for non-freeable types.
(xnew): New.
(XNEW): Undef and redefine.
(xcnew): New.
(XCNEW): Undef and redefine.
(xdelete): New.
(XDELETE): Undef and redefine.
(xnewvec): New.
(XNEWVEC): Undef and redefine.
(xcnewvec): New.
(XCNEWVEC): Undef and redefine.
(xresizevec): New.
(XRESIZEVEC): Undef and redefine.
(xdeletevec): New.
(XDELETEVEC): Undef and redefine.
(xnewvar): New.
(XNEWVAR): Undef and redefine.
(xcnewvar): New.
(XCNEWVAR): Undef and redefine.
(xresizevar): New.
(XRESIZEVAR): Undef and redefine.
---
gdb/common/common-utils.c | 7 ---
gdb/common/common-utils.h | 14 ++++-
gdb/common/poison.h | 132 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 145 insertions(+), 8 deletions(-)
diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 7139302..66d6161 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -95,13 +95,6 @@ xzalloc (size_t size)
}
void
-xfree (void *ptr)
-{
- if (ptr != NULL)
- free (ptr); /* ARI: free */
-}
-
-void
xmalloc_failed (size_t size)
{
malloc_failure (size);
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 4926a32..feb4790 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -23,6 +23,8 @@
#include <string>
#include <vector>
+#include "poison.h"
+
/* If possible, define FUNCTION_NAME, a macro containing the name of
the function being defined. Since this macro may not always be
defined, all uses must be protected by appropriate macro definition
@@ -47,7 +49,17 @@
/* Like xmalloc, but zero the memory. */
void *xzalloc (size_t);
-void xfree (void *);
+template <typename T>
+static void
+xfree (T *ptr)
+{
+ static_assert (IsFreeable<T>::value, "Trying to use xfree with a non-POD \
+data type. Use operator delete instead.");
+
+ if (ptr != NULL)
+ free (ptr); /* ARI: free */
+}
+
/* Like asprintf and vasprintf, but return the string, throw an error
if no memory. */
diff --git a/gdb/common/poison.h b/gdb/common/poison.h
index 37dd35e..de4cefa 100644
--- a/gdb/common/poison.h
+++ b/gdb/common/poison.h
@@ -84,4 +84,136 @@ void *memmove (D *dest, const S *src, size_t n) = delete;
#endif /* HAVE_IS_TRIVIALLY_COPYABLE */
+/* Poison XNEW and friends to catch usages of malloc-style allocations on
+ objects that require new/delete. */
+
+template<typename T>
+using IsMallocatable = std::is_pod<T>;
+
+template<typename T>
+using IsFreeable = gdb::Or<std::is_trivially_destructible<T>, std::is_void<T>>;
+
+template <typename T, typename = gdb::Requires<gdb::Not<IsFreeable<T>>>>
+void free (T *ptr) = delete;
+
+template<typename T>
+static T *
+xnew ()
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XNEW with a non-POD \
+data type. Use operator new instead.");
+ return XNEW (T);
+}
+
+#undef XNEW
+#define XNEW(T) xnew<T>()
+
+template<typename T>
+static T *
+xcnew ()
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XCNEW with a non-POD \
+data type. Use operator new instead.");
+ return XCNEW (T);
+}
+
+#undef XCNEW
+#define XCNEW(T) xcnew<T>()
+
+template<typename T>
+static void
+xdelete (T *p)
+{
+ static_assert (IsFreeable<T>::value, "Trying to use XDELETE with a non-POD \
+data type. Use operator delete instead.");
+ XDELETE (p);
+}
+
+#undef XDELETE
+#define XDELETE(P) xdelete (p)
+
+template<typename T>
+static T*
+xnewvec (size_t n)
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XNEWVEC with a \
+non-POD data type. Use operator new[] (or std::vector) instead.");
+ return XNEWVEC (T, n);
+}
+
+#undef XNEWVEC
+#define XNEWVEC(T, N) xnewvec<T> (N)
+
+template<typename T>
+static T*
+xcnewvec (size_t n)
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XCNEWVEC with a \
+non-POD data type. Use operator new[] (or std::vector) instead.");
+ return XCNEWVEC (T, n);
+}
+
+#undef XCNEWVEC
+#define XCNEWVEC(T, N) xcnewvec<T> (N)
+
+template<typename T>
+static T*
+xresizevec (T *p, size_t n)
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XRESIZEVEC with a \
+non-POD data type.");
+ return XRESIZEVEC (T, p, n);
+}
+
+#undef XRESIZEVEC
+#define XRESIZEVEC(T, P, N) xresizevec<T> (P, N)
+
+template<typename T>
+static void
+xdeletevec (T *p)
+{
+ static_assert (IsFreeable<T>::value, "Trying to use XDELETEVEC with a \
+non-POD data type. Use operator delete[] (or std::vector) instead.");
+ XDELETEVEC (p);
+}
+
+#undef XDELETEVEC
+#define XDELETEVEC(P) xdeletevec (P)
+
+template<typename T>
+static T*
+xnewvar (size_t s)
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XNEWVAR with a \
+non-POD data type.");
+ return XNEWVAR (T, s);;
+}
+
+#undef XNEWVAR
+#define XNEWVAR(T, S) xnewvar<T> (S)
+
+template<typename T>
+static T*
+xcnewvar (size_t s)
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XCNEWVAR with a \
+non-POD data type.");
+ return XCNEWVAR (T, s);
+}
+
+#undef XCNEWVAR
+#define XCNEWVAR(T, S) xcnewvar<T> (S)
+
+template<typename T>
+static T*
+xresizevar (T *p, size_t s)
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XRESIZEVAR with a \
+non-POD data type.");
+ return XRESIZEVAR (T, p, s);
+}
+
+#undef XRESIZEVAR
+#define XRESIZEVAR(T, P, S) xresizevar<T> (P, S)
+
#endif /* COMMON_POISON_H */
--
2.7.4
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete
2017-11-22 16:42 ` [PATCH 4/4] Poison XNEW and friends for types that should use new/delete Simon Marchi
@ 2017-11-23 15:02 ` Pedro Alves
2017-11-23 17:27 ` Simon Marchi
0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2017-11-23 15:02 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Simon Marchi
On 11/22/2017 04:41 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> This patch (finally!) makes it so that trying to use XNEW with a type
> that requires "new" will cause a compilation error.
Yay!
The criterion I
> initially used to allow a type to use XNEW (which calls malloc in the
> end) was std::is_trivially_constructible, but then realized that gcc 4.8
> did not have it. Instead, I went with:
>
Yeah, in the memset poisoning I just disabled the poisoning on older
compilers. This is the "#if HAVE_IS_TRIVIALLY_COPYABLE" check in
poison.h.
> using IsMallocatable = std::is_pod<T>;
>
Nit, I think "malloc-able" is more common than "malloc-atable".
At least, the latter sounds odd to me. Or sounds like
"m-allocatable", which is a different thing. :-)
> which is just a bit more strict, which doesn't hurt. A similar thing is
> done for macros that free instead of allocated, the criterion is:
>
> using IsFreeable = gdb::Or<std::is_trivially_destructible<T>, std::is_void<T>>;
>
> For simplicity, we could also do for std::is_pod for IsFreeable as well,
> if you prefer.
>
> I chose to put static_assert in the functions, instead of using
> gdb::Requires in the template as SFINAE, because it allows to put a
> message, which I think makes the compiler error more understandable.
> With gdb::Requires, the error is:
Right, I think static_assert is what I had suggest initially too.
SFINAE alone wouldn't sound right to me here.
The right alternative to static_assert would be SFINAE+"=delete;",
which is what we do for the memcpy/memcpy poisoning, and what
you're doing to "free".
=delete keeps the function/overload in the overload set, serving
as "honeypot", so you get an error about trying to call a
deleted function.
You could do that to xfree instead
of the static_assert, but I guess inlining xfree ends up being
a good thing.
> I think the first one is more likely to just make people yell at their
> screen, especially those less used to C++.
Yes, don't do that. :-)
>
> Generated-code-wise, it adds one more function call (xnew<T>) when using
> XNEW and building with -O0, but it all goes away with optimizations
> enabled.
Good.
>
> gdb/ChangeLog:
>
> * common/common-utils.h: Include poison.h.
> (xfree): Remove declaration, add definition with static_assert.
> * common/common-utils.c (xfree): Remove.
> * common/poison.h (IsMallocatable): Define.
> (IsFreeable): Define.
> (free): Delete for non-freeable types.
> (xnew): New.
> (XNEW): Undef and redefine.
> (xcnew): New.
> (XCNEW): Undef and redefine.
> (xdelete): New.
> (XDELETE): Undef and redefine.
> (xnewvec): New.
> (XNEWVEC): Undef and redefine.
> (xcnewvec): New.
> (XCNEWVEC): Undef and redefine.
> (xresizevec): New.
> (XRESIZEVEC): Undef and redefine.
> (xdeletevec): New.
> (XDELETEVEC): Undef and redefine.
> (xnewvar): New.
> (XNEWVAR): Undef and redefine.
> (xcnewvar): New.
> (XCNEWVAR): Undef and redefine.
> (xresizevar): New.
> (XRESIZEVAR): Undef and redefine.
> ---
> gdb/common/common-utils.c | 7 ---
> gdb/common/common-utils.h | 14 ++++-
> gdb/common/poison.h | 132 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 145 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
> index 7139302..66d6161 100644
> --- a/gdb/common/common-utils.c
> +++ b/gdb/common/common-utils.c
> @@ -95,13 +95,6 @@ xzalloc (size_t size)
> }
>
> void
> -xfree (void *ptr)
> -{
> - if (ptr != NULL)
> - free (ptr); /* ARI: free */
> -}
> -
> -void
> xmalloc_failed (size_t size)
> {
> malloc_failure (size);
> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
> index 4926a32..feb4790 100644
> --- a/gdb/common/common-utils.h
> +++ b/gdb/common/common-utils.h
> @@ -23,6 +23,8 @@
> #include <string>
> #include <vector>
>
> +#include "poison.h"
> +
> /* If possible, define FUNCTION_NAME, a macro containing the name of
> the function being defined. Since this macro may not always be
> defined, all uses must be protected by appropriate macro definition
> @@ -47,7 +49,17 @@
> /* Like xmalloc, but zero the memory. */
> void *xzalloc (size_t);
>
> -void xfree (void *);
> +template <typename T>
> +static void
> +xfree (T *ptr)
> +{
> + static_assert (IsFreeable<T>::value, "Trying to use xfree with a non-POD \
> +data type. Use operator delete instead.");
> +
> + if (ptr != NULL)
> + free (ptr); /* ARI: free */
> +}
> +
>
> /* Like asprintf and vasprintf, but return the string, throw an error
> if no memory. */
> diff --git a/gdb/common/poison.h b/gdb/common/poison.h
> index 37dd35e..de4cefa 100644
> --- a/gdb/common/poison.h
> +++ b/gdb/common/poison.h
> @@ -84,4 +84,136 @@ void *memmove (D *dest, const S *src, size_t n) = delete;
>
> #endif /* HAVE_IS_TRIVIALLY_COPYABLE */
>
> +/* Poison XNEW and friends to catch usages of malloc-style allocations on
> + objects that require new/delete. */
> +
> +template<typename T>
> +using IsMallocatable = std::is_pod<T>;
> +
> +template<typename T>
> +using IsFreeable = gdb::Or<std::is_trivially_destructible<T>, std::is_void<T>>;
> +
> +template <typename T, typename = gdb::Requires<gdb::Not<IsFreeable<T>>>>
> +void free (T *ptr) = delete;
> +
> +template<typename T>
> +static T *
> +xnew ()
> +{
> + static_assert (IsMallocatable<T>::value, "Trying to use XNEW with a non-POD \
> +data type. Use operator new instead.");
> + return XNEW (T);
> +}
> +
> +#undef XNEW
> +#define XNEW(T) xnew<T>()
> +
> +template<typename T>
> +static T *
> +xcnew ()
> +{
> + static_assert (IsMallocatable<T>::value, "Trying to use XCNEW with a non-POD \
> +data type. Use operator new instead.");
> + return XCNEW (T);
> +}
> +
> +#undef XCNEW
> +#define XCNEW(T) xcnew<T>()
> +
> +template<typename T>
> +static void
> +xdelete (T *p)
> +{
> + static_assert (IsFreeable<T>::value, "Trying to use XDELETE with a non-POD \
> +data type. Use operator delete instead.");
> + XDELETE (p);
> +}
> +
> +#undef XDELETE
> +#define XDELETE(P) xdelete (p)
> +
> +template<typename T>
> +static T*
Missing space after T in several of these functions.
> +xnewvec (size_t n)
> +{
> + static_assert (IsMallocatable<T>::value, "Trying to use XNEWVEC with a \
> +non-POD data type. Use operator new[] (or std::vector) instead.");
> + return XNEWVEC (T, n);
> +}
> +
Other than the formatting, this LGTM. Thanks a lot for doing this.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete
2017-11-23 15:02 ` Pedro Alves
@ 2017-11-23 17:27 ` Simon Marchi
2017-11-23 17:31 ` Pedro Alves
0 siblings, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-11-23 17:27 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-11-23 10:02, Pedro Alves wrote:
> On 11/22/2017 04:41 PM, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> This patch (finally!) makes it so that trying to use XNEW with a type
>> that requires "new" will cause a compilation error.
>
> Yay!
>
> The criterion I
>> initially used to allow a type to use XNEW (which calls malloc in the
>> end) was std::is_trivially_constructible, but then realized that gcc
>> 4.8
>> did not have it. Instead, I went with:
>>
>
> Yeah, in the memset poisoning I just disabled the poisoning on older
> compilers. This is the "#if HAVE_IS_TRIVIALLY_COPYABLE" check in
> poison.h.
I'm not sure if you are suggesting adding
HAVE_IS_TRIVIALLY_CONSTRUCTIBLE, but since we can use std::is_pod
easily, I think it's simpler just to use that (even if it's a bit more
strict than needed).
>> using IsMallocatable = std::is_pod<T>;
>>
>
> Nit, I think "malloc-able" is more common than "malloc-atable".
> At least, the latter sounds odd to me. Or sounds like
> "m-allocatable", which is a different thing. :-)
Agreed.
>
>> which is just a bit more strict, which doesn't hurt. A similar thing
>> is
>> done for macros that free instead of allocated, the criterion is:
>>
>> using IsFreeable = gdb::Or<std::is_trivially_destructible<T>,
>> std::is_void<T>>;
>>
>> For simplicity, we could also do for std::is_pod for IsFreeable as
>> well,
>> if you prefer.
>>
>> I chose to put static_assert in the functions, instead of using
>> gdb::Requires in the template as SFINAE, because it allows to put a
>> message, which I think makes the compiler error more understandable.
>> With gdb::Requires, the error is:
>
> Right, I think static_assert is what I had suggest initially too.
> SFINAE alone wouldn't sound right to me here.
>
> The right alternative to static_assert would be SFINAE+"=delete;",
> which is what we do for the memcpy/memcpy poisoning, and what
> you're doing to "free".
>
> =delete keeps the function/overload in the overload set, serving
> as "honeypot", so you get an error about trying to call a
> deleted function.
>
> You could do that to xfree instead
> of the static_assert, but I guess inlining xfree ends up being
> a good thing.
Ah yeah, it would probably make the message a bit clearer. Instead of
saying "I can't find a function that matches", it would say "I found a
function that matches, but you can't use it". I'll just keep
static_assert for xfree as well, since it ends up clearer anyway.
>> I think the first one is more likely to just make people yell at their
>> screen, especially those less used to C++.
>
> Yes, don't do that. :-)
>
>>
>> Generated-code-wise, it adds one more function call (xnew<T>) when
>> using
>> XNEW and building with -O0, but it all goes away with optimizations
>> enabled.
>
> Good.
>
>>
>> gdb/ChangeLog:
>>
>> * common/common-utils.h: Include poison.h.
>> (xfree): Remove declaration, add definition with static_assert.
>> * common/common-utils.c (xfree): Remove.
>> * common/poison.h (IsMallocatable): Define.
>> (IsFreeable): Define.
>> (free): Delete for non-freeable types.
>> (xnew): New.
>> (XNEW): Undef and redefine.
>> (xcnew): New.
>> (XCNEW): Undef and redefine.
>> (xdelete): New.
>> (XDELETE): Undef and redefine.
>> (xnewvec): New.
>> (XNEWVEC): Undef and redefine.
>> (xcnewvec): New.
>> (XCNEWVEC): Undef and redefine.
>> (xresizevec): New.
>> (XRESIZEVEC): Undef and redefine.
>> (xdeletevec): New.
>> (XDELETEVEC): Undef and redefine.
>> (xnewvar): New.
>> (XNEWVAR): Undef and redefine.
>> (xcnewvar): New.
>> (XCNEWVAR): Undef and redefine.
>> (xresizevar): New.
>> (XRESIZEVAR): Undef and redefine.
>> ---
>> gdb/common/common-utils.c | 7 ---
>> gdb/common/common-utils.h | 14 ++++-
>> gdb/common/poison.h | 132
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 145 insertions(+), 8 deletions(-)
>>
>> diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
>> index 7139302..66d6161 100644
>> --- a/gdb/common/common-utils.c
>> +++ b/gdb/common/common-utils.c
>> @@ -95,13 +95,6 @@ xzalloc (size_t size)
>> }
>>
>> void
>> -xfree (void *ptr)
>> -{
>> - if (ptr != NULL)
>> - free (ptr); /* ARI: free */
>> -}
>> -
>> -void
>> xmalloc_failed (size_t size)
>> {
>> malloc_failure (size);
>> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
>> index 4926a32..feb4790 100644
>> --- a/gdb/common/common-utils.h
>> +++ b/gdb/common/common-utils.h
>> @@ -23,6 +23,8 @@
>> #include <string>
>> #include <vector>
>>
>> +#include "poison.h"
>> +
>> /* If possible, define FUNCTION_NAME, a macro containing the name of
>> the function being defined. Since this macro may not always be
>> defined, all uses must be protected by appropriate macro
>> definition
>> @@ -47,7 +49,17 @@
>> /* Like xmalloc, but zero the memory. */
>> void *xzalloc (size_t);
>>
>> -void xfree (void *);
>> +template <typename T>
>> +static void
>> +xfree (T *ptr)
>> +{
>> + static_assert (IsFreeable<T>::value, "Trying to use xfree with a
>> non-POD \
>> +data type. Use operator delete instead.");
>> +
>> + if (ptr != NULL)
>> + free (ptr); /* ARI: free */
>> +}
>> +
>>
>> /* Like asprintf and vasprintf, but return the string, throw an error
>> if no memory. */
>> diff --git a/gdb/common/poison.h b/gdb/common/poison.h
>> index 37dd35e..de4cefa 100644
>> --- a/gdb/common/poison.h
>> +++ b/gdb/common/poison.h
>> @@ -84,4 +84,136 @@ void *memmove (D *dest, const S *src, size_t n) =
>> delete;
>>
>> #endif /* HAVE_IS_TRIVIALLY_COPYABLE */
>>
>> +/* Poison XNEW and friends to catch usages of malloc-style
>> allocations on
>> + objects that require new/delete. */
>> +
>> +template<typename T>
>> +using IsMallocatable = std::is_pod<T>;
>> +
>> +template<typename T>
>> +using IsFreeable = gdb::Or<std::is_trivially_destructible<T>,
>> std::is_void<T>>;
>> +
>> +template <typename T, typename =
>> gdb::Requires<gdb::Not<IsFreeable<T>>>>
>> +void free (T *ptr) = delete;
>> +
>> +template<typename T>
>> +static T *
>> +xnew ()
>> +{
>> + static_assert (IsMallocatable<T>::value, "Trying to use XNEW with a
>> non-POD \
>> +data type. Use operator new instead.");
>> + return XNEW (T);
>> +}
>> +
>> +#undef XNEW
>> +#define XNEW(T) xnew<T>()
>> +
>> +template<typename T>
>> +static T *
>> +xcnew ()
>> +{
>> + static_assert (IsMallocatable<T>::value, "Trying to use XCNEW with
>> a non-POD \
>> +data type. Use operator new instead.");
>> + return XCNEW (T);
>> +}
>> +
>> +#undef XCNEW
>> +#define XCNEW(T) xcnew<T>()
>> +
>> +template<typename T>
>> +static void
>> +xdelete (T *p)
>> +{
>> + static_assert (IsFreeable<T>::value, "Trying to use XDELETE with a
>> non-POD \
>> +data type. Use operator delete instead.");
>> + XDELETE (p);
>> +}
>> +
>> +#undef XDELETE
>> +#define XDELETE(P) xdelete (p)
>> +
>> +template<typename T>
>> +static T*
>
> Missing space after T in several of these functions.
Oops, thanks.
>> +xnewvec (size_t n)
>> +{
>> + static_assert (IsMallocatable<T>::value, "Trying to use XNEWVEC
>> with a \
>> +non-POD data type. Use operator new[] (or std::vector) instead.");
>> + return XNEWVEC (T, n);
>> +}
>> +
>
> Other than the formatting, this LGTM. Thanks a lot for doing this.
Thanks, I'm glad that I won't have to carry that branch on the side
anymore :)
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete
2017-11-23 17:27 ` Simon Marchi
@ 2017-11-23 17:31 ` Pedro Alves
0 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2017-11-23 17:31 UTC (permalink / raw)
To: Simon Marchi; +Cc: Simon Marchi, gdb-patches
On 11/23/2017 05:27 PM, Simon Marchi wrote:
> On 2017-11-23 10:02, Pedro Alves wrote:
>> On 11/22/2017 04:41 PM, Simon Marchi wrote:
>>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> The criterion I
>>> initially used to allow a type to use XNEW (which calls malloc in the
>>> end) was std::is_trivially_constructible, but then realized that gcc 4.8
>>> did not have it. Instead, I went with:
>>>
>>
>> Yeah, in the memset poisoning I just disabled the poisoning on older
>> compilers. This is the "#if HAVE_IS_TRIVIALLY_COPYABLE" check in
>> poison.h.
>
> I'm not sure if you are suggesting adding
> HAVE_IS_TRIVIALLY_CONSTRUCTIBLE, but since we can use std::is_pod
> easily, I think it's simpler just to use that (even if it's a bit more
> strict than needed).
Nope, I was really just stating a fact.
>>> which is just a bit more strict, which doesn't hurt. A similar thing is
>>> done for macros that free instead of allocated, the criterion is:
>>>
>>> using IsFreeable = gdb::Or<std::is_trivially_destructible<T>,
>>> std::is_void<T>>;
>>>
>>> For simplicity, we could also do for std::is_pod for IsFreeable as well,
>>> if you prefer.
>>>
>>> I chose to put static_assert in the functions, instead of using
>>> gdb::Requires in the template as SFINAE, because it allows to put a
>>> message, which I think makes the compiler error more understandable.
>>> With gdb::Requires, the error is:
>>
>> Right, I think static_assert is what I had suggest initially too.
>> SFINAE alone wouldn't sound right to me here.
>>
>> The right alternative to static_assert would be SFINAE+"=delete;",
>> which is what we do for the memcpy/memcpy poisoning, and what
>> you're doing to "free".
>>
>> =delete keeps the function/overload in the overload set, serving
>> as "honeypot", so you get an error about trying to call a
>> deleted function.
>>
>> You could do that to xfree instead
>> of the static_assert, but I guess inlining xfree ends up being
>> a good thing.
>
> Ah yeah, it would probably make the message a bit clearer. Instead of
> saying "I can't find a function that matches", it would say "I found a
> function that matches, but you can't use it".
Exactly.
> I'll just keep
> static_assert for xfree as well, since it ends up clearer anyway.
*nod*
>> Other than the formatting, this LGTM. Thanks a lot for doing this.
>
> Thanks, I'm glad that I won't have to carry that branch on the side
> anymore :)
>
:-)
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/4] remote: C++ify thread_item and threads_listing_context
2017-11-22 16:41 [PATCH 0/4] Poison XNEW and friends for non-POD types Simon Marchi
2017-11-22 16:41 ` [PATCH 1/4] Create private_inferior class hierarchy Simon Marchi
2017-11-22 16:42 ` [PATCH 4/4] Poison XNEW and friends for types that should use new/delete Simon Marchi
@ 2017-11-22 16:42 ` Simon Marchi
2017-11-23 14:22 ` Pedro Alves
2017-11-22 16:42 ` [PATCH 3/4] Create private_thread_info hierarchy Simon Marchi
3 siblings, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-11-22 16:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@polymtl.ca>
This patch C++ifies the thread_item and threads_listing_context
structures in remote.c. thread_item::{extra,name} are changed to
std::string. As a result, there's a bit of awkwardness in
remote_update_thread_list, where we have to xstrdup those strings when
filling the private_thread_info structure. This is removed in the
following patch, where private_thread_info is also C++ified and its
corresponding fields made std::string too. The xstrdup then becomes an
std::move.
Other than that there's nothing really special, it's a usual day-to-day
VEC -> vector and char* -> std::string change. It allows removing a
cleanup in remote_update_thread_list.
Note that an overload of hex2bin that returns a gdb::byte_vector is
added, with corresponding selftests.
gdb/ChangeLog:
* remote.c (struct thread_item): Add constructor and destructor.
<extra, name>: Change type to std::string.
<core, thread_handle>: Initialize.
(thread_item_t): Remove typedef.
(DEF_VEC_O(thread_item_t)): Remove.
(threads_listing_context) <contains_thread>: New method.
<remove_thread>: New method.
<items>: Change type to std::vector.
(clear_threads_listing_context): Remove.
(threads_listing_context_remove): Remove.
(remote_newthread_step): Use thread_item constructor, adjust to
change to std::vector.
(start_thread): Use thread_item constructor, adjust to change to
std::vector.
(end_thread): Adjust to change to std::vector and std::string.
(remote_get_threads_with_qthreadinfo): Use thread_item
constructor, adjust to std::vector.
(remote_update_thread_list): Adjust to change to std::vector and
std::string, use threads_listing_context methods.
(remove_child_of_pending_fork): Adjust.
(remove_new_fork_children): Adjust.
* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add rsp-low-selftests.c.
(SUBDIR_UNITTESTS_OBS): Add rsp-low-selftests.o.
* unittests/rsp-low-selftests.c: New file.
* common/rsp-low.h: Include common/byte-vector.h.
(hex2bin): New overload.
* common/rsp-low.c (hex2bin): New overload.
---
gdb/Makefile.in | 2 +
gdb/common/rsp-low.c | 13 +++
gdb/common/rsp-low.h | 6 ++
gdb/remote.c | 192 +++++++++++++++-----------------------
gdb/unittests/rsp-low-selftests.c | 59 ++++++++++++
5 files changed, 155 insertions(+), 117 deletions(-)
create mode 100644 gdb/unittests/rsp-low-selftests.c
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 78a43c5..8f96a1f 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -539,6 +539,7 @@ SUBDIR_UNITTESTS_SRCS = \
unittests/offset-type-selftests.c \
unittests/optional-selftests.c \
unittests/ptid-selftests.c \
+ unittests/rsp-low-selftests.c \
unittests/scoped_restore-selftests.c \
unittests/xml-utils-selftests.c
@@ -553,6 +554,7 @@ SUBDIR_UNITTESTS_OBS = \
offset-type-selftests.o \
optional-selftests.o \
ptid-selftests.o \
+ rsp-low-selftests.o \
scoped_restore-selftests.o \
xml-utils-selftests.o
diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
index 85987f7..9948fbb 100644
--- a/gdb/common/rsp-low.c
+++ b/gdb/common/rsp-low.c
@@ -132,6 +132,19 @@ hex2bin (const char *hex, gdb_byte *bin, int count)
/* See rsp-low.h. */
+gdb::byte_vector
+hex2bin (const char *hex)
+{
+ size_t bin_len = strlen (hex) / 2;
+ gdb::byte_vector bin (bin_len);
+
+ hex2bin (hex, bin.data (), bin_len);
+
+ return bin;
+}
+
+/* See rsp-low.h. */
+
std::string
hex2str (const char *hex)
{
diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
index 99dc93f..039f19c 100644
--- a/gdb/common/rsp-low.h
+++ b/gdb/common/rsp-low.h
@@ -20,6 +20,8 @@
#ifndef COMMON_RSP_LOW_H
#define COMMON_RSP_LOW_H
+#include "common/byte-vector.h"
+
/* Convert hex digit A to a number, or throw an exception. */
extern int fromhex (int a);
@@ -52,6 +54,10 @@ extern const char *unpack_varlen_hex (const char *buff, ULONGEST *result);
extern int hex2bin (const char *hex, gdb_byte *bin, int count);
+/* Like the above, but return a gdb::byte_vector. */
+
+gdb::byte_vector hex2bin (const char *hex);
+
/* Like hex2bin, but return a std::string. */
extern std::string hex2str (const char *hex);
diff --git a/gdb/remote.c b/gdb/remote.c
index f328080..0270f95 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2972,25 +2972,33 @@ remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
/* A thread found on the remote target. */
-typedef struct thread_item
+struct thread_item
{
+ thread_item (ptid_t ptid_)
+ : ptid (ptid_)
+ {}
+
+ ~thread_item ()
+ {
+ delete thread_handle;
+ }
+
/* The thread's PTID. */
ptid_t ptid;
/* The thread's extra info. May be NULL. */
- char *extra;
+ std::string extra;
/* The thread's name. May be NULL. */
- char *name;
+ std::string name;
/* The core the thread was running on. -1 if not known. */
- int core;
+ int core = -1;
/* The thread handle associated with the thread. */
- gdb::byte_vector *thread_handle;
+ gdb::byte_vector *thread_handle = NULL;
-} thread_item_t;
-DEF_VEC_O(thread_item_t);
+};
/* Context passed around to the various methods listing remote
threads. As new threads are found, they're added to the ITEMS
@@ -2998,66 +3006,54 @@ DEF_VEC_O(thread_item_t);
struct threads_listing_context
{
- /* The threads found on the remote target. */
- VEC (thread_item_t) *items;
-};
+ /* Return true if this object contains an entry for a thread with ptid
+ PTID. */
-/* Discard the contents of the constructed thread listing context. */
+ bool contains_thread (ptid_t ptid) const
+ {
+ auto match_ptid = [&] (const thread_item &item)
+ {
+ return item.ptid == ptid;
+ };
-static void
-clear_threads_listing_context (void *p)
-{
- struct threads_listing_context *context
- = (struct threads_listing_context *) p;
- int i;
- struct thread_item *item;
+ auto it = std::find_if (this->items.begin (),
+ this->items.end (),
+ match_ptid);
- for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i)
- {
- xfree (item->extra);
- xfree (item->name);
- delete item->thread_handle;
- }
+ return it != this->items.end ();
+ }
- VEC_free (thread_item_t, context->items);
-}
+ /* Remove the thread with ptid PTID. */
-/* Remove the thread specified as the related_pid field of WS
- from the CONTEXT list. */
+ void remove_thread (ptid_t ptid)
+ {
+ auto match_ptid = [&] (const thread_item &item)
+ {
+ return item.ptid == ptid;
+ };
-static void
-threads_listing_context_remove (struct target_waitstatus *ws,
- struct threads_listing_context *context)
-{
- struct thread_item *item;
- int i;
- ptid_t child_ptid = ws->value.related_pid;
+ auto it = std::remove_if (this->items.begin (),
+ this->items.end (),
+ match_ptid);
- for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i)
- {
- if (ptid_equal (item->ptid, child_ptid))
- {
- VEC_ordered_remove (thread_item_t, context->items, i);
- break;
- }
- }
-}
+ if (it != this->items.end ())
+ this->items.erase (it);
+ }
+
+ /* The threads found on the remote target. */
+ std::vector<thread_item> items;
+};
static int
remote_newthread_step (threadref *ref, void *data)
{
struct threads_listing_context *context
= (struct threads_listing_context *) data;
- struct thread_item item;
- int pid = ptid_get_pid (inferior_ptid);
-
- item.ptid = ptid_build (pid, threadref_to_int (ref), 0);
- item.core = -1;
- item.name = NULL;
- item.extra = NULL;
- item.thread_handle = nullptr;
+ int pid = inferior_ptid.pid ();
+ int lwp = threadref_to_int (ref);
+ ptid_t ptid (pid, lwp);
- VEC_safe_push (thread_item_t, context->items, &item);
+ context->items.emplace_back (ptid);
return 1; /* continue iterator */
}
@@ -3109,37 +3105,28 @@ start_thread (struct gdb_xml_parser *parser,
{
struct threads_listing_context *data
= (struct threads_listing_context *) user_data;
-
- struct thread_item item;
- char *id;
struct gdb_xml_value *attr;
- id = (char *) xml_find_attribute (attributes, "id")->value;
- item.ptid = read_ptid (id, NULL);
+ char *id = (char *) xml_find_attribute (attributes, "id")->value;
+ ptid_t ptid = read_ptid (id, NULL);
+
+ data->items.emplace_back (ptid);
+ thread_item &item = data->items.back ();
attr = xml_find_attribute (attributes, "core");
if (attr != NULL)
item.core = *(ULONGEST *) attr->value;
- else
- item.core = -1;
attr = xml_find_attribute (attributes, "name");
- item.name = attr != NULL ? xstrdup ((const char *) attr->value) : NULL;
+ if (attr != NULL)
+ item.name = (const char *) attr->value;
attr = xml_find_attribute (attributes, "handle");
if (attr != NULL)
{
- item.thread_handle = new gdb::byte_vector
- (strlen ((const char *) attr->value) / 2);
- hex2bin ((const char *) attr->value, item.thread_handle->data (),
- item.thread_handle->size ());
+ item.thread_handle = new gdb::byte_vector;
+ *item.thread_handle = hex2bin ((const char *) attr->value);
}
- else
- item.thread_handle = nullptr;
-
- item.extra = 0;
-
- VEC_safe_push (thread_item_t, data->items, &item);
}
static void
@@ -3150,8 +3137,8 @@ end_thread (struct gdb_xml_parser *parser,
struct threads_listing_context *data
= (struct threads_listing_context *) user_data;
- if (body_text && *body_text)
- VEC_last (thread_item_t, data->items)->extra = xstrdup (body_text);
+ if (body_text != NULL && *body_text != '\0')
+ data->items.back ().extra = body_text;
}
const struct gdb_xml_attribute thread_attributes[] = {
@@ -3227,15 +3214,8 @@ remote_get_threads_with_qthreadinfo (struct target_ops *ops,
{
do
{
- struct thread_item item;
-
- item.ptid = read_ptid (bufp, &bufp);
- item.core = -1;
- item.name = NULL;
- item.extra = NULL;
- item.thread_handle = nullptr;
-
- VEC_safe_push (thread_item_t, context->items, &item);
+ ptid_t ptid = read_ptid (bufp, &bufp);
+ context->items.emplace_back (ptid);
}
while (*bufp++ == ','); /* comma-separated list */
putpkt ("qsThreadInfo");
@@ -3261,12 +3241,8 @@ static void
remote_update_thread_list (struct target_ops *ops)
{
struct threads_listing_context context;
- struct cleanup *old_chain;
int got_list = 0;
- context.items = NULL;
- old_chain = make_cleanup (clear_threads_listing_context, &context);
-
/* We have a few different mechanisms to fetch the thread list. Try
them all, starting with the most preferred one first, falling
back to older methods. */
@@ -3275,12 +3251,11 @@ remote_update_thread_list (struct target_ops *ops)
|| remote_get_threads_with_ql (ops, &context))
{
int i;
- struct thread_item *item;
struct thread_info *tp, *tmp;
got_list = 1;
- if (VEC_empty (thread_item_t, context.items)
+ if (context.items.empty ()
&& remote_thread_always_alive (ops, inferior_ptid))
{
/* Some targets don't really support threads, but still
@@ -3288,7 +3263,6 @@ remote_update_thread_list (struct target_ops *ops)
listing packets, instead of replying "packet not
supported". Exit early so we don't delete the main
thread. */
- do_cleanups (old_chain);
return;
}
@@ -3297,15 +3271,7 @@ remote_update_thread_list (struct target_ops *ops)
target. */
ALL_THREADS_SAFE (tp, tmp)
{
- for (i = 0;
- VEC_iterate (thread_item_t, context.items, i, item);
- ++i)
- {
- if (ptid_equal (item->ptid, tp->ptid))
- break;
- }
-
- if (i == VEC_length (thread_item_t, context.items))
+ if (!context.contains_thread (tp->ptid))
{
/* Not found. */
delete_thread (tp->ptid);
@@ -3318,11 +3284,9 @@ remote_update_thread_list (struct target_ops *ops)
remove_new_fork_children (&context);
/* And now add threads we don't know about yet to our list. */
- for (i = 0;
- VEC_iterate (thread_item_t, context.items, i, item);
- ++i)
+ for (thread_item &item : context.items)
{
- if (!ptid_equal (item->ptid, null_ptid))
+ if (item.ptid != null_ptid)
{
struct private_thread_info *info;
/* In non-stop mode, we assume new found threads are
@@ -3331,16 +3295,14 @@ remote_update_thread_list (struct target_ops *ops)
stopped. */
int executing = target_is_non_stop_p () ? 1 : 0;
- remote_notice_new_inferior (item->ptid, executing);
+ remote_notice_new_inferior (item.ptid, executing);
- info = get_private_info_ptid (item->ptid);
- info->core = item->core;
- info->extra = item->extra;
- item->extra = NULL;
- info->name = item->name;
- item->name = NULL;
- info->thread_handle = item->thread_handle;
- item->thread_handle = nullptr;
+ info = get_private_info_ptid (item.ptid);
+ info->core = item.core;
+ info->extra = xstrdup (item.extra.c_str ());
+ info->name = xstrdup (item.name.c_str ());
+ info->thread_handle = item.thread_handle;
+ item.thread_handle = nullptr;
}
}
}
@@ -3353,8 +3315,6 @@ remote_update_thread_list (struct target_ops *ops)
no-op. See remote_thread_alive. */
prune_threads ();
}
-
- do_cleanups (old_chain);
}
/*
@@ -6520,7 +6480,7 @@ remove_child_of_pending_fork (QUEUE (stop_reply_p) *q,
if (event->ws.kind == TARGET_WAITKIND_FORKED
|| event->ws.kind == TARGET_WAITKIND_VFORKED
|| event->ws.kind == TARGET_WAITKIND_THREAD_EXITED)
- threads_listing_context_remove (&event->ws, context);
+ context->remove_thread (event->ws.value.related_pid);
return 1;
}
@@ -6546,9 +6506,7 @@ remove_new_fork_children (struct threads_listing_context *context)
struct target_waitstatus *ws = thread_pending_fork_status (thread);
if (is_pending_fork_parent (ws, pid, thread->ptid))
- {
- threads_listing_context_remove (ws, context);
- }
+ context->remove_thread (ws->value.related_pid);
}
/* Check for any pending fork events (not reported or processed yet)
diff --git a/gdb/unittests/rsp-low-selftests.c b/gdb/unittests/rsp-low-selftests.c
new file mode 100644
index 0000000..e20fedf
--- /dev/null
+++ b/gdb/unittests/rsp-low-selftests.c
@@ -0,0 +1,59 @@
+/* Unit tests for the rsp-low.c file.
+
+ Copyright (C) 2017 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "defs.h"
+#include "selftest.h"
+#include "common/rsp-low.h"
+
+namespace selftests {
+namespace rsp_low {
+
+/* Test the variant of hex2bin that returns a byte_vector. */
+
+static void test_hex2bin_byte_vector ()
+{
+ gdb::byte_vector bv;
+
+ /* Test an empty string. */
+ bv = hex2bin ("");
+ SELF_CHECK (bv.size () == 0);
+
+ /* Test a well-formated hex string. */
+ bv = hex2bin ("abcd01");
+ SELF_CHECK (bv.size () == 3);
+ SELF_CHECK (bv[0] == 0xab);
+ SELF_CHECK (bv[1] == 0xcd);
+ SELF_CHECK (bv[2] == 0x01);
+
+ /* Test an odd-length hex string. */
+ bv = hex2bin ("0123c");
+ SELF_CHECK (bv.size () == 2);
+ SELF_CHECK (bv[0] == 0x01);
+ SELF_CHECK (bv[1] == 0x23);
+}
+
+} /* namespace rsp_low */
+} /* namespace selftests */
+
+void
+_initialize_rsp_low_selftests ()
+{
+ selftests::register_test ("hex2bin_byte_vector",
+ selftests::rsp_low::test_hex2bin_byte_vector);
+}
--
2.7.4
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] remote: C++ify thread_item and threads_listing_context
2017-11-22 16:42 ` [PATCH 2/4] remote: C++ify thread_item and threads_listing_context Simon Marchi
@ 2017-11-23 14:22 ` Pedro Alves
2017-11-23 16:48 ` Simon Marchi
0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2017-11-23 14:22 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Simon Marchi
On 11/22/2017 04:41 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> This patch C++ifies the thread_item and threads_listing_context
> structures in remote.c. thread_item::{extra,name} are changed to
> std::string. As a result, there's a bit of awkwardness in
> remote_update_thread_list, where we have to xstrdup those strings when
> filling the private_thread_info structure. This is removed in the
> following patch, where private_thread_info is also C++ified and its
> corresponding fields made std::string too. The xstrdup then becomes an
> std::move.
>
> Other than that there's nothing really special, it's a usual day-to-day
> VEC -> vector and char* -> std::string change. It allows removing a
> cleanup in remote_update_thread_list.
>
> Note that an overload of hex2bin that returns a gdb::byte_vector is
> added, with corresponding selftests.
>
Awesome. Just a couple minor comments below.
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2972,25 +2972,33 @@ remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
>
> /* A thread found on the remote target. */
>
> -typedef struct thread_item
> +struct thread_item
> {
> + thread_item (ptid_t ptid_)
> + : ptid (ptid_)
> + {}
Could be explicit ?
> +
> + ~thread_item ()
> + {
> + delete thread_handle;
> + }
> +
> /* The thread's PTID. */
> ptid_t ptid;
>
> /* The thread's extra info. May be NULL. */
> - char *extra;
> + std::string extra;
>
> /* The thread's name. May be NULL. */
> - char *name;
> + std::string name;
>
> /* The core the thread was running on. -1 if not known. */
> - int core;
> + int core = -1;
>
> /* The thread handle associated with the thread. */
> - gdb::byte_vector *thread_handle;
> + gdb::byte_vector *thread_handle = NULL;
This could be made a non-pointer now, right?
> @@ -3109,37 +3105,28 @@ start_thread (struct gdb_xml_parser *parser,
> {
> attr = xml_find_attribute (attributes, "name");
> - item.name = attr != NULL ? xstrdup ((const char *) attr->value) : NULL;
> + if (attr != NULL)
> + item.name = (const char *) attr->value;
Are you missing the xstrdup here? I guess this is related to
the awkwardness you mentioned.
> @@ -3150,8 +3137,8 @@ end_thread (struct gdb_xml_parser *parser,
> struct threads_listing_context *data
> = (struct threads_listing_context *) user_data;
>
> - if (body_text && *body_text)
> - VEC_last (thread_item_t, data->items)->extra = xstrdup (body_text);
> + if (body_text != NULL && *body_text != '\0')
> + data->items.back ().extra = body_text;
And here?
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] remote: C++ify thread_item and threads_listing_context
2017-11-23 14:22 ` Pedro Alves
@ 2017-11-23 16:48 ` Simon Marchi
2017-11-23 16:52 ` Pedro Alves
0 siblings, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-11-23 16:48 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-11-23 09:22, Pedro Alves wrote:
> On 11/22/2017 04:41 PM, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> This patch C++ifies the thread_item and threads_listing_context
>> structures in remote.c. thread_item::{extra,name} are changed to
>> std::string. As a result, there's a bit of awkwardness in
>> remote_update_thread_list, where we have to xstrdup those strings when
>> filling the private_thread_info structure. This is removed in the
>> following patch, where private_thread_info is also C++ified and its
>> corresponding fields made std::string too. The xstrdup then becomes
>> an
>> std::move.
>>
>> Other than that there's nothing really special, it's a usual
>> day-to-day
>> VEC -> vector and char* -> std::string change. It allows removing a
>> cleanup in remote_update_thread_list.
>>
>> Note that an overload of hex2bin that returns a gdb::byte_vector is
>> added, with corresponding selftests.
>>
>
> Awesome. Just a couple minor comments below.
>
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -2972,25 +2972,33 @@ remote_threadlist_iterator (rmt_thread_action
>> stepfunction, void *context,
>>
>> /* A thread found on the remote target. */
>>
>> -typedef struct thread_item
>> +struct thread_item
>> {
>> + thread_item (ptid_t ptid_)
>> + : ptid (ptid_)
>> + {}
>
> Could be explicit ?
Yes. As you can see I don't have that reflex yet. I wish constructors
were explicit by default though...
>> +
>> + ~thread_item ()
>> + {
>> + delete thread_handle;
>> + }
>> +
>> /* The thread's PTID. */
>> ptid_t ptid;
>>
>> /* The thread's extra info. May be NULL. */
>> - char *extra;
>> + std::string extra;
>>
>> /* The thread's name. May be NULL. */
>> - char *name;
>> + std::string name;
>>
>> /* The core the thread was running on. -1 if not known. */
>> - int core;
>> + int core = -1;
>>
>> /* The thread handle associated with the thread. */
>> - gdb::byte_vector *thread_handle;
>> + gdb::byte_vector *thread_handle = NULL;
>
> This could be made a non-pointer now, right?
It is changed in the next patch, I thought it would be simpler to change
it to non-pointer at the same time as the corresponding field in
remote's private_thread_info. But now I realize that this state is not
really good, because the destructor frees the vector and the default
copy constructor/assignment operator are not deleted, so the vector can
end up double-freed. In any case, I think we'll want to disable copy
construction and copy assignment (to make sure we don't do copies by
accident). If I keep the field as a pointer, then I would need to write
a custom move constructor that will get removed in the next patch. So
instead I'll make the field non-pointer, as you suggest, it ends up
simpler. When transferring the thread_handle from the thread_item to
the private_thread_info, this should be enough:
info->thread_handle
= new gdb::byte_vector (std::move (item.thread_handle));
which will get replaced by a simple std::move in the next patch.
>> @@ -3109,37 +3105,28 @@ start_thread (struct gdb_xml_parser *parser,
>> {
>
>> attr = xml_find_attribute (attributes, "name");
>> - item.name = attr != NULL ? xstrdup ((const char *) attr->value) :
>> NULL;
>> + if (attr != NULL)
>> + item.name = (const char *) attr->value;
>
> Are you missing the xstrdup here? I guess this is related to
> the awkwardness you mentioned.
I don't think so, here item.name is an std::string, so the assignment
does a copy. This copy is required I think, because attr->value won't
exist after we're done parsing the XML.
>> @@ -3150,8 +3137,8 @@ end_thread (struct gdb_xml_parser *parser,
>> struct threads_listing_context *data
>> = (struct threads_listing_context *) user_data;
>>
>> - if (body_text && *body_text)
>> - VEC_last (thread_item_t, data->items)->extra = xstrdup
>> (body_text);
>> + if (body_text != NULL && *body_text != '\0')
>> + data->items.back ().extra = body_text;
>
> And here?
Same, extra is an std::string.
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/4] remote: C++ify thread_item and threads_listing_context
2017-11-23 16:48 ` Simon Marchi
@ 2017-11-23 16:52 ` Pedro Alves
0 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2017-11-23 16:52 UTC (permalink / raw)
To: Simon Marchi; +Cc: Simon Marchi, gdb-patches
On 11/23/2017 04:48 PM, Simon Marchi wrote:
> On 2017-11-23 09:22, Pedro Alves wrote:
>>> @@ -3109,37 +3105,28 @@ start_thread (struct gdb_xml_parser *parser,
>>> {
>>
>>> attr = xml_find_attribute (attributes, "name");
>>> - item.name = attr != NULL ? xstrdup ((const char *) attr->value) :
>>> NULL;
>>> + if (attr != NULL)
>>> + item.name = (const char *) attr->value;
>>
>> Are you missing the xstrdup here? I guess this is related to
>> the awkwardness you mentioned.
>
> I don't think so, here item.name is an std::string, so the assignment
> does a copy. This copy is required I think, because attr->value won't
> exist after we're done parsing the XML.
>
>>> @@ -3150,8 +3137,8 @@ end_thread (struct gdb_xml_parser *parser,
>>> struct threads_listing_context *data
>>> = (struct threads_listing_context *) user_data;
>>>
>>> - if (body_text && *body_text)
>>> - VEC_last (thread_item_t, data->items)->extra = xstrdup (body_text);
>>> + if (body_text != NULL && *body_text != '\0')
>>> + data->items.back ().extra = body_text;
>>
>> And here?
>
> Same, extra is an std::string.
Right, sorry, glanced over that too quickly. :-P
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/4] Create private_thread_info hierarchy
2017-11-22 16:41 [PATCH 0/4] Poison XNEW and friends for non-POD types Simon Marchi
` (2 preceding siblings ...)
2017-11-22 16:42 ` [PATCH 2/4] remote: C++ify thread_item and threads_listing_context Simon Marchi
@ 2017-11-22 16:42 ` Simon Marchi
2017-11-23 14:41 ` Pedro Alves
3 siblings, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-11-22 16:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@polymtl.ca>
There are multiple definitions of the private_thread_info structure
compiled in the same GDB build. Because of the one definition rule, we
need to change this if we want to be able to make them non-POD (e.g. use
std::vector fields). This patch creates a class hierarchy, with
private_thread_info being an abstract base class, and all the specific
implementations inheriting from it.
In order to poison XNEW/xfree for non-POD types, it is also needed to
get rid of the xfree in thread_info::~thread_info, which operates on an
opaque type. This is replaced by thread_info::priv now being a
unique_ptr, which calls the destructor of the private_thread_info
subclass when the thread is being destroyed.
Including gdbthread.h from darwin-nat.h gave these errors:
/Users/simark/src/binutils-gdb/gdb/gdbthread.h:609:3: error: must use 'class' tag to refer to type 'thread_info' in this scope
thread_info *m_thread;
^
class
/usr/include/mach/thread_act.h:240:15: note: class 'thread_info' is hidden by a non-type declaration of 'thread_info' here
kern_return_t thread_info
^
It turns out that there is a thread_info function in the Darwin/XNU/mach API:
http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/thread_info.html
Therefore, I had to add the class keyword at a couple of places in gdbthread.h,
I don't really see a way around it.
gdb/ChangeLog:
* gdbthread.h (private_thread_info): Define structure type, add
virtual pure destructor.
(thread_info) <priv>: Change type to unique_ptr.
<private_dtor>: Remove.
* thread.c (add_thread_with_info): Adjust to use of unique_ptr.
(private_thread_info::~private_thread_info): Provide default
implementation.
(thread_info::~thread_info): Don't call private_dtor nor
manually free priv.
* aix-thread.c (private_thread_info): Rename to ...
(aix_thread_info): ... this.
(sync_threadlists): Adjust.
(iter_tid): Adjust.
(aix_thread_resume): Adjust.
(aix_thread_fetch_registers): Adjust.
(aix_thread_store_registers): Adjust.
(aix_thread_extra_thread_info): Adjust.
* darwin-nat.h (private_thread_info): Rename to ...
(darwin_thread_info): ... this.
* darwin-nat.c (darwin_init_thread_list): Adjust.
(darwin_check_new_threads): Adjust.
(thread_info_from_private_thread_info): Adjust.
* linux-thread-db.c (private_thread_info): Rename to ...
(thread_db_thread_info): ... this, initialize fields.
<dying>: Change type to bool.
(update_thread_state): Adjust to type rename.
(record_thread): Adjust to type rename an use of unique_ptr.
(thread_db_pid_to_str): Likewise.
(thread_db_extra_thread_info): Likewise.
(thread_db_thread_handle_to_thread_info): Likewise.
(thread_db_get_thread_local_address): Likewise.
* nto-tdep.h (private_thread_info): Rename to ...
(nto_thread_info): ... this, initialize fields.
<name>: Change type to std::string.
* nto-tdep.c (nto_extra_thread_info): Adjust to type rename and
use of unique_ptr.
* nto-procfs.c (update_thread_private_data_name): Adjust to
std::string change, allocate nto_private_thread_info with new.
(update_thread_private_data): Adjust to unique_ptr.
* remote.c (private_thread_info): Rename to ...
(remote_thread_info): ... this, initialize data members with
default values.
<extra, name>: Change type to std::string.
<thread_handle>: Change type to non-pointer.
(free_private_thread_info): Remove.
(get_private_info_thread): Change return type, adjust to use of
unique_ptr, use remote_thread_info constructor.
(get_private_info_ptid): Change return type.
(remote_thread_name): Use get_private_info_thread, adjust to
change to std::string.
(struct thread_item) <~thread_item>: Remove.
<thread_handle>: Make non pointer.
(start_thread): Adjust to thread_item::thread_handle type
change.
(remote_update_thread_list): Adjust to type name change, move
strings from temporary to long-lived object instead of
duplicating.
(remote_threads_extra_info): Use get_private_info_thread.
(process_initial_stop_replies): Likewise.
(resume_clear_thread_private_info): Likewise.
(remote_resume): Adjust to type name change.
(remote_commit_resume): Use get_private_info_thread.
(process_stop_reply): Adjust to type name change.
(remote_stopped_by_sw_breakpoint): Use get_private_info_thread.
(remote_stopped_by_hw_breakpoint): Likewise.
(remote_stopped_by_watchpoint): Likewise.
(remote_stopped_data_address): Likewise.
(remote_core_of_thread): Likewise.
(remote_thread_handle_to_thread_info): Use
get_private_info_thread, adjust to thread_handle field type
change.
---
gdb/aix-thread.c | 49 ++++++++++++-------
gdb/darwin-nat.c | 14 +++---
gdb/darwin-nat.h | 5 +-
gdb/gdbthread.h | 18 ++++---
gdb/linux-thread-db.c | 41 ++++++++--------
gdb/nto-procfs.c | 30 +++---------
gdb/nto-tdep.c | 10 ++--
gdb/nto-tdep.h | 10 ++--
gdb/remote.c | 133 ++++++++++++++++++++------------------------------
gdb/thread.c | 14 ++----
10 files changed, 151 insertions(+), 173 deletions(-)
diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index b5c9f3b..efbc047 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -84,7 +84,8 @@ static int debug_aix_thread;
/* Private data attached to each element in GDB's thread list. */
-struct private_thread_info {
+struct aix_thread_info : public private_thread_info
+{
pthdb_pthread_t pdtid; /* thread's libpthdebug id */
pthdb_tid_t tid; /* kernel thread id */
};
@@ -756,10 +757,12 @@ sync_threadlists (void)
}
else if (gi == gcount)
{
- thread = add_thread (ptid_build (infpid, 0, pbuf[pi].pthid));
- thread->priv = XNEW (struct private_thread_info);
- thread->priv->pdtid = pbuf[pi].pdtid;
- thread->priv->tid = pbuf[pi].tid;
+ aix_thread_info *priv = new aix_thread_info;
+ priv->pdtid = pbuf[pi].pdtid;
+ priv->tid = pbuf[pi].tid;
+
+ thread = add_thread_with_info (ptid_t (infpid, 0, pbuf[pi].pthid), priv);
+
pi++;
}
else
@@ -776,8 +779,10 @@ sync_threadlists (void)
if (cmp_result == 0)
{
- gbuf[gi]->priv->pdtid = pdtid;
- gbuf[gi]->priv->tid = tid;
+ aix_thread_info *priv = (aix_thread_info *) gbuf[gi]->priv.get ();
+
+ priv->pdtid = pdtid;
+ priv->tid = tid;
pi++;
gi++;
}
@@ -789,9 +794,11 @@ sync_threadlists (void)
else
{
thread = add_thread (pptid);
- thread->priv = XNEW (struct private_thread_info);
- thread->priv->pdtid = pdtid;
- thread->priv->tid = tid;
+
+ aix_thread_info *priv = new aix_thread_info;
+ thread->priv.reset (priv);
+ priv->pdtid = pdtid;
+ priv->tid = tid;
pi++;
}
}
@@ -808,8 +815,9 @@ static int
iter_tid (struct thread_info *thread, void *tidp)
{
const pthdb_tid_t tid = *(pthdb_tid_t *)tidp;
+ aix_thread_info *priv = (aix_thread_info *) thread->priv.get ();
- return (thread->priv->tid == tid);
+ return priv->tid == tid;
}
/* Synchronize libpthdebug's state with the inferior and with GDB,
@@ -998,7 +1006,9 @@ aix_thread_resume (struct target_ops *ops,
error (_("aix-thread resume: unknown pthread %ld"),
ptid_get_lwp (ptid));
- tid[0] = thread->priv->tid;
+ aix_thread_info *priv = (aix_thread_info *) thread->priv.get ();
+
+ tid[0] = priv->tid;
if (tid[0] == PTHDB_INVALID_TID)
error (_("aix-thread resume: no tid for pthread %ld"),
ptid_get_lwp (ptid));
@@ -1314,10 +1324,11 @@ aix_thread_fetch_registers (struct target_ops *ops,
else
{
thread = find_thread_ptid (regcache_get_ptid (regcache));
- tid = thread->priv->tid;
+ aix_thread_info *priv = (aix_thread_info *) thread->priv.get ();
+ tid = priv->tid;
if (tid == PTHDB_INVALID_TID)
- fetch_regs_user_thread (regcache, thread->priv->pdtid);
+ fetch_regs_user_thread (regcache, priv->pdtid);
else
fetch_regs_kernel_thread (regcache, regno, tid);
}
@@ -1668,10 +1679,11 @@ aix_thread_store_registers (struct target_ops *ops,
else
{
thread = find_thread_ptid (regcache_get_ptid (regcache));
- tid = thread->priv->tid;
+ aix_thread_info *priv = (aix_thread_info *) thread->priv.get ();
+ tid = priv->tid;
if (tid == PTHDB_INVALID_TID)
- store_regs_user_thread (regcache, thread->priv->pdtid);
+ store_regs_user_thread (regcache, priv->pdtid);
else
store_regs_kernel_thread (regcache, regno, tid);
}
@@ -1759,9 +1771,10 @@ aix_thread_extra_thread_info (struct target_ops *self,
return NULL;
string_file buf;
+ aix_thread_info *priv = (aix_thread_info *) thread->priv.get ();
- pdtid = thread->priv->pdtid;
- tid = thread->priv->tid;
+ pdtid = priv->pdtid;
+ tid = priv->tid;
if (tid != PTHDB_INVALID_TID)
/* i18n: Like "thread-identifier %d, [state] running, suspended" */
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index f4ed455..0de3eb2 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -363,9 +363,8 @@ darwin_check_new_threads (struct inferior *inf)
if (new_ix < new_nbr && (old_ix == old_nbr || new_id < old_id))
{
/* A thread was created. */
- struct private_thread_info *pti;
+ darwin_thread_info *pti = new darwin_thread_info;
- pti = XCNEW (darwin_thread_t);
pti->gdb_port = new_id;
pti->msg_state = DARWIN_RUNNING;
@@ -1692,16 +1691,19 @@ darwin_attach_pid (struct inferior *inf)
push_target (darwin_ops);
}
-/* Get the thread_info object corresponding to this private_thread_info. */
+/* Get the thread_info object corresponding to this darwin_thread_info. */
static struct thread_info *
-thread_info_from_private_thread_info (private_thread_info *pti)
+thread_info_from_private_thread_info (darwin_thread_info *pti)
{
struct thread_info *it;
ALL_THREADS (it)
{
- if (it->priv->gdb_port == pti->gdb_port)
+ darwin_thread_info *iter_pti
+ = (darwin_thread_info *) it->priv.get ();
+
+ if (iter_pti->gdb_port == pti->gdb_port)
break;
}
@@ -1719,7 +1721,7 @@ darwin_init_thread_list (struct inferior *inf)
gdb_assert (!priv->threads.empty ());
- private_thread_info *first_pti = priv->threads.front ();
+ darwin_thread_info *first_pti = priv->threads.front ();
struct thread_info *first_thread
= thread_info_from_private_thread_info (first_pti);
diff --git a/gdb/darwin-nat.h b/gdb/darwin-nat.h
index 24c10b5..e7b029a 100644
--- a/gdb/darwin-nat.h
+++ b/gdb/darwin-nat.h
@@ -18,6 +18,7 @@
#define __DARWIN_NAT_H__
#include <mach/mach.h>
+#include "gdbthread.h"
/* Describe the mach exception handling state for a task. This state is saved
before being changed and restored when a process is detached.
@@ -69,7 +70,7 @@ enum darwin_msg_state
DARWIN_MESSAGE
};
-struct private_thread_info
+struct darwin_thread_info : public private_thread_info
{
/* The thread port from a GDB point of view. */
thread_t gdb_port;
@@ -92,7 +93,7 @@ struct private_thread_info
/* The last exception received. */
struct darwin_exception_msg event;
};
-typedef struct private_thread_info darwin_thread_t;
+typedef struct darwin_thread_info darwin_thread_t;
/* Describe an inferior. */
struct darwin_inferior : public private_inferior
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 49fc80f..90cc2b1 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -179,6 +179,12 @@ typedef struct value *value_ptr;
DEF_VEC_P (value_ptr);
typedef VEC (value_ptr) value_vec;
+/* Base class for target-specific thread data. */
+struct private_thread_info
+{
+ virtual ~private_thread_info () = 0;
+};
+
/* Threads are intrusively refcounted objects. Being the
user-selected thread is normally considered an implicit strong
reference and is thus not accounted in the refcount, unlike
@@ -345,11 +351,7 @@ public:
struct frame_id initiating_frame = null_frame_id;
/* Private data used by the target vector implementation. */
- struct private_thread_info *priv = NULL;
-
- /* Function that is called to free PRIVATE. If this is NULL, then
- xfree will be called on PRIVATE. */
- void (*private_dtor) (struct private_thread_info *) = NULL;
+ std::unique_ptr<private_thread_info> priv;
/* Branch trace information for this thread. */
struct btrace_thread_info btrace {};
@@ -604,7 +606,9 @@ public:
DISABLE_COPY_AND_ASSIGN (scoped_restore_current_thread);
private:
- thread_info *m_thread;
+ /* Use the "class" keyword here, because of a class with a "thread_info"
+ function in the Darwin API. */
+ class thread_info *m_thread;
inferior *m_inf;
frame_id m_selected_frame_id;
int m_selected_frame_level;
@@ -683,7 +687,7 @@ extern void print_selected_thread_frame (struct ui_out *uiout,
Selects thread THR. TIDSTR is the original string the thread ID
was parsed from. This is used in the error message if THR is not
alive anymore. */
-extern void thread_select (const char *tidstr, thread_info *thr);
+extern void thread_select (const char *tidstr, class thread_info *thr);
extern struct thread_info *thread_list;
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index ea032fc..9b8b3bf 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -251,14 +251,14 @@ delete_thread_db_info (int pid)
/* Use "struct private_thread_info" to cache thread state. This is
a substantial optimization. */
-struct private_thread_info
+struct thread_db_thread_info : public private_thread_info
{
/* Flag set when we see a TD_DEATH event for this thread. */
- unsigned int dying:1;
+ bool dying = false;
/* Cached thread state. */
- td_thrhandle_t th;
- thread_t tid;
+ td_thrhandle_t th {};
+ thread_t tid {};
};
\f
@@ -1040,7 +1040,7 @@ thread_db_inferior_created (struct target_ops *target, int from_tty)
from libthread_db thread state information. */
static void
-update_thread_state (struct private_thread_info *priv,
+update_thread_state (thread_db_thread_info *priv,
const td_thrinfo_t *ti_p)
{
priv->dying = (ti_p->ti_state == TD_THR_UNKNOWN
@@ -1057,8 +1057,6 @@ record_thread (struct thread_db_info *info,
ptid_t ptid, const td_thrhandle_t *th_p,
const td_thrinfo_t *ti_p)
{
- struct private_thread_info *priv;
-
/* A thread ID of zero may mean the thread library has not
initialized yet. Leave private == NULL until the thread library
has initialized. */
@@ -1066,7 +1064,7 @@ record_thread (struct thread_db_info *info,
return tp;
/* Construct the thread's private data. */
- priv = XCNEW (struct private_thread_info);
+ thread_db_thread_info *priv = new thread_db_thread_info;
priv->th = *th_p;
priv->tid = ti_p->ti_tid;
@@ -1078,7 +1076,7 @@ record_thread (struct thread_db_info *info,
if (tp == NULL || tp->state == THREAD_EXITED)
tp = add_thread_with_info (ptid, priv);
else
- tp->priv = priv;
+ tp->priv.reset (priv);
if (target_has_execution)
check_thread_signals ();
@@ -1381,11 +1379,11 @@ thread_db_pid_to_str (struct target_ops *ops, ptid_t ptid)
if (thread_info != NULL && thread_info->priv != NULL)
{
static char buf[64];
- thread_t tid;
+ thread_db_thread_info *priv
+ = (thread_db_thread_info *) thread_info->priv.get ();
- tid = thread_info->priv->tid;
snprintf (buf, sizeof (buf), "Thread 0x%lx (LWP %ld)",
- (unsigned long) tid, ptid_get_lwp (ptid));
+ (unsigned long) priv->tid, ptid_get_lwp (ptid));
return buf;
}
@@ -1404,7 +1402,9 @@ thread_db_extra_thread_info (struct target_ops *self,
if (info->priv == NULL)
return NULL;
- if (info->priv->dying)
+ thread_db_thread_info *priv = (thread_db_thread_info *) info->priv.get ();
+
+ if (priv->dying)
return "Exiting";
return NULL;
@@ -1434,7 +1434,9 @@ thread_db_thread_handle_to_thread_info (struct target_ops *ops,
ALL_NON_EXITED_THREADS (tp)
{
- if (tp->inf == inf && tp->priv != NULL && handle_tid == tp->priv->tid)
+ thread_db_thread_info *priv = (thread_db_thread_info *) tp->priv.get ();
+
+ if (tp->inf == inf && priv != NULL && handle_tid == priv->tid)
return tp;
}
@@ -1464,9 +1466,9 @@ thread_db_get_thread_local_address (struct target_ops *ops,
{
td_err_e err;
psaddr_t address;
- struct thread_db_info *info;
-
- info = get_thread_db_info (ptid_get_pid (ptid));
+ thread_db_info *info = get_thread_db_info (ptid_get_pid (ptid));
+ thread_db_thread_info *priv
+ = (thread_db_thread_info *) thread_info->priv.get ();
/* Finally, get the address of the variable. */
if (lm != 0)
@@ -1479,7 +1481,7 @@ thread_db_get_thread_local_address (struct target_ops *ops,
/* Note the cast through uintptr_t: this interface only works if
a target address fits in a psaddr_t, which is a host pointer.
So a 32-bit debugger can not access 64-bit TLS through this. */
- err = info->td_thr_tls_get_addr_p (&thread_info->priv->th,
+ err = info->td_thr_tls_get_addr_p (&priv->th,
(psaddr_t)(uintptr_t) lm,
offset, &address);
}
@@ -1497,8 +1499,7 @@ thread_db_get_thread_local_address (struct target_ops *ops,
PR libc/16831 due to GDB PR threads/16954 LOAD_MODULE is also NULL.
The constant number 1 depends on GNU __libc_setup_tls
initialization of l_tls_modid to 1. */
- err = info->td_thr_tlsbase_p (&thread_info->priv->th,
- 1, &address);
+ err = info->td_thr_tlsbase_p (&priv->th, 1, &address);
address = (char *) address + offset;
}
diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
index 1da1a98..5906eb6 100644
--- a/gdb/nto-procfs.c
+++ b/gdb/nto-procfs.c
@@ -248,38 +248,24 @@ static void
update_thread_private_data_name (struct thread_info *new_thread,
const char *newname)
{
- int newnamelen;
- struct private_thread_info *pti;
+ nto_thread_info *pti = (nto_thread_info *) new_thread->priv.get ();
gdb_assert (newname != NULL);
gdb_assert (new_thread != NULL);
- newnamelen = strlen (newname);
- if (!new_thread->priv)
- {
- new_thread->priv = xmalloc (offsetof (struct private_thread_info,
- name)
- + newnamelen + 1);
- memcpy (new_thread->priv->name, newname, newnamelen + 1);
- }
- else if (strcmp (newname, new_thread->priv->name) != 0)
+
+ if (pti)
{
- /* Reallocate if neccessary. */
- int oldnamelen = strlen (new_thread->priv->name);
-
- if (oldnamelen < newnamelen)
- new_thread->priv = xrealloc (new_thread->priv,
- offsetof (struct private_thread_info,
- name)
- + newnamelen + 1);
- memcpy (new_thread->priv->name, newname, newnamelen + 1);
+ pti = new nto_thread_info;
+ new_thread->priv.reset (pti);
}
+
+ pti->name = newname;
}
static void
update_thread_private_data (struct thread_info *new_thread,
pthread_t tid, int state, int flags)
{
- struct private_thread_info *pti;
procfs_info pidinfo;
struct _thread_name *tn;
procfs_threadctl tctl;
@@ -306,7 +292,7 @@ update_thread_private_data (struct thread_info *new_thread,
update_thread_private_data_name (new_thread, tn->name_buf);
- pti = (struct private_thread_info *) new_thread->priv;
+ nto_thread_info *pti = (nto_thread_info *) new_thread->priv.get ();
pti->tid = tid;
pti->state = state;
pti->flags = flags;
diff --git a/gdb/nto-tdep.c b/gdb/nto-tdep.c
index 27bd191..97b940a 100644
--- a/gdb/nto-tdep.c
+++ b/gdb/nto-tdep.c
@@ -380,9 +380,13 @@ static const char *nto_thread_state_str[] =
const char *
nto_extra_thread_info (struct target_ops *self, struct thread_info *ti)
{
- if (ti && ti->priv
- && ti->priv->state < ARRAY_SIZE (nto_thread_state_str))
- return (char *)nto_thread_state_str [ti->priv->state];
+ if (ti != NULL && ti->priv != NULL)
+ {
+ nto_thread_info *priv = (nto_thread_info *) ti->priv.get ();
+
+ if (priv->state < ARRAY_SIZE (nto_thread_state_str))
+ return nto_thread_state_str [priv->state];
+ }
return "";
}
diff --git a/gdb/nto-tdep.h b/gdb/nto-tdep.h
index afe3452..46acba2 100644
--- a/gdb/nto-tdep.h
+++ b/gdb/nto-tdep.h
@@ -134,12 +134,12 @@ typedef struct _debug_regs
qnx_reg64 padding[1024];
} nto_regset_t;
-struct private_thread_info
+struct nto_thread_info : public private_thread_info
{
- short tid;
- unsigned char state;
- unsigned char flags;
- char name[1];
+ short tid = 0;
+ unsigned char state = 0;
+ unsigned char flags = 0;
+ std::string name;
};
/* Per-inferior data, common for both procfs and remote. */
diff --git a/gdb/remote.c b/gdb/remote.c
index 0270f95..244ab32 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -439,23 +439,23 @@ struct remote_state
struct readahead_cache readahead_cache;
};
-/* Private data that we'll store in (struct thread_info)->private. */
-struct private_thread_info
+/* Private data that we'll store in (struct thread_info)->priv. */
+struct remote_thread_info : public private_thread_info
{
- char *extra;
- char *name;
- int core;
+ std::string extra;
+ std::string name;
+ int core = -1;
/* Thread handle, perhaps a pthread_t or thread_t value, stored as a
sequence of bytes. */
- gdb::byte_vector *thread_handle;
+ gdb::byte_vector thread_handle;
/* Whether the target stopped for a breakpoint/watchpoint. */
- enum target_stop_reason stop_reason;
+ enum target_stop_reason stop_reason = TARGET_STOPPED_BY_NO_REASON;
/* This is set to the data address of the access causing the target
to stop for a watchpoint. */
- CORE_ADDR watch_data_address;
+ CORE_ADDR watch_data_address = 0;
/* Fields used by the vCont action coalescing implemented in
remote_resume / remote_commit_resume. remote_resume stores each
@@ -465,26 +465,17 @@ struct private_thread_info
/* True if the last target_resume call for this thread was a step
request, false if a continue request. */
- int last_resume_step;
+ int last_resume_step = 0;
/* The signal specified in the last target_resume call for this
thread. */
- enum gdb_signal last_resume_sig;
+ gdb_signal last_resume_sig = GDB_SIGNAL_0;
/* Whether this thread was already vCont-resumed on the remote
side. */
- int vcont_resumed;
+ int vcont_resumed = 0;
};
-static void
-free_private_thread_info (struct private_thread_info *info)
-{
- xfree (info->extra);
- xfree (info->name);
- delete info->thread_handle;
- xfree (info);
-}
-
/* This data could be associated with a target, but we do not always
have access to the current target when we need it, so for now it is
static. This will be fine for as long as only one target is in use
@@ -1826,8 +1817,8 @@ remote_add_inferior (int fake_pid_p, int pid, int attached,
return inf;
}
-static struct private_thread_info *
- get_private_info_thread (struct thread_info *info);
+static remote_thread_info *
+ get_private_info_thread (thread_info *thread);
/* Add thread PTID to GDB's thread list. Tag it as executing/running
according to RUNNING. */
@@ -1946,34 +1937,20 @@ remote_notice_new_inferior (ptid_t currthread, int executing)
/* Return THREAD's private thread data, creating it if necessary. */
-static struct private_thread_info *
-get_private_info_thread (struct thread_info *thread)
+static remote_thread_info *
+get_private_info_thread (thread_info *thread)
{
gdb_assert (thread != NULL);
if (thread->priv == NULL)
- {
- struct private_thread_info *priv = XNEW (struct private_thread_info);
-
- thread->private_dtor = free_private_thread_info;
- thread->priv = priv;
+ thread->priv.reset (new remote_thread_info);
- priv->core = -1;
- priv->extra = NULL;
- priv->name = NULL;
- priv->name = NULL;
- priv->last_resume_step = 0;
- priv->last_resume_sig = GDB_SIGNAL_0;
- priv->vcont_resumed = 0;
- priv->thread_handle = nullptr;
- }
-
- return thread->priv;
+ return (remote_thread_info *) thread->priv.get ();
}
/* Return PTID's private thread data, creating it if necessary. */
-static struct private_thread_info *
+static remote_thread_info *
get_private_info_ptid (ptid_t ptid)
{
struct thread_info *info = find_thread_ptid (ptid);
@@ -2294,7 +2271,7 @@ static const char *
remote_thread_name (struct target_ops *ops, struct thread_info *info)
{
if (info->priv != NULL)
- return info->priv->name;
+ return get_private_info_thread (info)->name.c_str ();
return NULL;
}
@@ -2978,11 +2955,6 @@ struct thread_item
: ptid (ptid_)
{}
- ~thread_item ()
- {
- delete thread_handle;
- }
-
/* The thread's PTID. */
ptid_t ptid;
@@ -2996,8 +2968,7 @@ struct thread_item
int core = -1;
/* The thread handle associated with the thread. */
- gdb::byte_vector *thread_handle = NULL;
-
+ gdb::byte_vector thread_handle;
};
/* Context passed around to the various methods listing remote
@@ -3123,10 +3094,7 @@ start_thread (struct gdb_xml_parser *parser,
attr = xml_find_attribute (attributes, "handle");
if (attr != NULL)
- {
- item.thread_handle = new gdb::byte_vector;
- *item.thread_handle = hex2bin ((const char *) attr->value);
- }
+ item.thread_handle = hex2bin ((const char *) attr->value);
}
static void
@@ -3288,7 +3256,6 @@ remote_update_thread_list (struct target_ops *ops)
{
if (item.ptid != null_ptid)
{
- struct private_thread_info *info;
/* In non-stop mode, we assume new found threads are
executing until proven otherwise with a stop reply.
In all-stop, we can only get here if all threads are
@@ -3297,12 +3264,12 @@ remote_update_thread_list (struct target_ops *ops)
remote_notice_new_inferior (item.ptid, executing);
- info = get_private_info_ptid (item.ptid);
+ remote_thread_info *info
+ = get_private_info_ptid (item.ptid);
info->core = item.core;
- info->extra = xstrdup (item.extra.c_str ());
- info->name = xstrdup (item.name.c_str ());
- info->thread_handle = item.thread_handle;
- item.thread_handle = nullptr;
+ info->extra = std::move (item.extra);
+ info->name = std::move (item.name);
+ info->thread_handle = std::move (item.thread_handle);
}
}
}
@@ -3351,8 +3318,8 @@ remote_threads_extra_info (struct target_ops *self, struct thread_info *tp)
{
struct thread_info *info = find_thread_ptid (tp->ptid);
- if (info && info->priv)
- return info->priv->extra;
+ if (info != NULL && info->priv != NULL)
+ return get_private_info_thread (info)->extra.c_str ();
else
return NULL;
}
@@ -3928,7 +3895,7 @@ process_initial_stop_replies (int from_tty)
set_executing (event_ptid, 0);
set_running (event_ptid, 0);
- thread->priv->vcont_resumed = 0;
+ get_private_info_thread (thread)->vcont_resumed = 0;
}
/* "Notice" the new inferiors before anything related to
@@ -5552,8 +5519,10 @@ resume_clear_thread_private_info (struct thread_info *thread)
{
if (thread->priv != NULL)
{
- thread->priv->stop_reason = TARGET_STOPPED_BY_NO_REASON;
- thread->priv->watch_data_address = 0;
+ remote_thread_info *priv = get_private_info_thread (thread);
+
+ priv->stop_reason = TARGET_STOPPED_BY_NO_REASON;
+ priv->watch_data_address = 0;
}
}
@@ -5733,12 +5702,13 @@ remote_resume (struct target_ops *ops,
to do vCont action coalescing. */
if (target_is_non_stop_p () && execution_direction != EXEC_REVERSE)
{
- struct private_thread_info *remote_thr;
+ remote_thread_info *remote_thr;
if (ptid_equal (minus_one_ptid, ptid) || ptid_is_pid (ptid))
remote_thr = get_private_info_ptid (inferior_ptid);
else
remote_thr = get_private_info_ptid (ptid);
+
remote_thr->last_resume_step = step;
remote_thr->last_resume_sig = siggnal;
return;
@@ -6003,7 +5973,7 @@ remote_commit_resume (struct target_ops *ops)
/* Threads first. */
ALL_NON_EXITED_THREADS (tp)
{
- struct private_thread_info *remote_thr = tp->priv;
+ remote_thread_info *remote_thr = get_private_info_thread (tp);
if (!tp->executing || remote_thr->vcont_resumed)
continue;
@@ -7220,8 +7190,6 @@ process_stop_reply (struct stop_reply *stop_reply,
&& status->kind != TARGET_WAITKIND_SIGNALLED
&& status->kind != TARGET_WAITKIND_NO_RESUMED)
{
- struct private_thread_info *remote_thr;
-
/* Expedited registers. */
if (stop_reply->regcache)
{
@@ -7242,7 +7210,7 @@ process_stop_reply (struct stop_reply *stop_reply,
}
remote_notice_new_inferior (ptid, 0);
- remote_thr = get_private_info_ptid (ptid);
+ remote_thread_info *remote_thr = get_private_info_ptid (ptid);
remote_thr->core = stop_reply->core;
remote_thr->stop_reason = stop_reply->stop_reason;
remote_thr->watch_data_address = stop_reply->watch_data_address;
@@ -10039,7 +10007,8 @@ remote_stopped_by_sw_breakpoint (struct target_ops *ops)
struct thread_info *thread = inferior_thread ();
return (thread->priv != NULL
- && thread->priv->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT);
+ && (get_private_info_thread (thread)->stop_reason
+ == TARGET_STOPPED_BY_SW_BREAKPOINT));
}
/* The to_supports_stopped_by_sw_breakpoint method of target
@@ -10059,7 +10028,8 @@ remote_stopped_by_hw_breakpoint (struct target_ops *ops)
struct thread_info *thread = inferior_thread ();
return (thread->priv != NULL
- && thread->priv->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT);
+ && (get_private_info_thread (thread)->stop_reason
+ == TARGET_STOPPED_BY_HW_BREAKPOINT));
}
/* The to_supports_stopped_by_hw_breakpoint method of target
@@ -10077,7 +10047,8 @@ remote_stopped_by_watchpoint (struct target_ops *ops)
struct thread_info *thread = inferior_thread ();
return (thread->priv != NULL
- && thread->priv->stop_reason == TARGET_STOPPED_BY_WATCHPOINT);
+ && (get_private_info_thread (thread)->stop_reason
+ == TARGET_STOPPED_BY_WATCHPOINT));
}
static int
@@ -10086,9 +10057,10 @@ remote_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p)
struct thread_info *thread = inferior_thread ();
if (thread->priv != NULL
- && thread->priv->stop_reason == TARGET_STOPPED_BY_WATCHPOINT)
+ && (get_private_info_thread (thread)->stop_reason
+ == TARGET_STOPPED_BY_WATCHPOINT))
{
- *addr_p = thread->priv->watch_data_address;
+ *addr_p = get_private_info_thread (thread)->watch_data_address;
return 1;
}
@@ -12925,8 +12897,9 @@ remote_core_of_thread (struct target_ops *ops, ptid_t ptid)
{
struct thread_info *info = find_thread_ptid (ptid);
- if (info && info->priv)
- return info->priv->core;
+ if (info != NULL && info->priv != NULL)
+ return get_private_info_thread (info)->core;
+
return -1;
}
@@ -13523,14 +13496,14 @@ remote_thread_handle_to_thread_info (struct target_ops *ops,
ALL_NON_EXITED_THREADS (tp)
{
- struct private_thread_info *priv = get_private_info_thread (tp);
+ remote_thread_info *priv = get_private_info_thread (tp);
if (tp->inf == inf && priv != NULL)
{
- if (handle_len != priv->thread_handle->size ())
+ if (handle_len != priv->thread_handle.size ())
error (_("Thread handle size mismatch: %d vs %zu (from remote)"),
- handle_len, priv->thread_handle->size ());
- if (memcmp (thread_handle, priv->thread_handle->data (),
+ handle_len, priv->thread_handle.size ());
+ if (memcmp (thread_handle, priv->thread_handle.data (),
handle_len) == 0)
return tp;
}
diff --git a/gdb/thread.c b/gdb/thread.c
index d71568e..052549a 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -316,11 +316,11 @@ add_thread_silent (ptid_t ptid)
}
struct thread_info *
-add_thread_with_info (ptid_t ptid, struct private_thread_info *priv)
+add_thread_with_info (ptid_t ptid, private_thread_info *priv)
{
struct thread_info *result = add_thread_silent (ptid);
- result->priv = priv;
+ result->priv.reset (priv);
if (print_thread_events)
printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
@@ -335,6 +335,8 @@ add_thread (ptid_t ptid)
return add_thread_with_info (ptid, NULL);
}
+private_thread_info::~private_thread_info () = default;
+
thread_info::thread_info (struct inferior *inf_, ptid_t ptid_)
: ptid (ptid_), inf (inf_)
{
@@ -351,14 +353,6 @@ thread_info::thread_info (struct inferior *inf_, ptid_t ptid_)
thread_info::~thread_info ()
{
- if (this->priv)
- {
- if (this->private_dtor)
- this->private_dtor (this->priv);
- else
- xfree (this->priv);
- }
-
xfree (this->name);
}
--
2.7.4
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] Create private_thread_info hierarchy
2017-11-22 16:42 ` [PATCH 3/4] Create private_thread_info hierarchy Simon Marchi
@ 2017-11-23 14:41 ` Pedro Alves
2017-11-23 16:54 ` Simon Marchi
0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2017-11-23 14:41 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Simon Marchi
On 11/22/2017 04:41 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Including gdbthread.h from darwin-nat.h gave these errors:
>
> /Users/simark/src/binutils-gdb/gdb/gdbthread.h:609:3: error: must use 'class' tag to refer to type 'thread_info' in this scope
> thread_info *m_thread;
> ^
> class
> /usr/include/mach/thread_act.h:240:15: note: class 'thread_info' is hidden by a non-type declaration of 'thread_info' here
> kern_return_t thread_info
> ^
>
> It turns out that there is a thread_info function in the Darwin/XNU/mach API:
>
> http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/thread_info.html
>
> Therefore, I had to add the class keyword at a couple of places in gdbthread.h,
> I don't really see a way around it.
The way around it is to move all of gdb under "namespace gdb", and then
references to thread_info hit gdb::thread_info instead. :-)
> private:
> - thread_info *m_thread;
> + /* Use the "class" keyword here, because of a class with a "thread_info"
> + function in the Darwin API. */
> + class thread_info *m_thread;
This comment sounds incorrect. A a "thread_info" function/method
inside some class couldn't possibly interfere, right? The issue is
that there's a _free-function_ called thread_info, right?
Some nits below.
> @@ -776,8 +779,10 @@ sync_threadlists (void)
>
> if (cmp_result == 0)
> {
> - gbuf[gi]->priv->pdtid = pdtid;
> - gbuf[gi]->priv->tid = tid;
> + aix_thread_info *priv = (aix_thread_info *) gbuf[gi]->priv.get ();
...
> @@ -808,8 +815,9 @@ static int
> iter_tid (struct thread_info *thread, void *tidp)
> {
> const pthdb_tid_t tid = *(pthdb_tid_t *)tidp;
> + aix_thread_info *priv = (aix_thread_info *) thread->priv.get ();
This seems to scream for a "get_aix_thread_info (thread_info *);"
function.
>
> - return (thread->priv->tid == tid);
> + return priv->tid == tid;
> }
>
> @@ -1381,11 +1379,11 @@ thread_db_pid_to_str (struct target_ops *ops, ptid_t ptid)
> if (thread_info != NULL && thread_info->priv != NULL)
> {
> static char buf[64];
> - thread_t tid;
> + thread_db_thread_info *priv
> + = (thread_db_thread_info *) thread_info->priv.get ();
...
> - if (info->priv->dying)
> + thread_db_thread_info *priv = (thread_db_thread_info *) info->priv.get ();
> +
> + if (priv->dying)
> return "Exiting";
>
...
> return NULL;
> @@ -1434,7 +1434,9 @@ thread_db_thread_handle_to_thread_info (struct target_ops *ops,
>
> ALL_NON_EXITED_THREADS (tp)
> {
> - if (tp->inf == inf && tp->priv != NULL && handle_tid == tp->priv->tid)
> + thread_db_thread_info *priv = (thread_db_thread_info *) tp->priv.get ();
You know what I'll say, right? :-)
> +
> + if (tp->inf == inf && priv != NULL && handle_tid == priv->tid)
> return tp;
> }
>
> diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
> index 1da1a98..5906eb6 100644
> --- a/gdb/nto-procfs.c
> +++ b/gdb/nto-procfs.c
> @@ -248,38 +248,24 @@ static void
> update_thread_private_data_name (struct thread_info *new_thread,
> const char *newname)
> {
> - int newnamelen;
> - struct private_thread_info *pti;
> + nto_thread_info *pti = (nto_thread_info *) new_thread->priv.get ();
The comment really applies to all ports.
> -/* Private data that we'll store in (struct thread_info)->private. */
> -struct private_thread_info
> +/* Private data that we'll store in (struct thread_info)->priv. */
> +struct remote_thread_info : public private_thread_info
> {
> - char *extra;
> - char *name;
> - int core;
> + std::string extra;
> + std::string name;
> + int core = -1;
>
> /* Thread handle, perhaps a pthread_t or thread_t value, stored as a
> sequence of bytes. */
> - gdb::byte_vector *thread_handle;
> + gdb::byte_vector thread_handle;
Ah, OK, you did the pointer->value thing here. Great. :-)
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] Create private_thread_info hierarchy
2017-11-23 14:41 ` Pedro Alves
@ 2017-11-23 16:54 ` Simon Marchi
2017-11-23 16:56 ` Simon Marchi
0 siblings, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-11-23 16:54 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-11-23 09:41, Pedro Alves wrote:
> On 11/22/2017 04:41 PM, Simon Marchi wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>
>> Including gdbthread.h from darwin-nat.h gave these errors:
>>
>> /Users/simark/src/binutils-gdb/gdb/gdbthread.h:609:3: error: must use
>> 'class' tag to refer to type 'thread_info' in this scope
>> thread_info *m_thread;
>> ^
>> class
>> /usr/include/mach/thread_act.h:240:15: note: class 'thread_info' is
>> hidden by a non-type declaration of 'thread_info' here
>> kern_return_t thread_info
>> ^
>>
>> It turns out that there is a thread_info function in the
>> Darwin/XNU/mach API:
>>
>> http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/thread_info.html
>>
>> Therefore, I had to add the class keyword at a couple of places in
>> gdbthread.h,
>> I don't really see a way around it.
>
> The way around it is to move all of gdb under "namespace gdb", and then
> references to thread_info hit gdb::thread_info instead. :-)
But that would require us to improve gdb's C++ debugging support, so
that we don't have to type gdb:: every time! Is such thing even
possible?!
>> private:
>> - thread_info *m_thread;
>> + /* Use the "class" keyword here, because of a class with a
>> "thread_info"
>> + function in the Darwin API. */
>> + class thread_info *m_thread;
>
> This comment sounds incorrect. A a "thread_info" function/method
> inside some class couldn't possibly interfere, right? The issue is
> that there's a _free-function_ called thread_info, right?
Err yeah, the "class with a" part should not be there:
+ /* Use the "class" keyword here, because of a "thread_info"
+ function in the Darwin API. */
>
> Some nits below.
>
>> @@ -776,8 +779,10 @@ sync_threadlists (void)
>>
>> if (cmp_result == 0)
>> {
>> - gbuf[gi]->priv->pdtid = pdtid;
>> - gbuf[gi]->priv->tid = tid;
>> + aix_thread_info *priv = (aix_thread_info *) gbuf[gi]->priv.get
>> ();
>
> ...
>
>> @@ -808,8 +815,9 @@ static int
>> iter_tid (struct thread_info *thread, void *tidp)
>> {
>> const pthdb_tid_t tid = *(pthdb_tid_t *)tidp;
>> + aix_thread_info *priv = (aix_thread_info *) thread->priv.get ();
>
> This seems to scream for a "get_aix_thread_info (thread_info *);"
> function.
Will do.
>>
>> - return (thread->priv->tid == tid);
>> + return priv->tid == tid;
>> }
>>
>
>> @@ -1381,11 +1379,11 @@ thread_db_pid_to_str (struct target_ops *ops,
>> ptid_t ptid)
>> if (thread_info != NULL && thread_info->priv != NULL)
>> {
>> static char buf[64];
>> - thread_t tid;
>> + thread_db_thread_info *priv
>> + = (thread_db_thread_info *) thread_info->priv.get ();
>
> ...
>
>> - if (info->priv->dying)
>> + thread_db_thread_info *priv = (thread_db_thread_info *)
>> info->priv.get ();
>> +
>> + if (priv->dying)
>> return "Exiting";
>>
>
> ...
>
>> return NULL;
>> @@ -1434,7 +1434,9 @@ thread_db_thread_handle_to_thread_info (struct
>> target_ops *ops,
>>
>> ALL_NON_EXITED_THREADS (tp)
>> {
>> - if (tp->inf == inf && tp->priv != NULL && handle_tid ==
>> tp->priv->tid)
>> + thread_db_thread_info *priv = (thread_db_thread_info *)
>> tp->priv.get ();
>
> You know what I'll say, right? :-)
I think I'm starting to get the idea.
>> +
>> + if (tp->inf == inf && priv != NULL && handle_tid == priv->tid)
>> return tp;
>> }
>>
>
>> diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
>> index 1da1a98..5906eb6 100644
>> --- a/gdb/nto-procfs.c
>> +++ b/gdb/nto-procfs.c
>> @@ -248,38 +248,24 @@ static void
>> update_thread_private_data_name (struct thread_info *new_thread,
>> const char *newname)
>> {
>> - int newnamelen;
>> - struct private_thread_info *pti;
>> + nto_thread_info *pti = (nto_thread_info *) new_thread->priv.get ();
>
> The comment really applies to all ports.
Hehehe.
>> -/* Private data that we'll store in (struct thread_info)->private.
>> */
>> -struct private_thread_info
>> +/* Private data that we'll store in (struct thread_info)->priv. */
>> +struct remote_thread_info : public private_thread_info
>> {
>> - char *extra;
>> - char *name;
>> - int core;
>> + std::string extra;
>> + std::string name;
>> + int core = -1;
>>
>> /* Thread handle, perhaps a pthread_t or thread_t value, stored as
>> a
>> sequence of bytes. */
>> - gdb::byte_vector *thread_handle;
>> + gdb::byte_vector thread_handle;
>
> Ah, OK, you did the pointer->value thing here. Great. :-)
Yes, but as I mentioned in my other reply, I think it's better to do it
in the previous patch.
Thanks,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/4] Create private_thread_info hierarchy
2017-11-23 16:54 ` Simon Marchi
@ 2017-11-23 16:56 ` Simon Marchi
0 siblings, 0 replies; 19+ messages in thread
From: Simon Marchi @ 2017-11-23 16:56 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
>>> private:
>>> - thread_info *m_thread;
>>> + /* Use the "class" keyword here, because of a class with a
>>> "thread_info"
>>> + function in the Darwin API. */
>>> + class thread_info *m_thread;
>>
>> This comment sounds incorrect. A a "thread_info" function/method
>> inside some class couldn't possibly interfere, right? The issue is
>> that there's a _free-function_ called thread_info, right?
>
> Err yeah, the "class with a" part should not be there:
>
> + /* Use the "class" keyword here, because of a "thread_info"
> + function in the Darwin API. */
Ah actually I meant "because of a clash", I was one letter away.
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread