* [PATCH 2/3] gdbserver lwp_info: Initialize fields, use new/delete
2017-07-24 10:40 [PATCH 0/3] Create arch_lwp_info class hierarchy Simon Marchi
@ 2017-07-24 10:40 ` Simon Marchi
2017-07-25 9:58 ` Yao Qi
2017-07-24 10:40 ` [PATCH 1/3] gdb lwp_info: Add destructor, initialize " Simon Marchi
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2017-07-24 10:40 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
This patch minimally C++ifies gdbdserver's lwp_info, by initializing its
fields, using new/delete to (de-)allocate it and changing some field
types to bool. This is in preparation of using a unique_ptr in lwp_info
in the following patch.
gdb/gdbserver/ChangeLog:
* linux-low.h (struct lwp_info): Initialize fields,
<stop_expected, stopped, status_pending_p, stepping,
must_set_ptrace_flags, collecting_fast_tracepoint,
thread_known>: Change type to bool.
* linux-low.c (delete_lwp): De-allocate lwp_info with delete.
(handle_extended_wait): Adjust to bool.
(add_lwp): Allocate lwp_info with new.
(linux_create_inferior): Adjust to bool.
(linux_post_create_inferior): Likewise.
(linux_attach_lwp): Likewise.
(linux_attach): Likewise.
(linux_detach_one_lwp): Likewise.
(thread_still_has_status_pending_p): Likewise.
(maybe_move_out_of_jump_pad): Likewise.
(linux_low_filter_event): Likewise.
(linux_wait_for_event_filtered): Likewise.
(linux_wait_1): Likewise.
(send_sigstop): Likewise.
(mark_lwp_dead): Likewise.
(move_out_of_jump_pad_callback): Likewise.
(linux_resume_one_lwp_throw): Likewise.
(linux_set_resume_request): Likewise.
(proceed_one_lwp): Likewise.
(reset_lwp_ptrace_options_callback): Likewise.
* thread-db.c (find_one_thread): Likewise.
(attach_thread): Likewise.
---
gdb/gdbserver/linux-low.c | 94 +++++++++++++++++++++++------------------------
gdb/gdbserver/linux-low.h | 52 +++++++++++++-------------
gdb/gdbserver/thread-db.c | 4 +-
3 files changed, 73 insertions(+), 77 deletions(-)
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 3d7cfe3..e650b0d 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -415,7 +415,7 @@ delete_lwp (struct lwp_info *lwp)
remove_thread (thr);
free (lwp->arch_private);
- free (lwp);
+ delete lwp;
}
/* Add a process to the common process list, and set its private
@@ -535,9 +535,9 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
gdb_assert (child_proc != NULL);
child_lwp = add_lwp (ptid);
gdb_assert (child_lwp != NULL);
- child_lwp->stopped = 1;
- child_lwp->must_set_ptrace_flags = 1;
- child_lwp->status_pending_p = 0;
+ child_lwp->stopped = true;
+ child_lwp->must_set_ptrace_flags = true;
+ child_lwp->status_pending_p = false;
child_thr = get_lwp_thread (child_lwp);
child_thr->last_resume_kind = resume_stop;
child_thr->last_status.kind = TARGET_WAITKIND_STOPPED;
@@ -589,7 +589,7 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
/* The status_pending field contains bits denoting the
extended event, so when the pending event is handled,
the handler will look at lwp->waitstatus. */
- event_lwp->status_pending_p = 1;
+ event_lwp->status_pending_p = true;
event_lwp->status_pending = wstat;
/* Link the threads until the parent event is passed on to
@@ -630,7 +630,7 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
or leave it stopped. linux_resume_one_lwp is a nop if it
thinks the thread is currently running, so set this first
before calling linux_resume_one_lwp. */
- new_lwp->stopped = 1;
+ new_lwp->stopped = true;
/* If we're suspending all threads, leave this one suspended
too. If the fork/clone parent is stepping over a breakpoint,
@@ -645,14 +645,14 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
If we do get another signal, be sure not to lose it. */
if (WSTOPSIG (status) != SIGSTOP)
{
- new_lwp->stop_expected = 1;
- new_lwp->status_pending_p = 1;
+ new_lwp->stop_expected = true;
+ new_lwp->status_pending_p = true;
new_lwp->status_pending = status;
}
else if (report_thread_events)
{
new_lwp->waitstatus.kind = TARGET_WAITKIND_THREAD_CREATED;
- new_lwp->status_pending_p = 1;
+ new_lwp->status_pending_p = true;
new_lwp->status_pending = status;
}
@@ -712,8 +712,8 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
= xstrdup (linux_proc_pid_to_exec_file (lwpid_of (event_thr)));
/* Mark the exec status as pending. */
- event_lwp->stopped = 1;
- event_lwp->status_pending_p = 1;
+ event_lwp->stopped = true;
+ event_lwp->status_pending_p = true;
event_lwp->status_pending = wstat;
event_thr->last_resume_kind = resume_continue;
event_thr->last_status.kind = TARGET_WAITKIND_IGNORE;
@@ -935,11 +935,7 @@ save_stop_reason (struct lwp_info *lwp)
static struct lwp_info *
add_lwp (ptid_t ptid)
{
- struct lwp_info *lwp;
-
- lwp = XCNEW (struct lwp_info);
-
- lwp->waitstatus.kind = TARGET_WAITKIND_IGNORE;
+ lwp_info *lwp = new lwp_info;
if (the_low_target.new_thread != NULL)
the_low_target.new_thread (lwp);
@@ -1007,7 +1003,7 @@ linux_create_inferior (const char *program,
ptid = ptid_build (pid, pid, 0);
new_lwp = add_lwp (ptid);
- new_lwp->must_set_ptrace_flags = 1;
+ new_lwp->must_set_ptrace_flags = true;
post_fork_inferior (pid, program);
@@ -1029,7 +1025,7 @@ linux_post_create_inferior (void)
int options = linux_low_ptrace_options (proc->attached);
linux_enable_event_reporting (lwpid_of (current_thread), options);
- lwp->must_set_ptrace_flags = 0;
+ lwp->must_set_ptrace_flags = false;
}
}
@@ -1050,7 +1046,7 @@ linux_attach_lwp (ptid_t ptid)
/* We need to wait for SIGSTOP before being able to make the next
ptrace call on this LWP. */
- new_lwp->must_set_ptrace_flags = 1;
+ new_lwp->must_set_ptrace_flags = true;
if (linux_proc_pid_is_stopped (lwpid))
{
@@ -1113,7 +1109,7 @@ linux_attach_lwp (ptid_t ptid)
because we are guaranteed that the add_lwp call above added us to the
end of the list, and so the new thread has not yet reached
wait_for_sigstop (but will). */
- new_lwp->stop_expected = 1;
+ new_lwp->stop_expected = true;
return 0;
}
@@ -1220,7 +1216,7 @@ linux_attach (unsigned long pid)
if (!WIFSTOPPED (wstat) || WSTOPSIG (wstat) != SIGSTOP)
{
- lwp->status_pending_p = 1;
+ lwp->status_pending_p = true;
lwp->status_pending = wstat;
}
@@ -1514,7 +1510,7 @@ linux_detach_one_lwp (struct lwp_info *lwp)
target_pid_to_str (ptid_of (thread)));
kill_lwp (lwpid_of (thread), SIGCONT);
- lwp->stop_expected = 0;
+ lwp->stop_expected = false;
}
/* Pass on any pending signal for this thread. */
@@ -1774,7 +1770,7 @@ thread_still_has_status_pending_p (struct thread_info *thread)
{
if (debug_threads)
debug_printf ("discarding pending breakpoint status\n");
- lp->status_pending_p = 0;
+ lp->status_pending_p = false;
return 0;
}
}
@@ -2181,7 +2177,7 @@ maybe_move_out_of_jump_pad (struct lwp_info *lwp, int *wstat)
reporting to GDB. Otherwise, it's an IPA lib bug: just
report the signal to GDB, and pray for the best. */
- lwp->collecting_fast_tracepoint = 0;
+ lwp->collecting_fast_tracepoint = false;
if (r != 0
&& (status.adjusted_insn_addr <= lwp->stop_pc
@@ -2456,7 +2452,7 @@ linux_low_filter_event (int lwpid, int wstat)
child_ptid = ptid_build (lwpid, lwpid, 0);
child = add_lwp (child_ptid);
- child->stopped = 1;
+ child->stopped = true;
current_thread = child->thread;
}
@@ -2474,7 +2470,7 @@ linux_low_filter_event (int lwpid, int wstat)
thread = get_lwp_thread (child);
- child->stopped = 1;
+ child->stopped = true;
child->last_status = wstat;
@@ -2531,7 +2527,7 @@ linux_low_filter_event (int lwpid, int wstat)
/* The process is started, but GDBserver will do
architecture-specific setup after the program stops at
the first instruction. */
- child->status_pending_p = 1;
+ child->status_pending_p = true;
child->status_pending = wstat;
return child;
}
@@ -2544,7 +2540,7 @@ linux_low_filter_event (int lwpid, int wstat)
int options = linux_low_ptrace_options (proc->attached);
linux_enable_event_reporting (lwpid, options);
- child->must_set_ptrace_flags = 0;
+ child->must_set_ptrace_flags = false;
}
/* Always update syscall_state, even if it will be filtered later. */
@@ -2590,7 +2586,7 @@ linux_low_filter_event (int lwpid, int wstat)
{
if (debug_threads)
debug_printf ("Expected stop.\n");
- child->stop_expected = 0;
+ child->stop_expected = false;
if (thread->last_resume_kind == resume_stop)
{
@@ -2623,7 +2619,7 @@ linux_low_filter_event (int lwpid, int wstat)
}
}
- child->status_pending_p = 1;
+ child->status_pending_p = true;
child->status_pending = wstat;
return child;
}
@@ -2719,7 +2715,7 @@ linux_wait_for_event_filtered (ptid_t wait_ptid, ptid_t filter_ptid,
{
enqueue_one_deferred_signal (requested_child,
&requested_child->status_pending);
- requested_child->status_pending_p = 0;
+ requested_child->status_pending_p = false;
requested_child->status_pending = 0;
linux_resume_one_lwp (requested_child, 0, 0, NULL);
}
@@ -2745,7 +2741,7 @@ linux_wait_for_event_filtered (ptid_t wait_ptid, ptid_t filter_ptid,
debug_printf ("Got an event from pending child %ld (%04x)\n",
lwpid_of (event_thread), event_child->status_pending);
*wstatp = event_child->status_pending;
- event_child->status_pending_p = 0;
+ event_child->status_pending_p = false;
event_child->status_pending = 0;
current_thread = event_thread;
return lwpid_of (event_thread);
@@ -2820,7 +2816,7 @@ linux_wait_for_event_filtered (ptid_t wait_ptid, ptid_t filter_ptid,
{
event_child = get_thread_lwp (event_thread);
*wstatp = event_child->status_pending;
- event_child->status_pending_p = 0;
+ event_child->status_pending_p = false;
event_child->status_pending = 0;
break;
}
@@ -3480,7 +3476,7 @@ linux_wait_1 (ptid_t ptid,
event_child->collecting_fast_tracepoint
= linux_fast_tracepoint_collecting (event_child, NULL);
- if (event_child->collecting_fast_tracepoint != 1)
+ if (!event_child->collecting_fast_tracepoint)
{
/* No longer need this breakpoint. */
if (event_child->exit_jump_pad_bkpt != NULL)
@@ -3507,7 +3503,7 @@ linux_wait_1 (ptid_t ptid,
}
}
- if (event_child->collecting_fast_tracepoint == 0)
+ if (!event_child->collecting_fast_tracepoint)
{
if (debug_threads)
debug_printf ("fast tracepoint finished "
@@ -3845,7 +3841,7 @@ linux_wait_1 (ptid_t ptid,
starvation. */
if (ptid_equal (ptid, minus_one_ptid))
{
- event_child->status_pending_p = 1;
+ event_child->status_pending_p = true;
event_child->status_pending = w;
select_event_lwp (&event_child);
@@ -3853,7 +3849,7 @@ linux_wait_1 (ptid_t ptid,
/* current_thread and event_child must stay in sync. */
current_thread = get_lwp_thread (event_child);
- event_child->status_pending_p = 0;
+ event_child->status_pending_p = false;
w = event_child->status_pending;
}
@@ -4050,7 +4046,7 @@ send_sigstop (struct lwp_info *lwp)
if (debug_threads)
debug_printf ("Sending sigstop to lwp %d\n", pid);
- lwp->stop_expected = 1;
+ lwp->stop_expected = true;
kill_lwp (pid, SIGSTOP);
}
@@ -4093,7 +4089,7 @@ static void
mark_lwp_dead (struct lwp_info *lwp, int wstat)
{
/* Store the exit status for later. */
- lwp->status_pending_p = 1;
+ lwp->status_pending_p = true;
lwp->status_pending = wstat;
/* Store in waitstatus as well, as there's nothing else to process
@@ -4110,10 +4106,10 @@ mark_lwp_dead (struct lwp_info *lwp, int wstat)
}
/* Prevent trying to stop it. */
- lwp->stopped = 1;
+ lwp->stopped = true;
/* No further stops are expected from a dead lwp. */
- lwp->stop_expected = 0;
+ lwp->stop_expected = false;
}
/* Return true if LWP has exited already, and has a pending exit event
@@ -4229,7 +4225,7 @@ move_out_of_jump_pad_callback (struct inferior_list_entry *entry)
if (wstat)
{
- lwp->status_pending_p = 0;
+ lwp->status_pending_p = false;
enqueue_one_deferred_signal (lwp, wstat);
if (debug_threads)
@@ -4392,7 +4388,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
Code in this function that requires register access should be
guarded by proc->tdesc == NULL or something else. */
- if (lwp->stopped == 0)
+ if (!lwp->stopped)
return;
gdb_assert (lwp->waitstatus.kind == TARGET_WAITKIND_IGNORE);
@@ -4575,7 +4571,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
later try to stop the LWP and hang forever waiting for a stop
status. Note that we must not throw after this is cleared,
otherwise handle_zombie_lwp_error would get confused. */
- lwp->stopped = 0;
+ lwp->stopped = false;
lwp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
}
@@ -4606,7 +4602,7 @@ check_ptrace_stopped_lwp_gone (struct lwp_info *lp)
if (linux_proc_pid_is_trace_stopped_nowarn (lwpid_of (thread)) == 0)
{
lp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
- lp->status_pending_p = 0;
+ lp->status_pending_p = false;
return 1;
}
return 0;
@@ -4739,7 +4735,7 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg)
&& !lwp->status_pending_p
&& dequeue_one_deferred_signal (lwp, &lwp->status_pending))
{
- lwp->status_pending_p = 1;
+ lwp->status_pending_p = true;
if (debug_threads)
debug_printf ("Dequeueing deferred signal %d for LWP %ld, "
@@ -5294,7 +5290,7 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
if (thread->last_resume_kind == resume_stop
&& lwp->pending_signals_to_report == NULL
- && lwp->collecting_fast_tracepoint == 0)
+ && !lwp->collecting_fast_tracepoint)
{
/* We haven't reported this LWP as stopped yet (otherwise, the
last_status.kind check above would catch it, and we wouldn't
@@ -6466,7 +6462,7 @@ reset_lwp_ptrace_options_callback (struct inferior_list_entry *entry,
if (!lwp->stopped)
{
/* Stop the lwp so we can modify its ptrace options. */
- lwp->must_set_ptrace_flags = 1;
+ lwp->must_set_ptrace_flags = true;
linux_stop_lwp (lwp);
}
else
@@ -6476,7 +6472,7 @@ reset_lwp_ptrace_options_callback (struct inferior_list_entry *entry,
int options = linux_low_ptrace_options (proc->attached);
linux_enable_event_reporting (lwpid_of (thread), options);
- lwp->must_set_ptrace_flags = 0;
+ lwp->must_set_ptrace_flags = false;
}
return 0;
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 6328da0..dcc9315 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -267,7 +267,7 @@ extern struct linux_target_ops the_low_target;
struct lwp_info
{
/* Backlink to the parent object. */
- struct thread_info *thread;
+ struct thread_info *thread = NULL;
/* If this flag is set, the next SIGSTOP will be ignored (the
process will be immediately resumed). This means that either we
@@ -275,109 +275,109 @@ struct lwp_info
(so the SIGSTOP is still pending), or that we stopped the
inferior implicitly via PTRACE_ATTACH and have not waited for it
yet. */
- int stop_expected;
+ bool stop_expected = false;
- /* When this is true, we shall not try to resume this thread, even
+ /* When this is greater than 0, we shall not try to resume this thread, even
if last_resume_kind isn't resume_stop. */
- int suspended;
+ int suspended = 0;
/* If this flag is set, the lwp is known to be stopped right now (stop
event already received in a wait()). */
- int stopped;
+ bool stopped = false;
/* Signal whether we are in a SYSCALL_ENTRY or
in a SYSCALL_RETURN event.
Values:
- TARGET_WAITKIND_SYSCALL_ENTRY
- TARGET_WAITKIND_SYSCALL_RETURN */
- enum target_waitkind syscall_state;
+ enum target_waitkind syscall_state = TARGET_WAITKIND_IGNORE;
/* When stopped is set, the last wait status recorded for this lwp. */
- int last_status;
+ int last_status = 0;
/* If WAITSTATUS->KIND != TARGET_WAITKIND_IGNORE, the waitstatus for
this LWP's last event, to pass to GDB without any further
processing. This is used to store extended ptrace event
information or exit status until it can be reported to GDB. */
- struct target_waitstatus waitstatus;
+ struct target_waitstatus waitstatus = { TARGET_WAITKIND_IGNORE };
/* A pointer to the fork child/parent relative. Valid only while
the parent fork event is not reported to higher layers. Used to
avoid wildcard vCont actions resuming a fork child before GDB is
notified about the parent's fork event. */
- struct lwp_info *fork_relative;
+ struct lwp_info *fork_relative = NULL;
/* When stopped is set, this is where the lwp last stopped, with
decr_pc_after_break already accounted for. If the LWP is
running, this is the address at which the lwp was resumed. */
- CORE_ADDR stop_pc;
+ CORE_ADDR stop_pc = 0;
/* If this flag is set, STATUS_PENDING is a waitstatus that has not yet
been reported. */
- int status_pending_p;
- int status_pending;
+ bool status_pending_p = false;
+ int status_pending = 0;
/* The reason the LWP last stopped, if we need to track it
(breakpoint, watchpoint, etc.) */
- enum target_stop_reason stop_reason;
+ enum target_stop_reason stop_reason = TARGET_STOPPED_BY_NO_REASON;
/* On architectures where it is possible to know the data address of
a triggered watchpoint, STOPPED_DATA_ADDRESS is non-zero, and
contains such data address. Only valid if STOPPED_BY_WATCHPOINT
is true. */
- CORE_ADDR stopped_data_address;
+ CORE_ADDR stopped_data_address = 0;
/* If this is non-zero, it is a breakpoint to be reinserted at our next
stop (SIGTRAP stops only). */
- CORE_ADDR bp_reinsert;
+ CORE_ADDR bp_reinsert = 0;
/* If this flag is set, the last continue operation at the ptrace
level on this process was a single-step. */
- int stepping;
+ bool stepping = false;
/* Range to single step within. This is a copy of the step range
passed along the last resume request. See 'struct
thread_resume'. */
- CORE_ADDR step_range_start; /* Inclusive */
- CORE_ADDR step_range_end; /* Exclusive */
+ CORE_ADDR step_range_start = 0; /* Inclusive */
+ CORE_ADDR step_range_end = 0; /* Exclusive */
/* If this flag is set, we need to set the event request flags the
next time we see this LWP stop. */
- int must_set_ptrace_flags;
+ bool must_set_ptrace_flags = false;
/* If this is non-zero, it points to a chain of signals which need to
be delivered to this process. */
- struct pending_signals *pending_signals;
+ struct pending_signals *pending_signals = NULL;
/* A link used when resuming. It is initialized from the resume request,
and then processed and cleared in linux_resume_one_lwp. */
- struct thread_resume *resume;
+ struct thread_resume *resume = NULL;
/* True if it is known that this lwp is presently collecting a fast
tracepoint (it is in the jump pad or in some code that will
return to the jump pad. Normally, we won't care about this, but
we will if a signal arrives to this lwp while it is
collecting. */
- int collecting_fast_tracepoint;
+ bool collecting_fast_tracepoint = false;
/* If this is non-zero, it points to a chain of signals which need
to be reported to GDB. These were deferred because the thread
was doing a fast tracepoint collect when they arrived. */
- struct pending_signals *pending_signals_to_report;
+ struct pending_signals *pending_signals_to_report = NULL;
/* When collecting_fast_tracepoint is first found to be 1, we insert
a exit-jump-pad-quickly breakpoint. This is it. */
- struct breakpoint *exit_jump_pad_bkpt;
+ struct breakpoint *exit_jump_pad_bkpt = NULL;
#ifdef USE_THREAD_DB
- int thread_known;
+ bool thread_known = false;
/* The thread handle, used for e.g. TLS access. Only valid if
THREAD_KNOWN is set. */
td_thrhandle_t th;
#endif
/* Arch-specific additions. */
- struct arch_lwp_info *arch_private;
+ struct arch_lwp_info *arch_private = NULL;
};
int linux_pid_exe_is_elf_64_file (int pid, unsigned int *machine);
diff --git a/gdb/gdbserver/thread-db.c b/gdb/gdbserver/thread-db.c
index 1ffb79d..4bb3c59 100644
--- a/gdb/gdbserver/thread-db.c
+++ b/gdb/gdbserver/thread-db.c
@@ -198,7 +198,7 @@ find_one_thread (ptid_t ptid)
if (ti.ti_tid == 0)
return 0;
- lwp->thread_known = 1;
+ lwp->thread_known = true;
lwp->th = th;
return 1;
@@ -229,7 +229,7 @@ attach_thread (const td_thrhandle_t *th_p, td_thrinfo_t *ti_p)
lwp = find_lwp_pid (ptid);
gdb_assert (lwp != NULL);
- lwp->thread_known = 1;
+ lwp->thread_known = true;
lwp->th = *th_p;
return 1;
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] gdbserver lwp_info: Initialize fields, use new/delete
2017-07-24 10:40 ` [PATCH 2/3] gdbserver lwp_info: Initialize fields, use new/delete Simon Marchi
@ 2017-07-25 9:58 ` Yao Qi
2017-07-25 10:19 ` Simon Marchi
0 siblings, 1 reply; 15+ messages in thread
From: Yao Qi @ 2017-07-25 9:58 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
Simon Marchi <simon.marchi@ericsson.com> writes:
> @@ -3480,7 +3476,7 @@ linux_wait_1 (ptid_t ptid,
> event_child->collecting_fast_tracepoint
> = linux_fast_tracepoint_collecting (event_child, NULL);
>
> - if (event_child->collecting_fast_tracepoint != 1)
> + if (!event_child->collecting_fast_tracepoint)
> {
> /* No longer need this breakpoint. */
> if (event_child->exit_jump_pad_bkpt != NULL)
linux_fast_tracepoint_collecting doesn't return boolean, it returns 0, 1
and 2. See comments in tracepoint.c:fast_tracepoint_collecting.
--
Yao (齐尧)
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] gdbserver lwp_info: Initialize fields, use new/delete
2017-07-25 9:58 ` Yao Qi
@ 2017-07-25 10:19 ` Simon Marchi
2017-07-25 15:25 ` Yao Qi
0 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2017-07-25 10:19 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 2017-07-25 11:58 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
>
>> @@ -3480,7 +3476,7 @@ linux_wait_1 (ptid_t ptid,
>> event_child->collecting_fast_tracepoint
>> = linux_fast_tracepoint_collecting (event_child, NULL);
>>
>> - if (event_child->collecting_fast_tracepoint != 1)
>> + if (!event_child->collecting_fast_tracepoint)
>> {
>> /* No longer need this breakpoint. */
>> if (event_child->exit_jump_pad_bkpt != NULL)
>
> linux_fast_tracepoint_collecting doesn't return boolean, it returns 0, 1
> and 2. See comments in tracepoint.c:fast_tracepoint_collecting.
Oh, thanks for noticing this! I think I've been misguided be the comment on
linux_fast_tracepoint_collecting that says "true":
/* Convenience wrapper. Returns true if LWP is presently collecting a
fast tracepoint. */
static int
linux_fast_tracepoint_collecting (struct lwp_info *lwp,
struct fast_tpoint_collect_status *status)
Maybe we should make it return an enum so it's clear, because the function name
really sounds like it would return a bool.
Here's the a fixup to this patch that restores the original code:
From 61b9c0b842ef8ecddd0ef8a1be12676d6f9e61ba Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Tue, 25 Jul 2017 12:15:33 +0200
Subject: [PATCH] fixup fast_tracepoint_collecting does not return bool
---
gdb/gdbserver/linux-low.c | 8 ++++----
gdb/gdbserver/linux-low.h | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e650b0d..ab3e860 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2177,7 +2177,7 @@ maybe_move_out_of_jump_pad (struct lwp_info *lwp, int *wstat)
reporting to GDB. Otherwise, it's an IPA lib bug: just
report the signal to GDB, and pray for the best. */
- lwp->collecting_fast_tracepoint = false;
+ lwp->collecting_fast_tracepoint = 0;
if (r != 0
&& (status.adjusted_insn_addr <= lwp->stop_pc
@@ -3476,7 +3476,7 @@ linux_wait_1 (ptid_t ptid,
event_child->collecting_fast_tracepoint
= linux_fast_tracepoint_collecting (event_child, NULL);
- if (!event_child->collecting_fast_tracepoint)
+ if (event_child->collecting_fast_tracepoint != 1)
{
/* No longer need this breakpoint. */
if (event_child->exit_jump_pad_bkpt != NULL)
@@ -3503,7 +3503,7 @@ linux_wait_1 (ptid_t ptid,
}
}
- if (!event_child->collecting_fast_tracepoint)
+ if (event_child->collecting_fast_tracepoint == 0)
{
if (debug_threads)
debug_printf ("fast tracepoint finished "
@@ -5290,7 +5290,7 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
if (thread->last_resume_kind == resume_stop
&& lwp->pending_signals_to_report == NULL
- && !lwp->collecting_fast_tracepoint)
+ && lwp->collecting_fast_tracepoint == 0)
{
/* We haven't reported this LWP as stopped yet (otherwise, the
last_status.kind check above would catch it, and we wouldn't
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index dcc9315..f93aefb 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -358,7 +358,7 @@ struct lwp_info
return to the jump pad. Normally, we won't care about this, but
we will if a signal arrives to this lwp while it is
collecting. */
- bool collecting_fast_tracepoint = false;
+ int collecting_fast_tracepoint = 0;
/* If this is non-zero, it points to a chain of signals which need
to be reported to GDB. These were deferred because the thread
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] gdbserver lwp_info: Initialize fields, use new/delete
2017-07-25 10:19 ` Simon Marchi
@ 2017-07-25 15:25 ` Yao Qi
0 siblings, 0 replies; 15+ messages in thread
From: Yao Qi @ 2017-07-25 15:25 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
Simon Marchi <simon.marchi@ericsson.com> writes:
> Oh, thanks for noticing this! I think I've been misguided be the comment on
> linux_fast_tracepoint_collecting that says "true":
>
> /* Convenience wrapper. Returns true if LWP is presently collecting a
> fast tracepoint. */
>
> static int
> linux_fast_tracepoint_collecting (struct lwp_info *lwp,
> struct fast_tpoint_collect_status *status)
>
> Maybe we should make it return an enum so it's clear, because the function name
> really sounds like it would return a bool.
>
Agreed. The comments to lwp_info.collecting_fast_tracepoint should be
updated as well.
>
> Here's the a fixup to this patch that restores the original code:
The patch this fixup is good to me.
--
Yao (齐尧)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] gdb lwp_info: Add destructor, initialize fields, use new/delete
2017-07-24 10:40 [PATCH 0/3] Create arch_lwp_info class hierarchy Simon Marchi
2017-07-24 10:40 ` [PATCH 2/3] gdbserver lwp_info: Initialize fields, use new/delete Simon Marchi
@ 2017-07-24 10:40 ` Simon Marchi
2017-07-24 10:43 ` Simon Marchi
2017-07-25 9:39 ` Yao Qi
2017-07-24 10:40 ` [PATCH 3/3] Create arch_lwp_info class hierarchy Simon Marchi
2017-08-11 20:13 ` [PATCH 0/3] " Pedro Alves
3 siblings, 2 replies; 15+ messages in thread
From: Simon Marchi @ 2017-07-24 10:40 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@polymtl.ca>
Partially convert the lwp_info type to C++ by adding a constructor and
initializing fields directly in the struct declaration. Also, change a
few fields to bool.
I did this patch in order to be able to use a unique_ptr field in a
following patch.
gdb/ChangeLog:
* linux-nat.h (struct lwp_info): Add constructor, initialize
fields with default values.
<must_set_ptrace_flags, signalled, stopped, resumed, step,
stopped_data_address, ignore_sigint>: Change type to bool.
* linux-nat.c (linux_child_follow_fork): Adapt to type change to
bool.
(lwp_free): Use delete on lp.
(add_initial_lwp): Create lwp_info by calling lwp_info constructor.
(delete_lwp): Likewise.
(find_lwp_pid): Likewise.
(linux_nat_switch_fork): Adapt to type change to bool.
(linux_nat_post_attach_wait): Change type of SIGNALLED parameter
to bool *.
(attach_proc_task_lwp_callback): Adapt to type change to bool.
(linux_nat_attach): Adapt to type change to bool.
(detach_one_lwp): Likewise.
(linux_resume_one_lwp_throw): Likewise.
(resume_clear_callback): Likewise.
(resume_set_callback): Likewise.
(linux_handle_syscall_trap): Likewise.
(linux_handle_extended_wait): Likewise.
(wait_lwp): Likewise.
(stop_callback): Likewise.
(set_ignore_sigint): Likewise.
(maybe_clear_ignore_sigint): Likewise.
(check_stopped_by_watchpoint): Likewise.
(stop_wait_callback): Likewise.
(linux_nat_filter_event): Likewise.
(linux_nat_wait_1): Likewise.
---
gdb/linux-nat.c | 92 +++++++++++++++++++++++++--------------------------------
gdb/linux-nat.h | 52 +++++++++++++++++---------------
2 files changed, 68 insertions(+), 76 deletions(-)
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 8b29245..9378276 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -493,7 +493,7 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
old_chain = save_inferior_ptid ();
inferior_ptid = child_ptid;
child_lp = add_lwp (inferior_ptid);
- child_lp->stopped = 1;
+ child_lp->stopped = true;
child_lp->last_resume_kind = resume_stop;
/* Detach new forked process? */
@@ -557,7 +557,7 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
fprintf_unfiltered (gdb_stdlog,
"LCFF: waiting for VFORK_DONE on %d\n",
parent_pid);
- parent_lp->stopped = 1;
+ parent_lp->stopped = true;
/* We'll handle the VFORK_DONE event like any other
event, in target_wait. */
@@ -608,7 +608,7 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
resuming the inferior. */
parent_lp->status = 0;
parent_lp->waitstatus.kind = TARGET_WAITKIND_VFORK_DONE;
- parent_lp->stopped = 1;
+ parent_lp->stopped = true;
/* If we're in async mode, need to tell the event loop
there's something here to process. */
@@ -622,7 +622,7 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
struct lwp_info *child_lp;
child_lp = add_lwp (inferior_ptid);
- child_lp->stopped = 1;
+ child_lp->stopped = true;
child_lp->last_resume_kind = resume_stop;
/* Let the thread_db layer learn about this new process. */
@@ -838,7 +838,7 @@ static void
lwp_free (struct lwp_info *lp)
{
xfree (lp->arch_private);
- xfree (lp);
+ delete lp;
}
/* Traversal function for purge_lwp_list. */
@@ -884,19 +884,9 @@ purge_lwp_list (int pid)
static struct lwp_info *
add_initial_lwp (ptid_t ptid)
{
- struct lwp_info *lp;
-
gdb_assert (ptid_lwp_p (ptid));
- lp = XNEW (struct lwp_info);
-
- memset (lp, 0, sizeof (struct lwp_info));
-
- lp->last_resume_kind = resume_continue;
- lp->waitstatus.kind = TARGET_WAITKIND_IGNORE;
-
- lp->ptid = ptid;
- lp->core = -1;
+ lwp_info *lp = new lwp_info (ptid);
/* Add to sorted-by-reverse-creation-order list. */
lwp_list_add (lp);
@@ -935,9 +925,8 @@ delete_lwp (ptid_t ptid)
{
struct lwp_info *lp;
void **slot;
- struct lwp_info dummy;
+ lwp_info dummy (ptid);
- dummy.ptid = ptid;
slot = htab_find_slot (lwp_lwpid_htab, &dummy, NO_INSERT);
if (slot == NULL)
return;
@@ -962,14 +951,13 @@ find_lwp_pid (ptid_t ptid)
{
struct lwp_info *lp;
int lwp;
- struct lwp_info dummy;
if (ptid_lwp_p (ptid))
lwp = ptid_get_lwp (ptid);
else
lwp = ptid_get_pid (ptid);
- dummy.ptid = ptid_build (0, lwp, 0);
+ lwp_info dummy (ptid_build (0, lwp, 0));
lp = (struct lwp_info *) htab_find (lwp_lwpid_htab, &dummy);
return lp;
}
@@ -1010,7 +998,7 @@ linux_nat_switch_fork (ptid_t new_ptid)
purge_lwp_list (ptid_get_pid (inferior_ptid));
lp = add_lwp (new_ptid);
- lp->stopped = 1;
+ lp->stopped = true;
/* This changes the thread's ptid while preserving the gdb thread
num. Also changes the inferior pid, while preserving the
@@ -1045,7 +1033,7 @@ exit_lwp (struct lwp_info *lp)
Returns a wait status for that LWP, to cache. */
static int
-linux_nat_post_attach_wait (ptid_t ptid, int *signalled)
+linux_nat_post_attach_wait (ptid_t ptid, bool *signalled)
{
pid_t new_pid, pid = ptid_get_lwp (ptid);
int status;
@@ -1093,7 +1081,7 @@ linux_nat_post_attach_wait (ptid_t ptid, int *signalled)
if (WSTOPSIG (status) != SIGSTOP)
{
- *signalled = 1;
+ *signalled = true;
if (debug_linux_nat)
fprintf_unfiltered (gdb_stdlog,
"LNPAW: Received %s after attaching\n",
@@ -1176,14 +1164,14 @@ attach_proc_task_lwp_callback (ptid_t ptid)
/* The next time we wait for this LWP we'll see a SIGSTOP as
PTRACE_ATTACH brings it to a halt. */
- lp->signalled = 1;
+ lp->signalled = true;
/* We need to wait for a stop before being able to make the
next ptrace call on this LWP. */
lp->must_set_ptrace_flags = 1;
/* So that wait collects the SIGSTOP. */
- lp->resumed = 1;
+ lp->resumed = true;
/* Also add the LWP to gdb's thread list, in case a
matching libthread_db is not found (or the process uses
@@ -1279,10 +1267,10 @@ linux_nat_attach (struct target_ops *ops, const char *args, int from_tty)
status, (long) ptid_get_lwp (ptid));
}
- lp->stopped = 1;
+ lp->stopped = true;
/* Save the wait status to report later. */
- lp->resumed = 1;
+ lp->resumed = true;
if (debug_linux_nat)
fprintf_unfiltered (gdb_stdlog,
"LNA: waitpid %ld, saving status %s\n",
@@ -1419,7 +1407,7 @@ detach_one_lwp (struct lwp_info *lp, int *signo_p)
target_pid_to_str (lp->ptid));
kill_lwp (lwpid, SIGCONT);
- lp->signalled = 0;
+ lp->signalled = false;
}
if (signo_p == NULL)
@@ -1582,7 +1570,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lp, int step,
later try to stop the LWP and hang forever waiting for a stop
status. Note that we must not throw after this is cleared,
otherwise handle_zombie_lwp_error would get confused. */
- lp->stopped = 0;
+ lp->stopped = false;
lp->core = -1;
lp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
registers_changed_ptid (lp->ptid);
@@ -1714,7 +1702,7 @@ linux_nat_resume_callback (struct lwp_info *lp, void *except)
static int
resume_clear_callback (struct lwp_info *lp, void *data)
{
- lp->resumed = 0;
+ lp->resumed = false;
lp->last_resume_kind = resume_stop;
return 0;
}
@@ -1722,7 +1710,7 @@ resume_clear_callback (struct lwp_info *lp, void *data)
static int
resume_set_callback (struct lwp_info *lp, void *data)
{
- lp->resumed = 1;
+ lp->resumed = true;
lp->last_resume_kind = resume_continue;
return 0;
}
@@ -1888,7 +1876,7 @@ linux_handle_syscall_trap (struct lwp_info *lp, int stopping)
lp->syscall_state = TARGET_WAITKIND_IGNORE;
ptrace (PTRACE_CONT, ptid_get_lwp (lp->ptid), 0, 0);
- lp->stopped = 0;
+ lp->stopped = false;
return 1;
}
@@ -2063,8 +2051,8 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
pid, new_pid);
new_lp = add_lwp (ptid_build (ptid_get_pid (lp->ptid), new_pid, 0));
- new_lp->stopped = 1;
- new_lp->resumed = 1;
+ new_lp->stopped = true;
+ new_lp->resumed = true;
/* If the thread_db layer is active, let it record the user
level thread id and status, and add the thread to GDB's
@@ -2093,7 +2081,7 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
fork / vfork than for clone, so we do not try - but
we handle it for clone events here. */
- new_lp->signalled = 1;
+ new_lp->signalled = true;
/* We created NEW_LP so it cannot yet contain STATUS. */
gdb_assert (new_lp->status == 0);
@@ -2133,7 +2121,7 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
/* The thread that execed must have been resumed, but, when a
thread execs, it changes its tid to the tgid, and the old
tgid thread might have not been resumed. */
- lp->resumed = 1;
+ lp->resumed = true;
return 0;
}
@@ -2280,7 +2268,7 @@ wait_lwp (struct lwp_info *lp)
}
gdb_assert (WIFSTOPPED (status));
- lp->stopped = 1;
+ lp->stopped = true;
if (lp->must_set_ptrace_flags)
{
@@ -2349,7 +2337,7 @@ stop_callback (struct lwp_info *lp, void *data)
errno ? safe_strerror (errno) : "ERRNO-OK");
}
- lp->signalled = 1;
+ lp->signalled = true;
gdb_assert (lp->status == 0);
}
@@ -2413,7 +2401,7 @@ set_ignore_sigint (struct lwp_info *lp, void *data)
&& WSTOPSIG (lp->status) == SIGINT)
lp->status = 0;
else
- lp->ignore_sigint = 1;
+ lp->ignore_sigint = true;
return 0;
}
@@ -2436,7 +2424,7 @@ maybe_clear_ignore_sigint (struct lwp_info *lp)
fprintf_unfiltered (gdb_stdlog,
"MCIS: Clearing bogus flag for %s\n",
target_pid_to_str (lp->ptid));
- lp->ignore_sigint = 0;
+ lp->ignore_sigint = false;
}
}
@@ -2475,7 +2463,7 @@ check_stopped_by_watchpoint (struct lwp_info *lp)
linux_ops->to_stopped_data_address (¤t_target,
&lp->stopped_data_address);
else
- lp->stopped_data_address_p = 0;
+ lp->stopped_data_address_p = false;
}
do_cleanups (old_chain);
@@ -2549,11 +2537,11 @@ stop_wait_callback (struct lwp_info *lp, void *data)
if (lp->ignore_sigint && WIFSTOPPED (status)
&& WSTOPSIG (status) == SIGINT)
{
- lp->ignore_sigint = 0;
+ lp->ignore_sigint = false;
errno = 0;
ptrace (PTRACE_CONT, ptid_get_lwp (lp->ptid), 0, 0);
- lp->stopped = 0;
+ lp->stopped = false;
if (debug_linux_nat)
fprintf_unfiltered (gdb_stdlog,
"PTRACE_CONT %s, 0, 0 (%s) "
@@ -2593,7 +2581,7 @@ stop_wait_callback (struct lwp_info *lp, void *data)
/* Reset SIGNALLED only after the stop_wait_callback call
above as it does gdb_assert on SIGNALLED. */
- lp->signalled = 0;
+ lp->signalled = false;
}
}
@@ -2995,8 +2983,8 @@ linux_nat_filter_event (int lwpid, int status)
lwpid);
lp = add_lwp (ptid_build (lwpid, lwpid, 0));
- lp->stopped = 1;
- lp->resumed = 1;
+ lp->stopped = true;
+ lp->resumed = true;
add_thread (lp->ptid);
}
@@ -3019,7 +3007,7 @@ linux_nat_filter_event (int lwpid, int status)
/* This LWP is stopped now. (And if dead, this prevents it from
ever being continued.) */
- lp->stopped = 1;
+ lp->stopped = true;
if (WIFSTOPPED (status) && lp->must_set_ptrace_flags)
{
@@ -3088,7 +3076,7 @@ linux_nat_filter_event (int lwpid, int status)
ptid_get_lwp (lp->ptid), lp->resumed);
/* Dead LWP's aren't expected to reported a pending sigstop. */
- lp->signalled = 0;
+ lp->signalled = false;
/* Store the pending event in the waitstatus, because
W_EXITCODE(0,0) == 0. */
@@ -3101,7 +3089,7 @@ linux_nat_filter_event (int lwpid, int status)
if (lp->signalled
&& WIFSTOPPED (status) && WSTOPSIG (status) == SIGSTOP)
{
- lp->signalled = 0;
+ lp->signalled = false;
if (lp->last_resume_kind == resume_stop)
{
@@ -3138,7 +3126,7 @@ linux_nat_filter_event (int lwpid, int status)
target_pid_to_str (lp->ptid));
/* This is a delayed SIGINT. */
- lp->ignore_sigint = 0;
+ lp->ignore_sigint = false;
linux_resume_one_lwp (lp, lp->step, GDB_SIGNAL_0);
if (debug_linux_nat)
@@ -3177,7 +3165,7 @@ linux_nat_filter_event (int lwpid, int status)
mark it as ignored for all LWPs except this one. */
iterate_over_lwps (pid_to_ptid (ptid_get_pid (lp->ptid)),
set_ignore_sigint, NULL);
- lp->ignore_sigint = 0;
+ lp->ignore_sigint = false;
}
else
maybe_clear_ignore_sigint (lp);
@@ -3314,7 +3302,7 @@ linux_nat_wait_1 (struct target_ops *ops,
ptid_get_pid (inferior_ptid), 0));
lp = add_initial_lwp (inferior_ptid);
- lp->resumed = 1;
+ lp->resumed = true;
}
/* Make sure SIGCHLD is blocked until the sigsuspend below. */
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 3378bd6..a783674 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -29,82 +29,86 @@ struct arch_lwp_info;
struct lwp_info
{
+ lwp_info (ptid_t ptid_)
+ : ptid (ptid_)
+ {}
+
/* The process id of the LWP. This is a combination of the LWP id
and overall process id. */
ptid_t ptid;
/* If this flag is set, we need to set the event request flags the
next time we see this LWP stop. */
- int must_set_ptrace_flags;
+ bool must_set_ptrace_flags = false;
- /* Non-zero if we sent this LWP a SIGSTOP (but the LWP didn't report
+ /* True if we sent this LWP a SIGSTOP (but the LWP didn't report
it back yet). */
- int signalled;
+ bool signalled = false;
- /* Non-zero if this LWP is stopped. */
- int stopped;
+ /* True if this LWP is stopped. */
+ bool stopped = false;
- /* Non-zero if this LWP will be/has been resumed. Note that an LWP
+ /* True if this LWP will be/has been resumed. Note that an LWP
can be marked both as stopped and resumed at the same time. This
happens if we try to resume an LWP that has a wait status
pending. We shouldn't let the LWP run until that wait status has
been processed, but we should not report that wait status if GDB
didn't try to let the LWP run. */
- int resumed;
+ bool resumed = false;
/* The last resume GDB requested on this thread. */
- enum resume_kind last_resume_kind;
+ resume_kind last_resume_kind = resume_continue;
/* If non-zero, a pending wait status. */
- int status;
+ int status = 0;
/* When 'stopped' is set, this is where the lwp last stopped, with
decr_pc_after_break already accounted for. If the LWP is
running, and stepping, this is the address at which the lwp was
resumed (that is, it's the previous stop PC). If the LWP is
running and not stepping, this is 0. */
- CORE_ADDR stop_pc;
+ CORE_ADDR stop_pc = 0;
- /* Non-zero if we were stepping this LWP. */
- int step;
+ /* True if we were stepping this LWP. */
+ bool step = false;
/* The reason the LWP last stopped, if we need to track it
(breakpoint, watchpoint, etc.) */
- enum target_stop_reason stop_reason;
+ target_stop_reason stop_reason = TARGET_STOPPED_BY_NO_REASON;
/* On architectures where it is possible to know the data address of
- a triggered watchpoint, STOPPED_DATA_ADDRESS_P is non-zero, and
+ a triggered watchpoint, STOPPED_DATA_ADDRESS_P is true, and
STOPPED_DATA_ADDRESS contains such data address. Otherwise,
STOPPED_DATA_ADDRESS_P is false, and STOPPED_DATA_ADDRESS is
undefined. Only valid if STOPPED_BY_WATCHPOINT is true. */
- int stopped_data_address_p;
- CORE_ADDR stopped_data_address;
+ bool stopped_data_address_p = false;
+ CORE_ADDR stopped_data_address = 0;
- /* Non-zero if we expect a duplicated SIGINT. */
- int ignore_sigint;
+ /* True if we expect a duplicated SIGINT. */
+ bool ignore_sigint = false;
/* If WAITSTATUS->KIND != TARGET_WAITKIND_SPURIOUS, the waitstatus
for this LWP's last event. This may correspond to STATUS above,
or to a local variable in lin_lwp_wait. */
- struct target_waitstatus waitstatus;
+ target_waitstatus waitstatus { TARGET_WAITKIND_IGNORE };
/* Signal whether we are in a SYSCALL_ENTRY or
in a SYSCALL_RETURN event.
Values:
- TARGET_WAITKIND_SYSCALL_ENTRY
- TARGET_WAITKIND_SYSCALL_RETURN */
- enum target_waitkind syscall_state;
+ target_waitkind syscall_state = TARGET_WAITKIND_IGNORE;
/* The processor core this LWP was last seen on. */
- int core;
+ int core = -1;
/* Arch-specific additions. */
- struct arch_lwp_info *arch_private;
+ arch_lwp_info *arch_private = NULL;
/* Previous and next pointers in doubly-linked list of known LWPs,
sorted by reverse creation order. */
- struct lwp_info *prev;
- struct lwp_info *next;
+ lwp_info *prev = NULL;
+ lwp_info *next = NULL;
};
/* The global list of LWPs, for ALL_LWPS. Unlike the threads list,
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] gdb lwp_info: Add destructor, initialize fields, use new/delete
2017-07-24 10:40 ` [PATCH 1/3] gdb lwp_info: Add destructor, initialize " Simon Marchi
@ 2017-07-24 10:43 ` Simon Marchi
2017-07-25 9:39 ` Yao Qi
1 sibling, 0 replies; 15+ messages in thread
From: Simon Marchi @ 2017-07-24 10:43 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
On 2017-07-24 12:39, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> Partially convert the lwp_info type to C++ by adding a constructor and
> initializing fields directly in the struct declaration. Also, change a
> few fields to bool.
>
> I did this patch in order to be able to use a unique_ptr field in a
> following patch.
Err, the title should say "constructor", not "destructor". Fixed it
locally.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] gdb lwp_info: Add destructor, initialize fields, use new/delete
2017-07-24 10:40 ` [PATCH 1/3] gdb lwp_info: Add destructor, initialize " Simon Marchi
2017-07-24 10:43 ` Simon Marchi
@ 2017-07-25 9:39 ` Yao Qi
1 sibling, 0 replies; 15+ messages in thread
From: Yao Qi @ 2017-07-25 9:39 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, Simon Marchi
Simon Marchi <simon.marchi@ericsson.com> writes:
> gdb/ChangeLog:
>
> * linux-nat.h (struct lwp_info): Add constructor, initialize
> fields with default values.
> <must_set_ptrace_flags, signalled, stopped, resumed, step,
> stopped_data_address, ignore_sigint>: Change type to bool.
> * linux-nat.c (linux_child_follow_fork): Adapt to type change to
> bool.
> (lwp_free): Use delete on lp.
> (add_initial_lwp): Create lwp_info by calling lwp_info constructor.
> (delete_lwp): Likewise.
> (find_lwp_pid): Likewise.
> (linux_nat_switch_fork): Adapt to type change to bool.
> (linux_nat_post_attach_wait): Change type of SIGNALLED parameter
> to bool *.
> (attach_proc_task_lwp_callback): Adapt to type change to bool.
> (linux_nat_attach): Adapt to type change to bool.
> (detach_one_lwp): Likewise.
> (linux_resume_one_lwp_throw): Likewise.
> (resume_clear_callback): Likewise.
> (resume_set_callback): Likewise.
> (linux_handle_syscall_trap): Likewise.
> (linux_handle_extended_wait): Likewise.
> (wait_lwp): Likewise.
> (stop_callback): Likewise.
> (set_ignore_sigint): Likewise.
> (maybe_clear_ignore_sigint): Likewise.
> (check_stopped_by_watchpoint): Likewise.
> (stop_wait_callback): Likewise.
> (linux_nat_filter_event): Likewise.
> (linux_nat_wait_1): Likewise.
Patch is good to me.
--
Yao (齐尧)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] Create arch_lwp_info class hierarchy
2017-07-24 10:40 [PATCH 0/3] Create arch_lwp_info class hierarchy Simon Marchi
2017-07-24 10:40 ` [PATCH 2/3] gdbserver lwp_info: Initialize fields, use new/delete Simon Marchi
2017-07-24 10:40 ` [PATCH 1/3] gdb lwp_info: Add destructor, initialize " Simon Marchi
@ 2017-07-24 10:40 ` Simon Marchi
2017-08-09 20:47 ` Simon Marchi
2017-08-11 20:13 ` [PATCH 0/3] " Pedro Alves
3 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2017-07-24 10:40 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@polymtl.ca>
I am trying to "poison" the XNEW family of functions as well as xfree to
help catch their uses on non-POD types. It trips on this:
/usr/include/c++/5/type_traits: In instantiation of ‘struct std::is_pod<arch_lwp_info>’:
/home/emaisin/src/binutils-gdb/gdb/common/traits.h:66:8: required from ‘struct gdb::Or<std::is_pod<arch_lwp_info>, std::is_void<arch_lwp_info> >’
/home/emaisin/src/binutils-gdb/gdb/common/common-utils.h:59:15: required from ‘void xfree(T*) [with T = arch_lwp_info]’
/home/emaisin/src/binutils-gdb/gdb/linux-nat.c:840:26: required from here
/usr/include/c++/5/type_traits:656:12: error: invalid use of incomplete type ‘struct arch_lwp_info’
struct is_pod
^
The problem is that the arch_lwp_info type is defined internally by the
various arch-specific linux-nat implementations in GDB and GDBserver,
but opaque to linux-nat.c. When linux-nat.c tries to xfree an object of
incomplete type arch_lwp_info, it can't validate whether it is POD or
not. This patch converts this to a proper class hierarchy, with
arch_lwp_info being an abstract class, and various arches defining their
own type extending it.
This will allow using C++ features in these objects, if we ever need to.
gdb/ChangeLog:
* nat/linux-nat.h (arch_lwp_info): Define type, add virtual pure
destructor.
* linux-nat.h (struct arch_lwp_info): Remove forward
declaration.
(lwp_info) <arch_private>: Change type to use unique_ptr.
* linux-nat.c (arch_lwp_info::~arch_lwp_info): Provide default
implementation.
(lwp_set_arch_private_info, lwp_arch_private_info): Adapt to use
unique_ptr.
(lwp_free): Don't free lp->arch_private manually.
* arm-linux-nat.c (struct arch_lwp_info): Rename to ...
(struct arm_lwp_info): ... this, inherit arch_lwp_info,
initialize fields.
(update_registers_callback): Allocate arm_lwp_info with new,
adjust to unique_ptr usage.
(arm_linux_new_thread): Likewise.
(arm_linux_prepare_to_resume): Adjust to unique_ptr usage.
* s390-linux-nat.c (struct arch_lwp_info): Rename to ...
(s390_lwp_info): ... this, inherit arch_lwp_info, initialize
fields.
(s390_prepare_to_resume): Add cast.
(s390_mark_per_info_changed): Allocate s390_lwp_info with new,
add cast.
* nat/aarch64-linux-hw-point.h (struct arch_lwp_info): Rename
to ...
(struct aarch64_lwp_info): ... this, inherit arch_lwp_info,
initialize fields.
* nat/aarch64-linux-hw-point.c (debug_reg_change_callback): Add
cast, allocate aarch64_lwp_info with new.
* nat/aarch64-linux.c (aarch64_linux_prepare_to_resume): Add
cast.
(aarch64_linux_new_thread): Allocate aarch64_lwp_info with new.
* x86-linux.c (struct arch_lwp_info): Rename to ...
(struct x86_lwp_info): ... this, inherit arch_lwp_info, initialize
fields.
(lwp_set_debug_registers_changed): Allocate x86_lwp_info with
new, add cast.
(lwp_debug_registers_changed): Add cast.
gdb/gdbserver/ChangeLog:
* linux-low.h (struct lwp_info) <arch_private> Change type to
unique_ptr.
* linux-arm-low.c (struct arch_lwp_info): Rename to ...
(struct arm_lwp_info): ... this, inherit arch_lwp_info, initialize
fields.
(update_registers_callback, arm_stopped_by_watchpoint,
arm_stopped_data_address): Use lwp_arch_private_info and add cast.
(arm_new_thread): Allocate arm_lwp_info with new, use
lwp_set_arch_private_info.
(arm_new_fork, arm_prepare_to_resume): Use
lwp_arch_private_info and add cast.
* linux-low.c (delete_lwp): Don't manually free arch_private.
(lwp_set_arch_private_info, lwp_arch_private_info): Adapt to use
unique_ptr.
(arch_lwp_info::~arch_lwp_info): Provide default implementation.
* linux-mips-low.c (struct arch_lwp_info): Rename to ...
(struct mips_lwp_info): ... this, inherit arch_lwp_info, initialize
fields.
(update_watch_registers_callback): Use lwp_arch_private_info and
add cast.
(mips_linux_new_thread): Allocate mips_lwp_info with new, use
lwp_arch_private_info.
(mips_linux_prepare_to_resume): Use lwp_arch_private_info and add cast.
---
gdb/arm-linux-nat.c | 21 ++++++++++++---------
gdb/gdbserver/linux-arm-low.c | 31 +++++++++++++++++--------------
gdb/gdbserver/linux-low.c | 7 ++++---
gdb/gdbserver/linux-low.h | 2 +-
gdb/gdbserver/linux-mips-low.c | 16 +++++++++-------
gdb/linux-nat.c | 7 ++++---
gdb/linux-nat.h | 4 +---
gdb/nat/aarch64-linux-hw-point.c | 4 ++--
gdb/nat/aarch64-linux-hw-point.h | 6 +++---
gdb/nat/aarch64-linux.c | 4 ++--
gdb/nat/linux-nat.h | 7 ++++++-
gdb/nat/x86-linux.c | 12 +++++++-----
gdb/s390-linux-nat.c | 11 ++++++-----
13 files changed, 74 insertions(+), 58 deletions(-)
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index ad3085a..19f9926 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -726,11 +726,11 @@ struct arm_linux_process_info
};
/* Per-thread arch-specific data we want to keep. */
-struct arch_lwp_info
+struct arm_lwp_info : public arch_lwp_info
{
/* Non-zero if our copy differs from what's recorded in the thread. */
- char bpts_changed[MAX_BPTS];
- char wpts_changed[MAX_WPTS];
+ char bpts_changed[MAX_BPTS] = {0};
+ char wpts_changed[MAX_WPTS] = {0};
};
static struct arm_linux_process_info *arm_linux_process_list = NULL;
@@ -928,14 +928,16 @@ update_registers_callback (struct lwp_info *lwp, void *arg)
struct update_registers_data *data = (struct update_registers_data *) arg;
if (lwp->arch_private == NULL)
- lwp->arch_private = XCNEW (struct arch_lwp_info);
+ lwp->arch_private.reset (new arm_lwp_info);
+
+ struct arm_lwp_info *info = (arm_lwp_info *) lwp->arch_private.get ();
/* The actual update is done later just before resuming the lwp,
we just mark that the registers need updating. */
if (data->watch)
- lwp->arch_private->wpts_changed[data->index] = 1;
+ info->wpts_changed[data->index] = 1;
else
- lwp->arch_private->bpts_changed[data->index] = 1;
+ info->bpts_changed[data->index] = 1;
/* If the lwp isn't stopped, force it to momentarily pause, so
we can update its breakpoint registers. */
@@ -1186,7 +1188,7 @@ static void
arm_linux_new_thread (struct lwp_info *lp)
{
int i;
- struct arch_lwp_info *info = XCNEW (struct arch_lwp_info);
+ struct arm_lwp_info *info = new arm_lwp_info;
/* Mark that all the hardware breakpoint/watchpoint register pairs
for this thread need to be initialized. */
@@ -1197,7 +1199,7 @@ arm_linux_new_thread (struct lwp_info *lp)
info->wpts_changed[i] = 1;
}
- lp->arch_private = info;
+ lp->arch_private.reset (info);
}
/* Called when resuming a thread.
@@ -1208,7 +1210,8 @@ arm_linux_prepare_to_resume (struct lwp_info *lwp)
{
int pid, i;
struct arm_linux_hw_breakpoint *bpts, *wpts;
- struct arch_lwp_info *arm_lwp_info = lwp->arch_private;
+ struct arm_lwp_info *arm_lwp_info
+ = (struct arm_lwp_info *) lwp->arch_private.get ();
pid = ptid_get_lwp (lwp->ptid);
bpts = arm_linux_get_debug_reg_state (ptid_get_pid (lwp->ptid))->bpts;
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 5a3f465..9e28388 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -110,13 +110,14 @@ struct arch_process_info
};
/* Per-thread arch-specific data we want to keep. */
-struct arch_lwp_info
+struct arm_lwp_info : public arch_lwp_info
{
/* Non-zero if our copy differs from what's recorded in the thread. */
- char bpts_changed[MAX_BPTS];
- char wpts_changed[MAX_WPTS];
+ char bpts_changed[MAX_BPTS] = {0};
+ char wpts_changed[MAX_WPTS] = {0};
+
/* Cached stopped data address. */
- CORE_ADDR stopped_data_address;
+ CORE_ADDR stopped_data_address = 0;
};
/* These are in <asm/elf.h> in current kernels. */
@@ -471,6 +472,7 @@ update_registers_callback (struct inferior_list_entry *entry, void *arg)
{
struct thread_info *thread = (struct thread_info *) entry;
struct lwp_info *lwp = get_thread_lwp (thread);
+ arm_lwp_info *arm_lwp = (arm_lwp_info *) lwp_arch_private_info (lwp);
struct update_registers_data *data = (struct update_registers_data *) arg;
/* Only update the threads of the current process. */
@@ -479,9 +481,9 @@ update_registers_callback (struct inferior_list_entry *entry, void *arg)
/* The actual update is done later just before resuming the lwp,
we just mark that the registers need updating. */
if (data->watch)
- lwp->arch_private->wpts_changed[data->i] = 1;
+ arm_lwp->wpts_changed[data->i] = 1;
else
- lwp->arch_private->bpts_changed[data->i] = 1;
+ arm_lwp->bpts_changed[data->i] = 1;
/* If the lwp isn't stopped, force it to momentarily pause, so
we can update its breakpoint registers. */
@@ -594,6 +596,7 @@ static int
arm_stopped_by_watchpoint (void)
{
struct lwp_info *lwp = get_thread_lwp (current_thread);
+ arm_lwp_info *arm_lwp = (arm_lwp_info *) lwp_arch_private_info (lwp);
siginfo_t siginfo;
/* We must be able to set hardware watchpoints. */
@@ -617,8 +620,7 @@ arm_stopped_by_watchpoint (void)
return 0;
/* Cache stopped data address for use by arm_stopped_data_address. */
- lwp->arch_private->stopped_data_address
- = (CORE_ADDR) (uintptr_t) siginfo.si_addr;
+ arm_lwp->stopped_data_address = (CORE_ADDR) (uintptr_t) siginfo.si_addr;
return 1;
}
@@ -629,7 +631,8 @@ static CORE_ADDR
arm_stopped_data_address (void)
{
struct lwp_info *lwp = get_thread_lwp (current_thread);
- return lwp->arch_private->stopped_data_address;
+ arm_lwp_info *arm_lwp = (arm_lwp_info *) lwp_arch_private_info (lwp);
+ return arm_lwp->stopped_data_address;
}
/* Called when a new process is created. */
@@ -644,7 +647,7 @@ arm_new_process (void)
static void
arm_new_thread (struct lwp_info *lwp)
{
- struct arch_lwp_info *info = XCNEW (struct arch_lwp_info);
+ arm_lwp_info *info = new arm_lwp_info;
int i;
for (i = 0; i < MAX_BPTS; i++)
@@ -652,7 +655,7 @@ arm_new_thread (struct lwp_info *lwp)
for (i = 0; i < MAX_WPTS; i++)
info->wpts_changed[i] = 1;
- lwp->arch_private = info;
+ lwp_set_arch_private_info (lwp, info);
}
static void
@@ -661,7 +664,6 @@ arm_new_fork (struct process_info *parent, struct process_info *child)
struct arch_process_info *parent_proc_info;
struct arch_process_info *child_proc_info;
struct lwp_info *child_lwp;
- struct arch_lwp_info *child_lwp_info;
int i;
/* These are allocated by linux_add_process. */
@@ -692,7 +694,8 @@ arm_new_fork (struct process_info *parent, struct process_info *child)
/* Mark all the hardware breakpoints and watchpoints as changed to
make sure that the registers will be updated. */
child_lwp = find_lwp_pid (ptid_of (child));
- child_lwp_info = child_lwp->arch_private;
+ arm_lwp_info *child_lwp_info
+ = (arm_lwp_info *) lwp_arch_private_info (child_lwp);
for (i = 0; i < MAX_BPTS; i++)
child_lwp_info->bpts_changed[i] = 1;
for (i = 0; i < MAX_WPTS; i++)
@@ -708,7 +711,7 @@ arm_prepare_to_resume (struct lwp_info *lwp)
int pid = lwpid_of (thread);
struct process_info *proc = find_process_pid (pid_of (thread));
struct arch_process_info *proc_info = proc->priv->arch_private;
- struct arch_lwp_info *lwp_info = lwp->arch_private;
+ arm_lwp_info *lwp_info = (arm_lwp_info *) lwp_arch_private_info (lwp);
int i;
for (i = 0; i < arm_linux_get_hw_breakpoint_count (); i++)
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e650b0d..5decca7 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -152,7 +152,7 @@ void
lwp_set_arch_private_info (struct lwp_info *lwp,
struct arch_lwp_info *info)
{
- lwp->arch_private = info;
+ lwp->arch_private.reset (info);
}
/* See nat/linux-nat.h. */
@@ -160,7 +160,7 @@ lwp_set_arch_private_info (struct lwp_info *lwp,
struct arch_lwp_info *
lwp_arch_private_info (struct lwp_info *lwp)
{
- return lwp->arch_private;
+ return lwp->arch_private.get () ;
}
/* See nat/linux-nat.h. */
@@ -414,7 +414,6 @@ delete_lwp (struct lwp_info *lwp)
debug_printf ("deleting %ld\n", lwpid_of (thr));
remove_thread (thr);
- free (lwp->arch_private);
delete lwp;
}
@@ -7588,6 +7587,8 @@ linux_get_pc_64bit (struct regcache *regcache)
return pc;
}
+arch_lwp_info::~arch_lwp_info () = default;
+
static struct target_ops linux_target_ops = {
linux_create_inferior,
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index dcc9315..43c1735 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -377,7 +377,7 @@ struct lwp_info
#endif
/* Arch-specific additions. */
- struct arch_lwp_info *arch_private = NULL;
+ std::unique_ptr<arch_lwp_info> arch_private;
};
int linux_pid_exe_is_elf_64_file (int pid, unsigned int *machine);
diff --git a/gdb/gdbserver/linux-mips-low.c b/gdb/gdbserver/linux-mips-low.c
index b4a83b0..b09a099 100644
--- a/gdb/gdbserver/linux-mips-low.c
+++ b/gdb/gdbserver/linux-mips-low.c
@@ -188,10 +188,10 @@ struct arch_process_info
/* Per-thread arch-specific data we want to keep. */
-struct arch_lwp_info
+struct mips_lwp_info : public arch_lwp_info
{
/* Non-zero if our copy differs from what's recorded in the thread. */
- int watch_registers_changed;
+ int watch_registers_changed = 0;
};
/* From mips-linux-nat.c. */
@@ -298,6 +298,7 @@ update_watch_registers_callback (struct inferior_list_entry *entry,
{
struct thread_info *thread = (struct thread_info *) entry;
struct lwp_info *lwp = get_thread_lwp (thread);
+ mips_lwp_info *mips_lwp = (mips_lwp_info *) lwp_arch_private_info (lwp);
int pid = *(int *) pid_p;
/* Only update the threads of this process. */
@@ -305,7 +306,7 @@ update_watch_registers_callback (struct inferior_list_entry *entry,
{
/* The actual update is done later just before resuming the lwp,
we just mark that the registers need updating. */
- lwp->arch_private->watch_registers_changed = 1;
+ mips_lwp->watch_registers_changed = 1;
/* If the lwp isn't stopped, force it to momentarily pause, so
we can update its watch registers. */
@@ -334,11 +335,11 @@ mips_linux_new_process (void)
static void
mips_linux_new_thread (struct lwp_info *lwp)
{
- struct arch_lwp_info *info = XCNEW (struct arch_lwp_info);
+ mips_lwp_info *info = new mips_lwp_info;
info->watch_registers_changed = 1;
- lwp->arch_private = info;
+ lwp_set_arch_private_info (lwp, info);
}
/* Create a new mips_watchpoint and add it to the list. */
@@ -413,8 +414,9 @@ mips_linux_prepare_to_resume (struct lwp_info *lwp)
ptid_t ptid = ptid_of (get_lwp_thread (lwp));
struct process_info *proc = find_process_pid (ptid_get_pid (ptid));
struct arch_process_info *priv = proc->priv->arch_private;
+ mips_lwp_info *mips_lwp = (mips_lwp_info *) lwp_arch_private_info (lwp);
- if (lwp->arch_private->watch_registers_changed)
+ if (mips_lwp->watch_registers_changed)
{
/* Only update the watch registers if we have set or unset a
watchpoint already. */
@@ -428,7 +430,7 @@ mips_linux_prepare_to_resume (struct lwp_info *lwp)
perror_with_name ("Couldn't write watch register");
}
- lwp->arch_private->watch_registers_changed = 0;
+ mips_lwp->watch_registers_changed = 0;
}
}
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 9378276..130a4d4 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -322,13 +322,15 @@ ptid_of_lwp (struct lwp_info *lwp)
return lwp->ptid;
}
+arch_lwp_info::~arch_lwp_info () = default;
+
/* See nat/linux-nat.h. */
void
lwp_set_arch_private_info (struct lwp_info *lwp,
struct arch_lwp_info *info)
{
- lwp->arch_private = info;
+ lwp->arch_private.reset (info);
}
/* See nat/linux-nat.h. */
@@ -336,7 +338,7 @@ lwp_set_arch_private_info (struct lwp_info *lwp,
struct arch_lwp_info *
lwp_arch_private_info (struct lwp_info *lwp)
{
- return lwp->arch_private;
+ return lwp->arch_private.get ();
}
/* See nat/linux-nat.h. */
@@ -837,7 +839,6 @@ static int check_ptrace_stopped_lwp_gone (struct lwp_info *lp);
static void
lwp_free (struct lwp_info *lp)
{
- xfree (lp->arch_private);
delete lp;
}
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index a783674..57fab42 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -21,8 +21,6 @@
#include "target.h"
#include <signal.h>
-struct arch_lwp_info;
-
/* Structure describing an LWP. This is public only for the purposes
of ALL_LWPS; target-specific code should generally not access it
directly. */
@@ -103,7 +101,7 @@ struct lwp_info
int core = -1;
/* Arch-specific additions. */
- arch_lwp_info *arch_private = NULL;
+ std::unique_ptr<arch_lwp_info> arch_private;
/* Previous and next pointers in doubly-linked list of known LWPs,
sorted by reverse creation order. */
diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
index 9800d9a..bba2200 100644
--- a/gdb/nat/aarch64-linux-hw-point.c
+++ b/gdb/nat/aarch64-linux-hw-point.c
@@ -258,13 +258,13 @@ debug_reg_change_callback (struct lwp_info *lwp, void *ptr)
int tid = ptid_get_lwp (ptid_of_lwp (lwp));
int idx = param_p->idx;
int is_watchpoint = param_p->is_watchpoint;
- struct arch_lwp_info *info = lwp_arch_private_info (lwp);
+ aarch64_lwp_info *info = (aarch64_lwp_info *) lwp_arch_private_info (lwp);
dr_changed_t *dr_changed_ptr;
dr_changed_t dr_changed;
if (info == NULL)
{
- info = XCNEW (struct arch_lwp_info);
+ info = new aarch64_lwp_info;
lwp_set_arch_private_info (lwp, info);
}
diff --git a/gdb/nat/aarch64-linux-hw-point.h b/gdb/nat/aarch64-linux-hw-point.h
index 610a5f1..6547594 100644
--- a/gdb/nat/aarch64-linux-hw-point.h
+++ b/gdb/nat/aarch64-linux-hw-point.h
@@ -154,13 +154,13 @@ struct aarch64_debug_reg_state
/* Per-thread arch-specific data we want to keep. */
-struct arch_lwp_info
+struct aarch64_lwp_info : public arch_lwp_info
{
/* When bit N is 1, it indicates the Nth hardware breakpoint or
watchpoint register pair needs to be updated when the thread is
resumed; see aarch64_linux_prepare_to_resume. */
- dr_changed_t dr_changed_bp;
- dr_changed_t dr_changed_wp;
+ dr_changed_t dr_changed_bp = 0;
+ dr_changed_t dr_changed_wp = 0;
};
extern int aarch64_num_bp_regs;
diff --git a/gdb/nat/aarch64-linux.c b/gdb/nat/aarch64-linux.c
index 388eee8..12999da 100644
--- a/gdb/nat/aarch64-linux.c
+++ b/gdb/nat/aarch64-linux.c
@@ -33,7 +33,7 @@
void
aarch64_linux_prepare_to_resume (struct lwp_info *lwp)
{
- struct arch_lwp_info *info = lwp_arch_private_info (lwp);
+ aarch64_lwp_info *info = (aarch64_lwp_info *) lwp_arch_private_info (lwp);
/* NULL means this is the main thread still going through the shell,
or, no watchpoint has been set yet. In that case, there's
@@ -73,7 +73,7 @@ aarch64_linux_prepare_to_resume (struct lwp_info *lwp)
void
aarch64_linux_new_thread (struct lwp_info *lwp)
{
- struct arch_lwp_info *info = XNEW (struct arch_lwp_info);
+ aarch64_lwp_info *info = new aarch64_lwp_info;
/* Mark that all the hardware breakpoint/watchpoint register pairs
for this thread need to be initialized (with data from
diff --git a/gdb/nat/linux-nat.h b/gdb/nat/linux-nat.h
index 7dd18fe..751f32c 100644
--- a/gdb/nat/linux-nat.h
+++ b/gdb/nat/linux-nat.h
@@ -23,7 +23,12 @@
#include "target/waitstatus.h"
struct lwp_info;
-struct arch_lwp_info;
+
+/* Base class for arch-specific lwp data. */
+struct arch_lwp_info
+{
+ virtual ~arch_lwp_info () = 0;
+};
/* This is the kernel's hard limit. Not to be confused with SIGRTMIN. */
#ifndef __SIGRTMIN
diff --git a/gdb/nat/x86-linux.c b/gdb/nat/x86-linux.c
index b499e74..b35ed19 100644
--- a/gdb/nat/x86-linux.c
+++ b/gdb/nat/x86-linux.c
@@ -23,11 +23,11 @@
/* Per-thread arch-specific data we want to keep. */
-struct arch_lwp_info
+struct x86_lwp_info : public arch_lwp_info
{
/* Non-zero if our copy differs from what's recorded in the
thread. */
- int debug_registers_changed;
+ int debug_registers_changed = 0;
};
/* See nat/x86-linux.h. */
@@ -36,9 +36,11 @@ void
lwp_set_debug_registers_changed (struct lwp_info *lwp, int value)
{
if (lwp_arch_private_info (lwp) == NULL)
- lwp_set_arch_private_info (lwp, XCNEW (struct arch_lwp_info));
+ lwp_set_arch_private_info (lwp, new x86_lwp_info ());
- lwp_arch_private_info (lwp)->debug_registers_changed = value;
+ x86_lwp_info *info = (x86_lwp_info *) lwp_arch_private_info (lwp);
+
+ info->debug_registers_changed = value;
}
/* See nat/x86-linux.h. */
@@ -46,7 +48,7 @@ lwp_set_debug_registers_changed (struct lwp_info *lwp, int value)
int
lwp_debug_registers_changed (struct lwp_info *lwp)
{
- struct arch_lwp_info *info = lwp_arch_private_info (lwp);
+ x86_lwp_info *info = (x86_lwp_info *) lwp_arch_private_info (lwp);
/* NULL means either that this is the main thread still going
through the shell, or that no watchpoint has been set yet.
diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 90c73c4..4e38f16 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -44,10 +44,10 @@
/* Per-thread arch-specific data. */
-struct arch_lwp_info
+struct s390_lwp_info : public arch_lwp_info
{
/* Non-zero if the thread's PER info must be re-written. */
- int per_info_changed;
+ int per_info_changed = 0;
};
static int have_regset_last_break = 0;
@@ -665,7 +665,7 @@ s390_prepare_to_resume (struct lwp_info *lp)
CORE_ADDR watch_lo_addr = (CORE_ADDR)-1, watch_hi_addr = 0;
unsigned ix;
s390_watch_area *area;
- struct arch_lwp_info *lp_priv = lwp_arch_private_info (lp);
+ s390_lwp_info *lp_priv = (s390_lwp_info *) lwp_arch_private_info (lp);
struct s390_debug_reg_state *state = s390_get_debug_reg_state (pid);
int step = lwp_is_stepping (lp);
@@ -763,9 +763,10 @@ static void
s390_mark_per_info_changed (struct lwp_info *lp)
{
if (lwp_arch_private_info (lp) == NULL)
- lwp_set_arch_private_info (lp, XCNEW (struct arch_lwp_info));
+ lwp_set_arch_private_info (lp, new s390_lwp_info);
- lwp_arch_private_info (lp)->per_info_changed = 1;
+ s390_lwp_info *lp_private = (s390_lwp_info *) lwp_arch_private_info (lp);
+ lp_private->per_info_changed = 1;
}
/* When attaching to a new thread, mark its PER info as changed. */
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] Create arch_lwp_info class hierarchy
2017-07-24 10:40 ` [PATCH 3/3] Create arch_lwp_info class hierarchy Simon Marchi
@ 2017-08-09 20:47 ` Simon Marchi
0 siblings, 0 replies; 15+ messages in thread
From: Simon Marchi @ 2017-08-09 20:47 UTC (permalink / raw)
To: gdb-patches; +Cc: Yao Qi
On 2017-07-24 12:39 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> I am trying to "poison" the XNEW family of functions as well as xfree to
> help catch their uses on non-POD types. It trips on this:
>
> /usr/include/c++/5/type_traits: In instantiation of ‘struct std::is_pod<arch_lwp_info>’:
> /home/emaisin/src/binutils-gdb/gdb/common/traits.h:66:8: required from ‘struct gdb::Or<std::is_pod<arch_lwp_info>, std::is_void<arch_lwp_info> >’
> /home/emaisin/src/binutils-gdb/gdb/common/common-utils.h:59:15: required from ‘void xfree(T*) [with T = arch_lwp_info]’
> /home/emaisin/src/binutils-gdb/gdb/linux-nat.c:840:26: required from here
> /usr/include/c++/5/type_traits:656:12: error: invalid use of incomplete type ‘struct arch_lwp_info’
> struct is_pod
> ^
>
> The problem is that the arch_lwp_info type is defined internally by the
> various arch-specific linux-nat implementations in GDB and GDBserver,
> but opaque to linux-nat.c. When linux-nat.c tries to xfree an object of
> incomplete type arch_lwp_info, it can't validate whether it is POD or
> not. This patch converts this to a proper class hierarchy, with
> arch_lwp_info being an abstract class, and various arches defining their
> own type extending it.
>
> This will allow using C++ features in these objects, if we ever need to.
>
> gdb/ChangeLog:
>
> * nat/linux-nat.h (arch_lwp_info): Define type, add virtual pure
> destructor.
> * linux-nat.h (struct arch_lwp_info): Remove forward
> declaration.
> (lwp_info) <arch_private>: Change type to use unique_ptr.
> * linux-nat.c (arch_lwp_info::~arch_lwp_info): Provide default
> implementation.
> (lwp_set_arch_private_info, lwp_arch_private_info): Adapt to use
> unique_ptr.
> (lwp_free): Don't free lp->arch_private manually.
> * arm-linux-nat.c (struct arch_lwp_info): Rename to ...
> (struct arm_lwp_info): ... this, inherit arch_lwp_info,
> initialize fields.
> (update_registers_callback): Allocate arm_lwp_info with new,
> adjust to unique_ptr usage.
> (arm_linux_new_thread): Likewise.
> (arm_linux_prepare_to_resume): Adjust to unique_ptr usage.
> * s390-linux-nat.c (struct arch_lwp_info): Rename to ...
> (s390_lwp_info): ... this, inherit arch_lwp_info, initialize
> fields.
> (s390_prepare_to_resume): Add cast.
> (s390_mark_per_info_changed): Allocate s390_lwp_info with new,
> add cast.
> * nat/aarch64-linux-hw-point.h (struct arch_lwp_info): Rename
> to ...
> (struct aarch64_lwp_info): ... this, inherit arch_lwp_info,
> initialize fields.
> * nat/aarch64-linux-hw-point.c (debug_reg_change_callback): Add
> cast, allocate aarch64_lwp_info with new.
> * nat/aarch64-linux.c (aarch64_linux_prepare_to_resume): Add
> cast.
> (aarch64_linux_new_thread): Allocate aarch64_lwp_info with new.
> * x86-linux.c (struct arch_lwp_info): Rename to ...
> (struct x86_lwp_info): ... this, inherit arch_lwp_info, initialize
> fields.
> (lwp_set_debug_registers_changed): Allocate x86_lwp_info with
> new, add cast.
> (lwp_debug_registers_changed): Add cast.
>
> gdb/gdbserver/ChangeLog:
>
> * linux-low.h (struct lwp_info) <arch_private> Change type to
> unique_ptr.
> * linux-arm-low.c (struct arch_lwp_info): Rename to ...
> (struct arm_lwp_info): ... this, inherit arch_lwp_info, initialize
> fields.
> (update_registers_callback, arm_stopped_by_watchpoint,
> arm_stopped_data_address): Use lwp_arch_private_info and add cast.
> (arm_new_thread): Allocate arm_lwp_info with new, use
> lwp_set_arch_private_info.
> (arm_new_fork, arm_prepare_to_resume): Use
> lwp_arch_private_info and add cast.
> * linux-low.c (delete_lwp): Don't manually free arch_private.
> (lwp_set_arch_private_info, lwp_arch_private_info): Adapt to use
> unique_ptr.
> (arch_lwp_info::~arch_lwp_info): Provide default implementation.
> * linux-mips-low.c (struct arch_lwp_info): Rename to ...
> (struct mips_lwp_info): ... this, inherit arch_lwp_info, initialize
> fields.
> (update_watch_registers_callback): Use lwp_arch_private_info and
> add cast.
> (mips_linux_new_thread): Allocate mips_lwp_info with new, use
> lwp_arch_private_info.
> (mips_linux_prepare_to_resume): Use lwp_arch_private_info and add cast.
> ---
> gdb/arm-linux-nat.c | 21 ++++++++++++---------
> gdb/gdbserver/linux-arm-low.c | 31 +++++++++++++++++--------------
> gdb/gdbserver/linux-low.c | 7 ++++---
> gdb/gdbserver/linux-low.h | 2 +-
> gdb/gdbserver/linux-mips-low.c | 16 +++++++++-------
> gdb/linux-nat.c | 7 ++++---
> gdb/linux-nat.h | 4 +---
> gdb/nat/aarch64-linux-hw-point.c | 4 ++--
> gdb/nat/aarch64-linux-hw-point.h | 6 +++---
> gdb/nat/aarch64-linux.c | 4 ++--
> gdb/nat/linux-nat.h | 7 ++++++-
> gdb/nat/x86-linux.c | 12 +++++++-----
> gdb/s390-linux-nat.c | 11 ++++++-----
> 13 files changed, 74 insertions(+), 58 deletions(-)
>
> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
> index ad3085a..19f9926 100644
> --- a/gdb/arm-linux-nat.c
> +++ b/gdb/arm-linux-nat.c
> @@ -726,11 +726,11 @@ struct arm_linux_process_info
> };
>
> /* Per-thread arch-specific data we want to keep. */
> -struct arch_lwp_info
> +struct arm_lwp_info : public arch_lwp_info
> {
> /* Non-zero if our copy differs from what's recorded in the thread. */
> - char bpts_changed[MAX_BPTS];
> - char wpts_changed[MAX_WPTS];
> + char bpts_changed[MAX_BPTS] = {0};
> + char wpts_changed[MAX_WPTS] = {0};
> };
>
> static struct arm_linux_process_info *arm_linux_process_list = NULL;
> @@ -928,14 +928,16 @@ update_registers_callback (struct lwp_info *lwp, void *arg)
> struct update_registers_data *data = (struct update_registers_data *) arg;
>
> if (lwp->arch_private == NULL)
> - lwp->arch_private = XCNEW (struct arch_lwp_info);
> + lwp->arch_private.reset (new arm_lwp_info);
> +
> + struct arm_lwp_info *info = (arm_lwp_info *) lwp->arch_private.get ();
>
> /* The actual update is done later just before resuming the lwp,
> we just mark that the registers need updating. */
> if (data->watch)
> - lwp->arch_private->wpts_changed[data->index] = 1;
> + info->wpts_changed[data->index] = 1;
> else
> - lwp->arch_private->bpts_changed[data->index] = 1;
> + info->bpts_changed[data->index] = 1;
>
> /* If the lwp isn't stopped, force it to momentarily pause, so
> we can update its breakpoint registers. */
> @@ -1186,7 +1188,7 @@ static void
> arm_linux_new_thread (struct lwp_info *lp)
> {
> int i;
> - struct arch_lwp_info *info = XCNEW (struct arch_lwp_info);
> + struct arm_lwp_info *info = new arm_lwp_info;
>
> /* Mark that all the hardware breakpoint/watchpoint register pairs
> for this thread need to be initialized. */
> @@ -1197,7 +1199,7 @@ arm_linux_new_thread (struct lwp_info *lp)
> info->wpts_changed[i] = 1;
> }
>
> - lp->arch_private = info;
> + lp->arch_private.reset (info);
> }
>
> /* Called when resuming a thread.
> @@ -1208,7 +1210,8 @@ arm_linux_prepare_to_resume (struct lwp_info *lwp)
> {
> int pid, i;
> struct arm_linux_hw_breakpoint *bpts, *wpts;
> - struct arch_lwp_info *arm_lwp_info = lwp->arch_private;
> + struct arm_lwp_info *arm_lwp_info
> + = (struct arm_lwp_info *) lwp->arch_private.get ();
>
> pid = ptid_get_lwp (lwp->ptid);
> bpts = arm_linux_get_debug_reg_state (ptid_get_pid (lwp->ptid))->bpts;
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index 5a3f465..9e28388 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -110,13 +110,14 @@ struct arch_process_info
> };
>
> /* Per-thread arch-specific data we want to keep. */
> -struct arch_lwp_info
> +struct arm_lwp_info : public arch_lwp_info
> {
> /* Non-zero if our copy differs from what's recorded in the thread. */
> - char bpts_changed[MAX_BPTS];
> - char wpts_changed[MAX_WPTS];
> + char bpts_changed[MAX_BPTS] = {0};
> + char wpts_changed[MAX_WPTS] = {0};
> +
> /* Cached stopped data address. */
> - CORE_ADDR stopped_data_address;
> + CORE_ADDR stopped_data_address = 0;
> };
>
> /* These are in <asm/elf.h> in current kernels. */
> @@ -471,6 +472,7 @@ update_registers_callback (struct inferior_list_entry *entry, void *arg)
> {
> struct thread_info *thread = (struct thread_info *) entry;
> struct lwp_info *lwp = get_thread_lwp (thread);
> + arm_lwp_info *arm_lwp = (arm_lwp_info *) lwp_arch_private_info (lwp);
> struct update_registers_data *data = (struct update_registers_data *) arg;
>
> /* Only update the threads of the current process. */
> @@ -479,9 +481,9 @@ update_registers_callback (struct inferior_list_entry *entry, void *arg)
> /* The actual update is done later just before resuming the lwp,
> we just mark that the registers need updating. */
> if (data->watch)
> - lwp->arch_private->wpts_changed[data->i] = 1;
> + arm_lwp->wpts_changed[data->i] = 1;
> else
> - lwp->arch_private->bpts_changed[data->i] = 1;
> + arm_lwp->bpts_changed[data->i] = 1;
>
> /* If the lwp isn't stopped, force it to momentarily pause, so
> we can update its breakpoint registers. */
> @@ -594,6 +596,7 @@ static int
> arm_stopped_by_watchpoint (void)
> {
> struct lwp_info *lwp = get_thread_lwp (current_thread);
> + arm_lwp_info *arm_lwp = (arm_lwp_info *) lwp_arch_private_info (lwp);
> siginfo_t siginfo;
>
> /* We must be able to set hardware watchpoints. */
> @@ -617,8 +620,7 @@ arm_stopped_by_watchpoint (void)
> return 0;
>
> /* Cache stopped data address for use by arm_stopped_data_address. */
> - lwp->arch_private->stopped_data_address
> - = (CORE_ADDR) (uintptr_t) siginfo.si_addr;
> + arm_lwp->stopped_data_address = (CORE_ADDR) (uintptr_t) siginfo.si_addr;
>
> return 1;
> }
> @@ -629,7 +631,8 @@ static CORE_ADDR
> arm_stopped_data_address (void)
> {
> struct lwp_info *lwp = get_thread_lwp (current_thread);
> - return lwp->arch_private->stopped_data_address;
> + arm_lwp_info *arm_lwp = (arm_lwp_info *) lwp_arch_private_info (lwp);
> + return arm_lwp->stopped_data_address;
> }
>
> /* Called when a new process is created. */
> @@ -644,7 +647,7 @@ arm_new_process (void)
> static void
> arm_new_thread (struct lwp_info *lwp)
> {
> - struct arch_lwp_info *info = XCNEW (struct arch_lwp_info);
> + arm_lwp_info *info = new arm_lwp_info;
> int i;
>
> for (i = 0; i < MAX_BPTS; i++)
> @@ -652,7 +655,7 @@ arm_new_thread (struct lwp_info *lwp)
> for (i = 0; i < MAX_WPTS; i++)
> info->wpts_changed[i] = 1;
>
> - lwp->arch_private = info;
> + lwp_set_arch_private_info (lwp, info);
> }
>
> static void
> @@ -661,7 +664,6 @@ arm_new_fork (struct process_info *parent, struct process_info *child)
> struct arch_process_info *parent_proc_info;
> struct arch_process_info *child_proc_info;
> struct lwp_info *child_lwp;
> - struct arch_lwp_info *child_lwp_info;
> int i;
>
> /* These are allocated by linux_add_process. */
> @@ -692,7 +694,8 @@ arm_new_fork (struct process_info *parent, struct process_info *child)
> /* Mark all the hardware breakpoints and watchpoints as changed to
> make sure that the registers will be updated. */
> child_lwp = find_lwp_pid (ptid_of (child));
> - child_lwp_info = child_lwp->arch_private;
> + arm_lwp_info *child_lwp_info
> + = (arm_lwp_info *) lwp_arch_private_info (child_lwp);
> for (i = 0; i < MAX_BPTS; i++)
> child_lwp_info->bpts_changed[i] = 1;
> for (i = 0; i < MAX_WPTS; i++)
> @@ -708,7 +711,7 @@ arm_prepare_to_resume (struct lwp_info *lwp)
> int pid = lwpid_of (thread);
> struct process_info *proc = find_process_pid (pid_of (thread));
> struct arch_process_info *proc_info = proc->priv->arch_private;
> - struct arch_lwp_info *lwp_info = lwp->arch_private;
> + arm_lwp_info *lwp_info = (arm_lwp_info *) lwp_arch_private_info (lwp);
> int i;
>
> for (i = 0; i < arm_linux_get_hw_breakpoint_count (); i++)
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index e650b0d..5decca7 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -152,7 +152,7 @@ void
> lwp_set_arch_private_info (struct lwp_info *lwp,
> struct arch_lwp_info *info)
> {
> - lwp->arch_private = info;
> + lwp->arch_private.reset (info);
> }
>
> /* See nat/linux-nat.h. */
> @@ -160,7 +160,7 @@ lwp_set_arch_private_info (struct lwp_info *lwp,
> struct arch_lwp_info *
> lwp_arch_private_info (struct lwp_info *lwp)
> {
> - return lwp->arch_private;
> + return lwp->arch_private.get () ;
> }
>
> /* See nat/linux-nat.h. */
> @@ -414,7 +414,6 @@ delete_lwp (struct lwp_info *lwp)
> debug_printf ("deleting %ld\n", lwpid_of (thr));
>
> remove_thread (thr);
> - free (lwp->arch_private);
> delete lwp;
> }
>
> @@ -7588,6 +7587,8 @@ linux_get_pc_64bit (struct regcache *regcache)
> return pc;
> }
>
> +arch_lwp_info::~arch_lwp_info () = default;
> +
>
> static struct target_ops linux_target_ops = {
> linux_create_inferior,
> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
> index dcc9315..43c1735 100644
> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -377,7 +377,7 @@ struct lwp_info
> #endif
>
> /* Arch-specific additions. */
> - struct arch_lwp_info *arch_private = NULL;
> + std::unique_ptr<arch_lwp_info> arch_private;
> };
>
> int linux_pid_exe_is_elf_64_file (int pid, unsigned int *machine);
> diff --git a/gdb/gdbserver/linux-mips-low.c b/gdb/gdbserver/linux-mips-low.c
> index b4a83b0..b09a099 100644
> --- a/gdb/gdbserver/linux-mips-low.c
> +++ b/gdb/gdbserver/linux-mips-low.c
> @@ -188,10 +188,10 @@ struct arch_process_info
>
> /* Per-thread arch-specific data we want to keep. */
>
> -struct arch_lwp_info
> +struct mips_lwp_info : public arch_lwp_info
> {
> /* Non-zero if our copy differs from what's recorded in the thread. */
> - int watch_registers_changed;
> + int watch_registers_changed = 0;
> };
>
> /* From mips-linux-nat.c. */
> @@ -298,6 +298,7 @@ update_watch_registers_callback (struct inferior_list_entry *entry,
> {
> struct thread_info *thread = (struct thread_info *) entry;
> struct lwp_info *lwp = get_thread_lwp (thread);
> + mips_lwp_info *mips_lwp = (mips_lwp_info *) lwp_arch_private_info (lwp);
> int pid = *(int *) pid_p;
>
> /* Only update the threads of this process. */
> @@ -305,7 +306,7 @@ update_watch_registers_callback (struct inferior_list_entry *entry,
> {
> /* The actual update is done later just before resuming the lwp,
> we just mark that the registers need updating. */
> - lwp->arch_private->watch_registers_changed = 1;
> + mips_lwp->watch_registers_changed = 1;
>
> /* If the lwp isn't stopped, force it to momentarily pause, so
> we can update its watch registers. */
> @@ -334,11 +335,11 @@ mips_linux_new_process (void)
> static void
> mips_linux_new_thread (struct lwp_info *lwp)
> {
> - struct arch_lwp_info *info = XCNEW (struct arch_lwp_info);
> + mips_lwp_info *info = new mips_lwp_info;
>
> info->watch_registers_changed = 1;
>
> - lwp->arch_private = info;
> + lwp_set_arch_private_info (lwp, info);
> }
>
> /* Create a new mips_watchpoint and add it to the list. */
> @@ -413,8 +414,9 @@ mips_linux_prepare_to_resume (struct lwp_info *lwp)
> ptid_t ptid = ptid_of (get_lwp_thread (lwp));
> struct process_info *proc = find_process_pid (ptid_get_pid (ptid));
> struct arch_process_info *priv = proc->priv->arch_private;
> + mips_lwp_info *mips_lwp = (mips_lwp_info *) lwp_arch_private_info (lwp);
>
> - if (lwp->arch_private->watch_registers_changed)
> + if (mips_lwp->watch_registers_changed)
> {
> /* Only update the watch registers if we have set or unset a
> watchpoint already. */
> @@ -428,7 +430,7 @@ mips_linux_prepare_to_resume (struct lwp_info *lwp)
> perror_with_name ("Couldn't write watch register");
> }
>
> - lwp->arch_private->watch_registers_changed = 0;
> + mips_lwp->watch_registers_changed = 0;
> }
> }
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 9378276..130a4d4 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -322,13 +322,15 @@ ptid_of_lwp (struct lwp_info *lwp)
> return lwp->ptid;
> }
>
> +arch_lwp_info::~arch_lwp_info () = default;
> +
> /* See nat/linux-nat.h. */
>
> void
> lwp_set_arch_private_info (struct lwp_info *lwp,
> struct arch_lwp_info *info)
> {
> - lwp->arch_private = info;
> + lwp->arch_private.reset (info);
> }
>
> /* See nat/linux-nat.h. */
> @@ -336,7 +338,7 @@ lwp_set_arch_private_info (struct lwp_info *lwp,
> struct arch_lwp_info *
> lwp_arch_private_info (struct lwp_info *lwp)
> {
> - return lwp->arch_private;
> + return lwp->arch_private.get ();
> }
>
> /* See nat/linux-nat.h. */
> @@ -837,7 +839,6 @@ static int check_ptrace_stopped_lwp_gone (struct lwp_info *lp);
> static void
> lwp_free (struct lwp_info *lp)
> {
> - xfree (lp->arch_private);
> delete lp;
> }
>
> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
> index a783674..57fab42 100644
> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
> @@ -21,8 +21,6 @@
> #include "target.h"
> #include <signal.h>
>
> -struct arch_lwp_info;
> -
> /* Structure describing an LWP. This is public only for the purposes
> of ALL_LWPS; target-specific code should generally not access it
> directly. */
> @@ -103,7 +101,7 @@ struct lwp_info
> int core = -1;
>
> /* Arch-specific additions. */
> - arch_lwp_info *arch_private = NULL;
> + std::unique_ptr<arch_lwp_info> arch_private;
>
> /* Previous and next pointers in doubly-linked list of known LWPs,
> sorted by reverse creation order. */
> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
> index 9800d9a..bba2200 100644
> --- a/gdb/nat/aarch64-linux-hw-point.c
> +++ b/gdb/nat/aarch64-linux-hw-point.c
> @@ -258,13 +258,13 @@ debug_reg_change_callback (struct lwp_info *lwp, void *ptr)
> int tid = ptid_get_lwp (ptid_of_lwp (lwp));
> int idx = param_p->idx;
> int is_watchpoint = param_p->is_watchpoint;
> - struct arch_lwp_info *info = lwp_arch_private_info (lwp);
> + aarch64_lwp_info *info = (aarch64_lwp_info *) lwp_arch_private_info (lwp);
> dr_changed_t *dr_changed_ptr;
> dr_changed_t dr_changed;
>
> if (info == NULL)
> {
> - info = XCNEW (struct arch_lwp_info);
> + info = new aarch64_lwp_info;
> lwp_set_arch_private_info (lwp, info);
> }
>
> diff --git a/gdb/nat/aarch64-linux-hw-point.h b/gdb/nat/aarch64-linux-hw-point.h
> index 610a5f1..6547594 100644
> --- a/gdb/nat/aarch64-linux-hw-point.h
> +++ b/gdb/nat/aarch64-linux-hw-point.h
> @@ -154,13 +154,13 @@ struct aarch64_debug_reg_state
>
> /* Per-thread arch-specific data we want to keep. */
>
> -struct arch_lwp_info
> +struct aarch64_lwp_info : public arch_lwp_info
> {
> /* When bit N is 1, it indicates the Nth hardware breakpoint or
> watchpoint register pair needs to be updated when the thread is
> resumed; see aarch64_linux_prepare_to_resume. */
> - dr_changed_t dr_changed_bp;
> - dr_changed_t dr_changed_wp;
> + dr_changed_t dr_changed_bp = 0;
> + dr_changed_t dr_changed_wp = 0;
> };
>
> extern int aarch64_num_bp_regs;
> diff --git a/gdb/nat/aarch64-linux.c b/gdb/nat/aarch64-linux.c
> index 388eee8..12999da 100644
> --- a/gdb/nat/aarch64-linux.c
> +++ b/gdb/nat/aarch64-linux.c
> @@ -33,7 +33,7 @@
> void
> aarch64_linux_prepare_to_resume (struct lwp_info *lwp)
> {
> - struct arch_lwp_info *info = lwp_arch_private_info (lwp);
> + aarch64_lwp_info *info = (aarch64_lwp_info *) lwp_arch_private_info (lwp);
>
> /* NULL means this is the main thread still going through the shell,
> or, no watchpoint has been set yet. In that case, there's
> @@ -73,7 +73,7 @@ aarch64_linux_prepare_to_resume (struct lwp_info *lwp)
> void
> aarch64_linux_new_thread (struct lwp_info *lwp)
> {
> - struct arch_lwp_info *info = XNEW (struct arch_lwp_info);
> + aarch64_lwp_info *info = new aarch64_lwp_info;
>
> /* Mark that all the hardware breakpoint/watchpoint register pairs
> for this thread need to be initialized (with data from
> diff --git a/gdb/nat/linux-nat.h b/gdb/nat/linux-nat.h
> index 7dd18fe..751f32c 100644
> --- a/gdb/nat/linux-nat.h
> +++ b/gdb/nat/linux-nat.h
> @@ -23,7 +23,12 @@
> #include "target/waitstatus.h"
>
> struct lwp_info;
> -struct arch_lwp_info;
> +
> +/* Base class for arch-specific lwp data. */
> +struct arch_lwp_info
> +{
> + virtual ~arch_lwp_info () = 0;
> +};
>
> /* This is the kernel's hard limit. Not to be confused with SIGRTMIN. */
> #ifndef __SIGRTMIN
> diff --git a/gdb/nat/x86-linux.c b/gdb/nat/x86-linux.c
> index b499e74..b35ed19 100644
> --- a/gdb/nat/x86-linux.c
> +++ b/gdb/nat/x86-linux.c
> @@ -23,11 +23,11 @@
>
> /* Per-thread arch-specific data we want to keep. */
>
> -struct arch_lwp_info
> +struct x86_lwp_info : public arch_lwp_info
> {
> /* Non-zero if our copy differs from what's recorded in the
> thread. */
> - int debug_registers_changed;
> + int debug_registers_changed = 0;
> };
>
> /* See nat/x86-linux.h. */
> @@ -36,9 +36,11 @@ void
> lwp_set_debug_registers_changed (struct lwp_info *lwp, int value)
> {
> if (lwp_arch_private_info (lwp) == NULL)
> - lwp_set_arch_private_info (lwp, XCNEW (struct arch_lwp_info));
> + lwp_set_arch_private_info (lwp, new x86_lwp_info ());
>
> - lwp_arch_private_info (lwp)->debug_registers_changed = value;
> + x86_lwp_info *info = (x86_lwp_info *) lwp_arch_private_info (lwp);
> +
> + info->debug_registers_changed = value;
> }
>
> /* See nat/x86-linux.h. */
> @@ -46,7 +48,7 @@ lwp_set_debug_registers_changed (struct lwp_info *lwp, int value)
> int
> lwp_debug_registers_changed (struct lwp_info *lwp)
> {
> - struct arch_lwp_info *info = lwp_arch_private_info (lwp);
> + x86_lwp_info *info = (x86_lwp_info *) lwp_arch_private_info (lwp);
>
> /* NULL means either that this is the main thread still going
> through the shell, or that no watchpoint has been set yet.
> diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
> index 90c73c4..4e38f16 100644
> --- a/gdb/s390-linux-nat.c
> +++ b/gdb/s390-linux-nat.c
> @@ -44,10 +44,10 @@
>
> /* Per-thread arch-specific data. */
>
> -struct arch_lwp_info
> +struct s390_lwp_info : public arch_lwp_info
> {
> /* Non-zero if the thread's PER info must be re-written. */
> - int per_info_changed;
> + int per_info_changed = 0;
> };
>
> static int have_regset_last_break = 0;
> @@ -665,7 +665,7 @@ s390_prepare_to_resume (struct lwp_info *lp)
> CORE_ADDR watch_lo_addr = (CORE_ADDR)-1, watch_hi_addr = 0;
> unsigned ix;
> s390_watch_area *area;
> - struct arch_lwp_info *lp_priv = lwp_arch_private_info (lp);
> + s390_lwp_info *lp_priv = (s390_lwp_info *) lwp_arch_private_info (lp);
> struct s390_debug_reg_state *state = s390_get_debug_reg_state (pid);
> int step = lwp_is_stepping (lp);
>
> @@ -763,9 +763,10 @@ static void
> s390_mark_per_info_changed (struct lwp_info *lp)
> {
> if (lwp_arch_private_info (lp) == NULL)
> - lwp_set_arch_private_info (lp, XCNEW (struct arch_lwp_info));
> + lwp_set_arch_private_info (lp, new s390_lwp_info);
>
> - lwp_arch_private_info (lp)->per_info_changed = 1;
> + s390_lwp_info *lp_private = (s390_lwp_info *) lwp_arch_private_info (lp);
> + lp_private->per_info_changed = 1;
> }
>
> /* When attaching to a new thread, mark its PER info as changed. */
>
Yao, did you have any comments on patch 3/3?
Simon
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Create arch_lwp_info class hierarchy
2017-07-24 10:40 [PATCH 0/3] Create arch_lwp_info class hierarchy Simon Marchi
` (2 preceding siblings ...)
2017-07-24 10:40 ` [PATCH 3/3] Create arch_lwp_info class hierarchy Simon Marchi
@ 2017-08-11 20:13 ` Pedro Alves
2017-08-12 11:31 ` Simon Marchi
3 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2017-08-11 20:13 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 07/24/2017 11:39 AM, Simon Marchi wrote:
> I have the goal of "poisoning" the XNEW/xfree-family of functions, so
> that we catch their usages with non-POD types. A few things need to be
> fixed in the mean time, this is one.
>
> The common lwp code in linux-nat.c and gdbserver/linux-low.c xfrees the
> private lwp data of type arch_lwp_info. However, that type is opaque
> from its point of view, as its defined differently in each arch-specific
> implementation. This trips on the std::is_pod<T> check, since the
> compiler can't tell whether the type is POD or not if it doesn't know
> about it.
>
> I think the right fix going forward is to make a hierachy out of these
> structs, so that they all inherit from a common base. That's what patch
> 3 does. Patches 1 and 2 simply C++ify the GDB and GDBserver lwp_info
> structures so that it's possible to use unique_ptr fields.
>
> Simon Marchi (3):
> gdb lwp_info: Add destructor, initialize fields, use new/delete
> gdbserver lwp_info: Initialize fields, use new/delete
> Create arch_lwp_info class hierarchy
Looks fine to me as well.
Let me just lay down some thoughts:
We don't really need to make these types have vtable pointers / don't
really need to polymorphic, since there's only ever going to be one
arch type in a build. So we could instead move the arch-specific
definitions to arch-specific headers, still name the arch-specific types
and then have linux-low.c etc. include it. That way, we'd use
arch_lwp_info throughout just like today. arch_lwp_info would just
be different types defined in different headers depending on arch.
I.e., e.g., in a linux-arm-low.h:
struct arch_lwp_info : public arch_lwp_info_base
{
// arm bits.
};
and then in linux-low.h we'd have
#ifdef __arm__
# include "linux-arm-low.h"
#elif defined __i686__
# include "linux-x86-low.h"
#elif ...
...
#endif
A follow up thing that we could do is have arch_lwp_info inherit
from lwp_info and always allocate arch_lwp_info objects.
Or for clarity, rename lwp_info to lwp_info_base and make the
arch version be The lwp_info type. I.e., e.g., in linux-arm-low.h:
struct lwp_info : public lwp_info_base
{
// arm bits.
};
This would avoid the double/separate allocation of
lwp_info + arch_lwp_info.
Anyway, just ideas. I'm really fine with what you have.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/3] Create arch_lwp_info class hierarchy
2017-08-11 20:13 ` [PATCH 0/3] " Pedro Alves
@ 2017-08-12 11:31 ` Simon Marchi
2017-08-14 11:53 ` Pedro Alves
0 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2017-08-12 11:31 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-08-11 22:13, Pedro Alves wrote:
> Let me just lay down some thoughts:
>
> We don't really need to make these types have vtable pointers / don't
> really need to polymorphic, since there's only ever going to be one
> arch type in a build. So we could instead move the arch-specific
> definitions to arch-specific headers, still name the arch-specific
> types
> and then have linux-low.c etc. include it. That way, we'd use
> arch_lwp_info throughout just like today. arch_lwp_info would just
> be different types defined in different headers depending on arch.
>
> I.e., e.g., in a linux-arm-low.h:
>
> struct arch_lwp_info : public arch_lwp_info_base
> {
> // arm bits.
> };
>
> and then in linux-low.h we'd have
>
> #ifdef __arm__
> # include "linux-arm-low.h"
> #elif defined __i686__
> # include "linux-x86-low.h"
> #elif ...
> ...
> #endif
>
> A follow up thing that we could do is have arch_lwp_info inherit
> from lwp_info and always allocate arch_lwp_info objects.
>
> Or for clarity, rename lwp_info to lwp_info_base and make the
> arch version be The lwp_info type. I.e., e.g., in linux-arm-low.h:
>
> struct lwp_info : public lwp_info_base
> {
> // arm bits.
> };
>
> This would avoid the double/separate allocation of
> lwp_info + arch_lwp_info.
I like the idea. I tried to do this and fell in some traps, maybe you
have some ideas about how to make it better. In linux-nat.c, for
example, I started with this:
#if defined __arm__
# include "arm-linux-nat.h"
#elif defined __i686__ || defined __x86_64__
# include "nat/x86-linux.h"
#else
# error "Missing arch-specific include."
#endif
But then I realized that many arches have linux support, but don't have
arch_lwp_info. So I changed to
...
#else
/* Define a dummy arch_lwp_info for arches that don't define one. */
struct arch_lwp_info [};
#endif
But then I realized that I forgot to include the header for s390, and
the compiler (when building for a s390 host) didn't warn me. This is
dangerous and fragile since we end up with two definitions of
arch_lwp_info (the s390 one and that fallback one), and nothing to warn
about it. So I changed it to listing explicitly the architectures that
don't defined their own arch_lwp_info:
...
#elif defined __alpha__ || defined __powerpc__ || ...
/* Define a dummy arch_lwp_info for arches that don't define one. */
struct arch_lwp_info {};
#else
# error "Missing arch-specific include."
#endif
That would work, but requires listing all the arches that need the
fallback definition of arch_lwp_info, so it gets pretty ugly.
Any idea to make this simple but safe? Otherwise, I'll just go with the
current version of the patch.
Simon
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/3] Create arch_lwp_info class hierarchy
2017-08-12 11:31 ` Simon Marchi
@ 2017-08-14 11:53 ` Pedro Alves
2017-08-16 18:44 ` Simon Marchi
0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2017-08-14 11:53 UTC (permalink / raw)
To: Simon Marchi; +Cc: Simon Marchi, gdb-patches
On 08/12/2017 12:29 PM, Simon Marchi wrote:
> But then I realized that I forgot to include the header for s390, and
> the compiler (when building for a s390 host) didn't warn me. This is
> dangerous and fragile since we end up with two definitions of
> arch_lwp_info (the s390 one and that fallback one), and nothing to warn
> about it.
Not sure whether that's really such a bad problem. It seems like the
sort of thing that'd crash quite visibly/quickly if you actually try to
run the resulting binary. It's not like we're adding ports
every day. :-)
> So I changed it to listing explicitly the architectures that
> don't defined their own arch_lwp_info:
>
> ...
> #elif defined __alpha__ || defined __powerpc__ || ...
> /* Define a dummy arch_lwp_info for arches that don't define one. */
> struct arch_lwp_info {};
> #else
> # error "Missing arch-specific include."
> #endif
>
> That would work, but requires listing all the arches that need the
> fallback definition of arch_lwp_info, so it gets pretty ugly.
>
> Any idea to make this simple but safe? Otherwise, I'll just go with the
> current version of the patch.
There's a 3rd, much simpler option.
The problem we ran into is one of blurred division of responsibility: the
arch-specific code is responsible for allocating the arch-specific
arch_lwp_info, which it must be, because it's not possible to allocate an
incomplete type (must know its size), but we free the object in common code,
where the type is opaque.
We can solve the original xfree poisoning problem by simply making the
arch-specific code responsible for releasing arch_lwp_info too, where the
type is known, just like it is responsible for allocating it.
See below a proof of concept doing that for the x86 port. Making other
archs do the same should be trivial. Using new/delete/cdtors/in-class
initialization for arch_lwp_info would be a separate, orthogonal change
(and so would class-ification of linux_nat_new_thread, linux_target_ops,
etc.).
From 40188bdb4ff7eae2876960808672c72529c934b5 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 14 Aug 2017 12:16:11 +0100
Subject: [PATCH] arch-specific code deletes arch-specific bits
---
gdb/gdbserver/linux-low.h | 4 ++++
gdb/gdbserver/linux-x86-low.c | 1 +
gdb/linux-nat.c | 18 +++++++++++++++++-
gdb/linux-nat.h | 3 +++
gdb/nat/x86-linux.c | 6 ++++++
gdb/nat/x86-linux.h | 4 ++++
gdb/x86-linux-nat.c | 1 +
7 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 4e7fbec..e7f76d1 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -194,6 +194,10 @@ struct linux_target_ops
allocate it here. */
void (*new_thread) (struct lwp_info *);
+ /* Hook to call when a thread is being deleted. If extra per-thread
+ architecture-specific data is needed, delete it here. */
+ void (*delete_thread) (struct lwp_info *);
+
/* Hook to call, if any, when a new fork is attached. */
void (*new_fork) (struct process_info *parent, struct process_info *child);
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 12c7a1b..338d203 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -2961,6 +2961,7 @@ struct linux_target_ops the_low_target =
x86_siginfo_fixup,
x86_linux_new_process,
x86_linux_new_thread,
+ x86_linux_delete_thread,
x86_linux_new_fork,
x86_linux_prepare_to_resume,
x86_linux_process_qsupported,
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 3dc3637..6be4f5b 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -197,6 +197,9 @@ static struct target_ops linux_ops_saved;
/* The method to call, if any, when a new thread is attached. */
static void (*linux_nat_new_thread) (struct lwp_info *);
+/* The method to call, if any, when a thread is destroyed. */
+static void (*linux_nat_delete_thread) (struct lwp_info *);
+
/* The method to call, if any, when a new fork is attached. */
static linux_nat_new_fork_ftype *linux_nat_new_fork;
@@ -837,7 +840,9 @@ static int check_ptrace_stopped_lwp_gone (struct lwp_info *lp);
static void
lwp_free (struct lwp_info *lp)
{
- xfree (lp->arch_private);
+ /* Let the arch specific bits release arch_lwp_info. */
+ if (linux_nat_delete_thread != NULL)
+ linux_nat_delete_thread (lp);
delete lp;
}
@@ -4863,6 +4868,17 @@ linux_nat_set_new_thread (struct target_ops *t,
linux_nat_new_thread = new_thread;
}
+/* Register a method to call whenever a new thread is attached. */
+void
+linux_nat_set_delete_thread (struct target_ops *t,
+ void (*delete_thread) (struct lwp_info *))
+{
+ /* Save the pointer. We only support a single registered instance
+ of the GNU/Linux native target, so we do not need to map this to
+ T. */
+ linux_nat_delete_thread = delete_thread;
+}
+
/* See declaration in linux-nat.h. */
void
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index a783674..8d8e033 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -169,6 +169,9 @@ void linux_nat_add_target (struct target_ops *);
/* Register a method to call whenever a new thread is attached. */
void linux_nat_set_new_thread (struct target_ops *, void (*) (struct lwp_info *));
+/* Register a method to call whenever a new thread is deleted. */
+void linux_nat_set_delete_thread (struct target_ops *,
+ void (*) (struct lwp_info *));
/* Register a method to call whenever a new fork is attached. */
typedef void (linux_nat_new_fork_ftype) (struct lwp_info *parent,
diff --git a/gdb/nat/x86-linux.c b/gdb/nat/x86-linux.c
index b499e74..e2e9cee 100644
--- a/gdb/nat/x86-linux.c
+++ b/gdb/nat/x86-linux.c
@@ -65,6 +65,12 @@ x86_linux_new_thread (struct lwp_info *lwp)
lwp_set_debug_registers_changed (lwp, 1);
}
+void
+x86_linux_delete_thread (struct lwp_info *lwp)
+{
+ xfree (lwp_arch_private_info (lwp));
+}
+
/* See nat/x86-linux.h. */
void
diff --git a/gdb/nat/x86-linux.h b/gdb/nat/x86-linux.h
index 1b7fee4..32c4922 100644
--- a/gdb/nat/x86-linux.h
+++ b/gdb/nat/x86-linux.h
@@ -39,6 +39,10 @@ extern int lwp_debug_registers_changed (struct lwp_info *lwp);
extern void x86_linux_new_thread (struct lwp_info *lwp);
+/* Function to call when a thread is being deleted. */
+
+extern void x86_linux_delete_thread (struct lwp_info *lwp);
+
/* Function to call prior to resuming a thread. */
extern void x86_linux_prepare_to_resume (struct lwp_info *lwp);
diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
index 2c4afb1..d0480bd 100644
--- a/gdb/x86-linux-nat.c
+++ b/gdb/x86-linux-nat.c
@@ -387,6 +387,7 @@ x86_linux_add_target (struct target_ops *t)
{
linux_nat_add_target (t);
linux_nat_set_new_thread (t, x86_linux_new_thread);
+ linux_nat_set_delete_thread (t, x86_linux_delete_thread);
linux_nat_set_new_fork (t, x86_linux_new_fork);
linux_nat_set_forget_process (t, x86_forget_process);
linux_nat_set_prepare_to_resume (t, x86_linux_prepare_to_resume);
--
2.5.5
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/3] Create arch_lwp_info class hierarchy
2017-08-14 11:53 ` Pedro Alves
@ 2017-08-16 18:44 ` Simon Marchi
2017-08-18 11:23 ` Pedro Alves
0 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2017-08-16 18:44 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
On 2017-08-14 13:53, Pedro Alves wrote:
> On 08/12/2017 12:29 PM, Simon Marchi wrote:
>
>> But then I realized that I forgot to include the header for s390, and
>> the compiler (when building for a s390 host) didn't warn me. This is
>> dangerous and fragile since we end up with two definitions of
>> arch_lwp_info (the s390 one and that fallback one), and nothing to
>> warn
>> about it.
>
> Not sure whether that's really such a bad problem. It seems like the
> sort of thing that'd crash quite visibly/quickly if you actually try to
> run the resulting binary. It's not like we're adding ports
> every day. :-)
How would it crash visibly? I thought it would most likely crash or
corrupt memory seemingly randomly. That's irrelevant because of you
suggestion below, but I'm asking just for the sake of the discussion.
>> So I changed it to listing explicitly the architectures that
>> don't defined their own arch_lwp_info:
>>
>> ...
>> #elif defined __alpha__ || defined __powerpc__ || ...
>> /* Define a dummy arch_lwp_info for arches that don't define one. */
>> struct arch_lwp_info {};
>> #else
>> # error "Missing arch-specific include."
>> #endif
>>
>> That would work, but requires listing all the arches that need the
>> fallback definition of arch_lwp_info, so it gets pretty ugly.
>>
>> Any idea to make this simple but safe? Otherwise, I'll just go with
>> the
>> current version of the patch.
>
> There's a 3rd, much simpler option.
>
> The problem we ran into is one of blurred division of responsibility:
> the
> arch-specific code is responsible for allocating the arch-specific
> arch_lwp_info, which it must be, because it's not possible to allocate
> an
> incomplete type (must know its size), but we free the object in common
> code,
> where the type is opaque.
>
> We can solve the original xfree poisoning problem by simply making the
> arch-specific code responsible for releasing arch_lwp_info too, where
> the
> type is known, just like it is responsible for allocating it.
>
> See below a proof of concept doing that for the x86 port. Making other
> archs do the same should be trivial. Using new/delete/cdtors/in-class
> initialization for arch_lwp_info would be a separate, orthogonal change
> (and so would class-ification of linux_nat_new_thread,
> linux_target_ops,
> etc.).
Yes, that makes sense. I'll try that. Thanks for the prototype.
Simon
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/3] Create arch_lwp_info class hierarchy
2017-08-16 18:44 ` Simon Marchi
@ 2017-08-18 11:23 ` Pedro Alves
0 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2017-08-18 11:23 UTC (permalink / raw)
To: Simon Marchi; +Cc: Simon Marchi, gdb-patches
On 08/16/2017 07:44 PM, Simon Marchi wrote:
> On 2017-08-14 13:53, Pedro Alves wrote:
>> On 08/12/2017 12:29 PM, Simon Marchi wrote:
>>
>>> But then I realized that I forgot to include the header for s390, and
>>> the compiler (when building for a s390 host) didn't warn me. This is
>>> dangerous and fragile since we end up with two definitions of
>>> arch_lwp_info (the s390 one and that fallback one), and nothing to warn
>>> about it.
>>
>> Not sure whether that's really such a bad problem. It seems like the
>> sort of thing that'd crash quite visibly/quickly if you actually try to
>> run the resulting binary. It's not like we're adding ports
>> every day. :-)
>
> How would it crash visibly? I thought it would most likely crash or
> corrupt memory seemingly randomly.
Yeah, I'm just assuming that that randomness would be
noticeable quickly, say, in that it likely wouldn't survive
a test run at least.
> That's irrelevant because of you
> suggestion below, but I'm asking just for the sake of the discussion.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 15+ messages in thread