[2/2] Initialize main_thread_id earlier
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
Commit Message
I wrote a patch using is_main_thread (), and found it returning false in the
main thread due to main_thread_id not being initialized yet.
Initialization currently takes place in _initialize_run_on_main_thread, but
that's too late for earlier uses.
Fix this by also initializing on first use.
Tested on x86_64-linux.
---
gdb/run-on-main-thread.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
Comments
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
Tom> I wrote a patch using is_main_thread (), and found it returning false in the
Tom> main thread due to main_thread_id not being initialized yet.
Tom> Initialization currently takes place in _initialize_run_on_main_thread, but
Tom> that's too late for earlier uses.
Tom> Fix this by also initializing on first use.
I wonder if it's enough to have:
static std::thread::id main_thread_id = std::this_thread::get_id ();
Tom> +#if CXX_STD_THREAD
Tom> +static void
Tom> +initialize_main_thread_id (void)
"(void)" is only needed for C, you can just write "()" here.
Tom
On 2023-07-28 11:59, Tom de Vries via Gdb-patches wrote:
> I wrote a patch using is_main_thread (), and found it returning false in the
> main thread due to main_thread_id not being initialized yet.
>
> Initialization currently takes place in _initialize_run_on_main_thread, but
> that's too late for earlier uses.
>
> Fix this by also initializing on first use.
>
> Tested on x86_64-linux.
> ---
> gdb/run-on-main-thread.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/run-on-main-thread.c b/gdb/run-on-main-thread.c
> index 91d25dae28f..bee5885d9a6 100644
> --- a/gdb/run-on-main-thread.c
> +++ b/gdb/run-on-main-thread.c
> @@ -94,12 +94,27 @@ run_on_main_thread (std::function<void ()> &&func)
> serial_event_set (runnable_event);
> }
>
> +#if CXX_STD_THREAD
> +static void
> +initialize_main_thread_id (void)
> +{
> + static bool initialized = false;
> +
> + if (initialized)
> + return;
> + initialized = true;
> +
> + main_thread_id = std::this_thread::get_id ();
> +}
> +#endif
> +
> /* See run-on-main-thread.h. */
>
> bool
> is_main_thread ()
> {
> #if CXX_STD_THREAD
> + initialize_main_thread_id ();
This is assuming that is_main_thread() will always be called once by
the main thread, before any other thread could call it. Otherwise if
is_main_thread is called by some other thread first,
is_main_thread -> initialize_main_thread_id will record the wrong thread
id in the main_thread_id. At least a comment somewhere would
be warranted. I think putting:
gdb_assert (is_main_thread ());
somewhere in the early main code would be good. It would prevent such
situation from ever happening undetected.
On 7/28/23 15:10, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Tom> I wrote a patch using is_main_thread (), and found it returning false in the
> Tom> main thread due to main_thread_id not being initialized yet.
>
> Tom> Initialization currently takes place in _initialize_run_on_main_thread, but
> Tom> that's too late for earlier uses.
>
> Tom> Fix this by also initializing on first use.
>
> I wonder if it's enough to have:
>
> static std::thread::id main_thread_id = std::this_thread::get_id ();
>
In principle it's enough but it does expose us to the "Static
Initialization Order Fiasco" [1], so I prefer this solution.
> Tom> +#if CXX_STD_THREAD
> Tom> +static void
> Tom> +initialize_main_thread_id (void)
>
> "(void)" is only needed for C, you can just write "()" here.
>
Ack, I'll update it in my reply to Pedro.
Thanks,
- Tom
[1] https://en.cppreference.com/w/cpp/language/siof
On 7/28/23 17:36, Pedro Alves wrote:
> On 2023-07-28 11:59, Tom de Vries via Gdb-patches wrote:
>> I wrote a patch using is_main_thread (), and found it returning false in the
>> main thread due to main_thread_id not being initialized yet.
>>
>> Initialization currently takes place in _initialize_run_on_main_thread, but
>> that's too late for earlier uses.
>>
>> Fix this by also initializing on first use.
>>
>> Tested on x86_64-linux.
>> ---
>> gdb/run-on-main-thread.c | 17 ++++++++++++++++-
>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/run-on-main-thread.c b/gdb/run-on-main-thread.c
>> index 91d25dae28f..bee5885d9a6 100644
>> --- a/gdb/run-on-main-thread.c
>> +++ b/gdb/run-on-main-thread.c
>> @@ -94,12 +94,27 @@ run_on_main_thread (std::function<void ()> &&func)
>> serial_event_set (runnable_event);
>> }
>>
>> +#if CXX_STD_THREAD
>> +static void
>> +initialize_main_thread_id (void)
>> +{
>> + static bool initialized = false;
>> +
>> + if (initialized)
>> + return;
>> + initialized = true;
>> +
>> + main_thread_id = std::this_thread::get_id ();
>> +}
>> +#endif
>> +
>> /* See run-on-main-thread.h. */
>>
>> bool
>> is_main_thread ()
>> {
>> #if CXX_STD_THREAD
>> + initialize_main_thread_id ();
>
Hi Pedro,
thanks for the review.
> This is assuming that is_main_thread() will always be called once by
> the main thread, before any other thread could call it. Otherwise if
> is_main_thread is called by some other thread first,
> is_main_thread -> initialize_main_thread_id will record the wrong thread
> id in the main_thread_id. At least a comment somewhere would
> be warranted. I think putting:
>
> gdb_assert (is_main_thread ());
>
> somewhere in the early main code would be good. It would prevent such
> situation from ever happening undetected.
>
Updated accordingly. Any further comments?
Thanks,
- Tom
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>> I wonder if it's enough to have:
>> static std::thread::id main_thread_id = std::this_thread::get_id ();
Tom> In principle it's enough but it does expose us to the "Static
Tom> Initialization Order Fiasco" [1], so I prefer this solution.
If we think it's safe enough to initialize it on first use, just moving
the definition into is_main_thread is enough to avoid this problem.
Now, in theory it is possible for gdb to start a thread very early and
then have that thread call is_main_thread, setting the global
incorrectly. If we care about that then we need an explicit call to set
it early during startup.
Tom
On 8/2/23 18:55, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>>> I wonder if it's enough to have:
>>> static std::thread::id main_thread_id = std::this_thread::get_id ();
>
> Tom> In principle it's enough but it does expose us to the "Static
> Tom> Initialization Order Fiasco" [1], so I prefer this solution.
>
> If we think it's safe enough to initialize it on first use, just moving
> the definition into is_main_thread is enough to avoid this problem.
>
Done.
> Now, in theory it is possible for gdb to start a thread very early and
> then have that thread call is_main_thread, setting the global
> incorrectly. If we care about that then we need an explicit call to set
> it early during startup.
Yes, Pedro had the same remark, that was already fixed in the previous
version.
Any further comments?
Thanks,
- Tom
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>> If we think it's safe enough to initialize it on first use, just
>> moving
>> the definition into is_main_thread is enough to avoid this problem.
>>
Tom> Done.
This wasn't really what I meant but it's not important.
Approved-By: Tom Tromey <tom@tromey.com>
Tom
On 8/2/23 22:48, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>>> If we think it's safe enough to initialize it on first use, just
>>> moving
>>> the definition into is_main_thread is enough to avoid this problem.
>>>
>
> Tom> Done.
>
> This wasn't really what I meant but it's not important.
>
Thanks for the review.
Sorry for misunderstanding this, if you'd like me to follow up in any
way, let me know.
Committed.
Thanks,
- Tom
> Approved-By: Tom Tromey <tom@tromey.com>
>
> Tom
@@ -94,12 +94,27 @@ run_on_main_thread (std::function<void ()> &&func)
serial_event_set (runnable_event);
}
+#if CXX_STD_THREAD
+static void
+initialize_main_thread_id (void)
+{
+ static bool initialized = false;
+
+ if (initialized)
+ return;
+ initialized = true;
+
+ main_thread_id = std::this_thread::get_id ();
+}
+#endif
+
/* See run-on-main-thread.h. */
bool
is_main_thread ()
{
#if CXX_STD_THREAD
+ initialize_main_thread_id ();
return std::this_thread::get_id () == main_thread_id;
#else
return true;
@@ -111,7 +126,7 @@ void
_initialize_run_on_main_thread ()
{
#if CXX_STD_THREAD
- main_thread_id = std::this_thread::get_id ();
+ initialize_main_thread_id ();
#endif
runnable_event = make_serial_event ();
add_file_handler (serial_event_fd (runnable_event), run_events, nullptr,