Re: [RFC 1/3] [gdb/symtab] Fix data race in index_cache::enable

[Tom de Vries <tdevries@suse.de>]

On 7/28/23 20:27, Simon Marchi wrote:

Hi Simon,

thanks for the review.

>> The race is between:
>> - the main thread setting index_cache::m_enabled
>>    (due to command "set index-cache enabled on")
>> - a worker thread reading index_cache::m_enabled to determine whether an
>>    index-cache entry for $exec needs to be written
>>    (due to command "file $exec")
>>
>> Fix this by capturing the value of index_cache::m_enabled during the file
>> command.
> For completeness: in 30392, I initially suggested making m_enabled an
> std::atomic.  To which you responded:
> 
>      By making it a std::atomic<bool>, we make this non-determinism
>      defined behaviour, so tsan stops complaining, but we want
>      deterministic behaviour instead.
> 
> I agree with that.  Capturing the value at the time of the command makes
> sense.
> 

Ack, thanks for making that explicit.

> One comment on the implementation.  Do we really need to save an
> index_cache_store_context object in cooked_index?  Could we:
> 
>   - make get_store_context return it by value
>   - std::move it to the thread (or just copy it, if it's not expected to
>     be large) into the worker thread's closure (in
>     cooked_index::start_writing_index)
>   - pass it down by const-ref to whatever needs it

I've implemented this approach in v2 ( 
https://sourceware.org/pipermail/gdb-patches/2023-August/201273.html ).

I also realized that get_store_context is basically a constructor, so 
I've remodeled things in this way as well.

Thanks,
- Tom