Re: [PATCH] Introduce struct packed template, fix -fsanitize=thread for per_cu fields

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

On 7/8/22 16:54, Tom de Vries wrote:
> On 7/7/22 17:26, Pedro Alves wrote:
>> On 2022-07-07 11:18 a.m., Tom de Vries wrote:
>>> On 7/6/22 21:20, Pedro Alves wrote:
>>>> On 2022-07-04 8:45 p.m., Tom de Vries via Gdb-patches wrote:
>>>>> On 7/4/22 20:32, Tom Tromey wrote:
>>>>>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>>>>>
>>>>>> Tom>  /* The number of bits needed to represent all languages, 
>>>>>> with enough
>>>>>> Tom>     padding to allow for reasonable growth.  */
>>>>>> Tom> -#define LANGUAGE_BITS 5
>>>>>> Tom> +#define LANGUAGE_BITS 8
>>>>>>
>>>>>> This will negatively affect the size of symbols and so I think it 
>>>>>> should
>>>>>> be avoided.
>>>>>>
>>>>>
>>>>> Ack, Pedro suggested a way to avoid this:
>>>>> ...
>>>>> +  struct {
>>>>> +    /* The language of this CU.  */
>>>>> +    ENUM_BITFIELD (language) m_lang : LANGUAGE_BITS;
>>>>> +  };
>>>>> ...
>>>>>
>>>>
>>>> It actually doesn't avoid it in this case,
>>>
>>> We were merely discussing the usage of LANGUAGE_BITS for 
>>> general_symbol_info::m_language, and indeed using the "struct { ... 
>>> };" approach avoids changing the LANGUAGE_BITS and introducing a 
>>> penalty on symbol size (which is a more numerous entity than CUs).
>>>
>>
>> Yeah, sorry, I realized it after sending and decided I'd deserve the 
>> incoming cluebat.  :-)
>>
> 
> Heh :)
> 
>>> Still, of course it's also good to keep the dwarf2_per_cu_data struct 
>>> as small as possible, so thanks for looking into this.
>>
>> It's that, but also the desire to settle on some infrastructure or 
>> approach that we can reuse
>> going forward.
>>
> 
> Sure.
> 
>>
>>>> I have not actually tested this with -fsanitize=thread, though.  
>>>> Would you
>>>> be up for testing that, Tom, if this approach looks reasonable?
>>>>
>>>
>>> Yes, of course.
>>>
>>> I've applied the patch and then started with my latest approach which 
>>> avoid locks and uses atomics:
>>
>> Thanks.
>>
>>> ...
>>> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
>>> index f98d8b27649..bc1af0ec2d3 100644
>>> --- a/gdb/dwarf2/read.h
>>> +++ b/gdb/dwarf2/read.h
>>> @@ -108,6 +108,7 @@ struct dwarf2_per_cu_data
>>>         m_header_read_in (false),
>>>         mark (false),
>>>         files_read (false),
>>> +      m_lang (language_unknown),
>>>         scanned (false)
>>>     {
>>>     }
>>> @@ -180,7 +181,7 @@ struct dwarf2_per_cu_data
>>>     packed<dwarf_unit_type, 1> m_unit_type = (dwarf_unit_type) 0;
>>>
>>>     /* The language of this CU.  */
>>> -  packed<language, LANGUAGE_BYTES> m_lang = language_unknown;
>>> +  std::atomic<language> m_lang __attribute__((packed));
>>>
>>>   public:
>>>     /* True if this CU has been scanned by the indexer; false if
>>> @@ -332,11 +333,13 @@ struct dwarf2_per_cu_data
>>>
>>>     void set_lang (enum language lang)
>>>     {
>>> -    /* We'd like to be more strict here, similar to what is done in
>>> -       set_unit_type,  but currently a partial unit can go from 
>>> unknown to
>>> -       minimal to ada to c.  */
>>> -    if (m_lang != lang)
>>> -      m_lang = lang;
>>> +    enum language nope = language_unknown;
>>> +    if (m_lang.compare_exchange_strong (nope, lang))
>>> +      return;
>>> +    nope = lang;
>>> +    if (m_lang.compare_exchange_strong (nope, lang))
>>> +      return;
>>> +    gdb_assert_not_reached ();
>>>     }
>>>
>>>     /* Free any cached file names.  */
>>> ...
>>>
>>> I've tried both:
>>> ...
>>>    packed<std::atomic<language>, LANGUAGE_BYTES> m_lang
>>>      = language_unknown;
>>> ...
>>> and:
>>> ...
>>>    std::atomic<packed<language, LANGUAGE_BYTES>> m_lang
>>>      = language_unknown;
>>> ...
>>> and both give compilation errors:
>>> ...
>>> src/gdb/dwarf2/read.h:184:58: error: could not convert 
>>> ‘language_unknown’ from ‘language’ to ‘std::atomic<packed<language, 
>>> 1> >’
>>>     std::atomic<packed<language, LANGUAGE_BYTES>> m_lang = 
>>> language_unknown;
>>>                                                            
>>> ^~~~~~~~~~~~~~~~
>>> ...
>>> and:
>>> ...
>>> src/gdb/../gdbsupport/packed.h:84:47: error: bit-field 
>>> ‘std::atomic<language> packed<std::atomic<language>, 1>::m_val’ with 
>>> non-integral type
>>> ...
>>>
>>> Maybe one of the two should work and the pack template needs further 
>>> changes, I'm not sure.
>>
>> Yes, I think std::atomic<packed<language, LANGUAGE_BYTES>> should 
>> work.  We need to write
>> the initialized using {}, like this:
>>
>>    std::atomic<packed<language, LANGUAGE_BYTES>> m_lang 
>> {language_unknown};
>>
>> and then we run into errors when comparing m_lang with enum language.  
>> That is because
>> the preexisting operator==/operator!= would require converting from 
>> enum language to
>> packed<language, LANGUAGE_BYTES>, and then from packed<language, 
>> LANGUAGE_BYTES> to
>> std::atomic<packed<language, LANGUAGE_BYTES>>.  That is two implicit 
>> conversions, but
>> C++ only does one automatically.  We can fix that by adding some 
>> operator==/operator!=
>> implementations.
>>
>> I've done that in patch #1 attached.  I've also ditched the 
>> non-attribute-packed implementation.
>>
> 
> Ack.
> 
>>>
>>> Note btw that the attribute packed works here:
>>> ...
>>> +  std::atomic<language> m_lang __attribute__((packed));
>>> ...
>>> in the sense that it's got alignment 1:
>>> ...
>>>          struct atomic<language>    m_lang \
>>>            __attribute__((__aligned__(1))); /*    16     4 */
>>> ...
>>> but given that there's no LANGUAGE_BITS/BYTES, we're back to size 4 
>>> for the m_lang field, and size 128 overall.
>>>
>>> So for now I've settled for:
>>> ...
>>> +  std::atomic<LANGUAGE_CONTAINER> m_lang;
>>> ...
>>> which does get me back to size 120.
>>>
>>> WIP patch attached.
>>
>> Please find attached 3 patches:
>>
>> #1 - Introduce struct packed template
>> #2 - your original patch, but using struct packed, split to a separate 
>> patch. commit log updated.
>> #3 - a version of your std::atomic WIP patch that uses 
>> std::atomic<packed>
>>
>> Patches #1 and #2 pass the testsuite cleanly for me.  Patch #3 
>> compiles, but
>> runs into a couple regressions due to the gdb_assert_not_reached in 
>> set_lang
>> being reached.  I am not surprised since that set_lang code in your patch
>> looked WIP and I just blindly converted to the new approach to show 
>> the code
>> compiles.
>>
> 
> I've rebased #1 and #2 on master, and then applied my current WIP set, 
> copying the style that I found in #3.
> 
> I'm currently testing it, I've pushed to 
> https://github.com/vries/gdb/commits/sanitize-thread-7 if you're 
> interested.
> 
> #1 and #2 LGTM.
> 

So, pushed.

Thanks,
- Tom

> Thanks a lot for all the help :)
> 
> - Tom