Re: [PATCH v3 4/6] Share parts of gdb/gdbthread.h with gdbserver

Pedro Alves
2017-02-15T16:15:43Z

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