Re: [PATCH v2 1/6] [gdb/symtab] Fix data race on index_cache::m_enabled

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

On 8/2/23 21:29, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> With gdb build with -fsanitize=thread and test-case gdb.base/index-cache.exp I
> Tom> run into:
> [race]
> 
> Tom> Fix this by capturing the value of index_cache::m_enabled in the main thread,
> Tom> and using the captured value in the worker thread.
> 
> Thanks for the patch.  I have some stylistic nits but otherwise it seems
> fine to me.
> 
> Tom> +  struct index_cache_store_context ctx (global_index_cache);
> 
> I think we tend to avoid the struct/class keyword in cases like this now.
> 
> Tom> +				 const struct index_cache_store_context &ctx)
> 
> ... basically everywhere.
> 
> Tom> +index_cache_store_context::index_cache_store_context (const index_cache &ic)
> Tom> +{
> Tom> +  m_enabled = ic.enabled ();
> 
> It's better to use the initializer syntax like
> 
>    : m_enabled (ic.enabled ())
> 
> This can be in the header if possible (I didn't try).
> 
> Tom> +struct index_cache_store_context
> Tom> +{
> Tom> +  friend class index_cache;
> Tom> +
> Tom> +  index_cache_store_context(const index_cache &ic);
> 
> Single-argument constructors should be 'explicit' as a rule.

Ack, fixed in a v3 version ( 
https://sourceware.org/pipermail/gdb-patches/2023-August/201339.html ).

Thanks,
- Tom