This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 4/6] Share parts of gdb/gdbthread.h with gdbserver
- From: Pedro Alves <palves at redhat dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>, GDB Patches <gdb-patches at sourceware dot org>
- Cc: Luis Machado <lgustavo at codesourcery dot com>
- Date: Wed, 15 Feb 2017 16:15:17 +0000
- Subject: Re: [PATCH v3 4/6] Share parts of gdb/gdbthread.h with gdbserver
- Authentication-results: sourceware.org; auth=none
- References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170208032257.15443-1-sergiodj@redhat.com> <20170208032257.15443-5-sergiodj@redhat.com>
On 02/08/2017 03:22 AM, Sergio Durigan Junior wrote:
> diff --git a/gdb/common/common-gdbthread.h b/gdb/common/common-gdbthread.h
> new file mode 100644
> index 0000000..eb66de9
> --- /dev/null
> +++ b/gdb/common/common-gdbthread.h
> @@ -0,0 +1,45 @@
> +/* Common multi-process/thread control defs for GDB and gdbserver.
> + Copyright (C) 1987-2017 Free Software Foundation, Inc.
> + Contributed by Lynx Real-Time Systems, Inc. Los Gatos, CA.
> +
> +
Spurious blank line. That "contributed by" line is meaningless
for this new file, please remove it.
> + This file is part of GDB.
> +#ifndef COMMON_THREAD_H
> +#define COMMON_THREAD_H
This macro name does not patch the file name.
> +/* See common/common-gdbthread.h. */
> +
> +void
> +set_executing (ptid_t ptid ATTRIBUTE_UNUSED, int executing ATTRIBUTE_UNUSED)
> +{
> + gdb_assert (current_thread != NULL);
> + current_thread->last_resume_kind = resume_stop;
> + current_thread->last_status = get_last_target_waitstatus ();
> +}
> +
This is a bit too hacky to live IMO. :-/ The implementation
of the function is doing nothing related to its interface.
Do we really need the set_executing call in the new shared file? AFAICS,
set_executing is only called at the very end of startup_inferior today.
Couldn't we leave that call out of the common code and add it on the
gdb side, after the common startup_inferior returns?
>
> -/* Marks thread PTID as executing, or not. If PTID is minus_one_ptid,
> - marks all threads.
> -
> - Note that this is different from the running state. See the
> - description of state and executing fields of struct
> - thread_info. */
> -extern void set_executing (ptid_t ptid, int executing);
> -
> /* Reports if thread PTID is executing. */
> extern int is_executing (ptid_t ptid);
Having a setter in one place, and the getter somewhere
else is sure to generate confusion. In cases like these,
please leave a breadcrumb comment. (there may be more cases.)
Thanks,
Pedro Alves