Re: [PATCH v8 5/6] gdb/infrun: enable/disable thread events of all targets in stop_all_threads

[Pedro Alves <palves@redhat.com>]

On 5/14/20 9:44 AM, Aktemur, Tankut Baris wrote:
> On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index c5bf2d0ad74..6602bc28d5e 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -4769,8 +4769,12 @@ stop_all_threads (void)
>>
>>    scoped_restore_current_thread restore_thread;
>>
>> -  target_thread_events (1);
>> -  SCOPE_EXIT { target_thread_events (0); };
>> +  /* Enable thread events of all targets.  */
>> +  for (auto *target : all_non_exited_process_targets ())
>> +    {
>> +      switch_to_target_no_thread (target);
>> +      target_thread_events (true);
>> +    }
>>
>>    /* Request threads to stop, and then wait for the stops.  Because
>>       threads we already know about can spawn more threads while we're
>> @@ -4962,6 +4966,13 @@ stop_all_threads (void)
>>  	}
>>      }
>>
>> +  /* Disable thread events of all targets.  */
>> +  for (auto *target : all_non_exited_process_targets ())
>> +    {
>> +      switch_to_target_no_thread (target);
>> +      target_thread_events (false);
>> +    }
>> +
>>    if (debug_infrun)
>>      fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");
>>  }
> 
> In internal communication, Markus pointed out that while the original
> SCOPED_EXIT block would run also in the case of exceptions, the new code
> does not.  Here is an update that uses SCOPED_EXIT for the thread event
> disabling loop, too:
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c5bf2d0ad74..04fcc390d17 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -4769,8 +4769,22 @@ stop_all_threads (void)
> 
>    scoped_restore_current_thread restore_thread;
> 
> -  target_thread_events (1);
> -  SCOPE_EXIT { target_thread_events (0); };
> +  /* Enable thread events of all targets.  */
> +  for (auto *target : all_non_exited_process_targets ())
> +    {
> +      switch_to_target_no_thread (target);
> +      target_thread_events (true);
> +    }
> +
> +  SCOPE_EXIT
> +    {
> +      /* Disable thread events of all targets.  */
> +      for (auto *target : all_non_exited_process_targets ())
> +       {
> +         switch_to_target_no_thread (target);
> +         target_thread_events (false);
> +       }
> +    };
> 

Yes, I noticed it too, but forgot to address it.  Thanks for bringing
it up.

I think the:

  if (debug_infrun)
    fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");

bit should also move to within SCOPE_EXIT, since we currently
print that before the destructors are run.  But that could be seen
as an unrelated change.

Thanks,
Pedro Alves