Re: [PATCH] Fix "set debug parser"

[Simon Marchi <simark@simark.ca>]

On 2025-05-02 16:30, Simon Marchi wrote:
> 
> 
> On 2025-04-26 19:12, Tom Tromey wrote:
>> While debugging my longer series, I discovered that I broken "set
>> debug parser" a couple years ago.  This patch fixes it and adds a
>> minimal test case so that it, hopefully, will not break again.
>>
>> This patch also adds parser debugging to the C++ name canonicalizer.
> 
> I think this introduced some tsan failures, since multiple threads write
> yydebug without synchronization:
> 
>   WARNING: ThreadSanitizer: data race (pid=1311054)
>     Read of size 4 at 0x58c55ad4a1f0 by thread T5:
>       #0 scoped_restore_tmpl<int>::scoped_restore_tmpl<bool>(int*, bool) /home/simark/src/binutils-gdb/gdb/../gdbsupport/scoped_restore.h:71 (gdb+0xb7a0fe) (BuildId: 8f118bec5111347e0f2a78a7dc2fed594eb4ef55)
>       #1 scoped_restore_tmpl<int> make_scoped_restore<int, bool>(int*, bool) /home/simark/src/binutils-gdb/gdb/../gdbsupport/scoped_restore.h:115 (gdb+0xb6bdbd) (BuildId: 8f118bec5111347e0f2a78a7dc2fed594eb4ef55)
>       #2 cp_demangled_name_to_comp(char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) /home/simark/src/binutils-gdb/gdb/cp-name-parser.y:2051 (gdb+0xfaec69) (BuildId: 8f118bec5111347e0f2a78a7dc2fed594eb4ef55)
>       #3 cp_canonicalize_string(char const*) /home/simark/src/binutils-gdb/gdb/cp-support.c:635 (gdb+0xfbf01d) (BuildId: 8f118bec5111347e0f2a78a7dc2fed594eb4ef55)
>       #4 c_canonicalize_name(char const*) /home/simark/src/binutils-gdb/gdb/c-lang.c:732 (gdb+0xe70f6f) (BuildId: 8f118bec5111347e0f2a78a7dc2fed594eb4ef55)
>       #5 cooked_index_shard::finalize(parent_map_map const*) /home/simark/src/binutils-gdb/gdb/dwarf2/cooked-index-shard.c:273 (gdb+0x104cbcf) (BuildId: 8f118bec5111347e0f2a78a7dc2fed594eb4ef55)
> 
> Changing yydebug to be an std::atomic would probably fix it, but it's
> generated by bison, not sure this is an option.  We'll probably need to
> do a custom scoped restore type for this.
> 
> Simon

I gave this a try, and it's actually not as easy as that.  While we can
lock where we write to yydebug, we can't easily lock where we read
yydebug, because it is in bison-generated code.  So it still triggers
TSan.

Also, I'm not sure that it's a good idea to restore yydebug (i.e. to use
scoped_restore).  Imaging that parser_debug is 1, meaning that the
intention is to print debug output.

 - Thread 1 saves yydebug's old val (0) and sets yydebug to 1
 - Thread 2 saves yydebug's old val (1) and sets yydebug to 1
 - Thread 1 executes the rest of the function, printing debug output,
   restores yydebug to 0 when exiting
 - Thread 2 executes the rest of the function, but does not print debug
   output, since yydebug was reset by thread 1 (that's bad)
 - Thread 2 restores yydebug to 1 when exiting

It doesn't really matter we leave yydebug set at when exiting, it only
matters that on entry, we set it to the value we want (parser_debug).
So we should just do this on entry:

  yydebug = parser_debug;

It should work well even with multiple threads, but TSan will still
warn.  I am not sure if it's a race condition we really care about, but
it would still be nice to make TSan happy to make debugging important
races easier.

The only idea I have right now is to run a "sed" after generating
cp-name-parser.c, to change the type of "int yydebug" to either an
atomic or a thread_local variable, whichever incurs the lowest read-side
cost.

Simon