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
Thanks for the review.
On Wednesday, February 15 2017, Pedro Alves wrote:
> 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.
Fixed.
>> + This file is part of GDB.
>
>> +#ifndef COMMON_THREAD_H
>> +#define COMMON_THREAD_H
>
> This macro name does not patch the file name.
Fixed.
>> +/* 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?
Yes, that works and is better than the current solution, I agree. I'll
implement it.
>>
>> -/* 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.)
With set_executing being called after startup_inferior on GDB, this
change will not be there anymore (i.e., set_executing will not be
touched). But thanks for the heads up, I'll check other places where I
moved functions from GDB to gdbserver and make sure everything is
commented.
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/