[PATCH] Fix "set debug parser"

[PATCH] Fix "set debug parser"

[Tom Tromey]

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.
---
 gdb/cp-name-parser.y             |  3 +++
 gdb/parse.c                      |  4 ++--
 gdb/parser-defs.h                |  3 +++
 gdb/printcmd.c                   |  4 +++-
 gdb/testsuite/gdb.base/exprs.exp | 12 ++++++++++++
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/gdb/cp-name-parser.y b/gdb/cp-name-parser.y
index 10f6e2d1491..e7317b732cc 100644
--- a/gdb/cp-name-parser.y
+++ b/gdb/cp-name-parser.y
@@ -2047,6 +2047,9 @@ cp_demangled_name_to_comp (const char *demangled_name,
   auto result = std::make_unique<demangle_parse_info> ();
   cpname_state state (demangled_name, result.get ());
 
+  scoped_restore restore_yydebug = make_scoped_restore (&yydebug,
+							parser_debug);
+
   if (yyparse (&state))
     {
       if (state.global_errmsg && errmsg)
diff --git a/gdb/parse.c b/gdb/parse.c
index 3108017dbb7..64653c8ae6e 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -60,8 +60,8 @@ show_expressiondebug (struct ui_file *file, int from_tty,
 }
 
 
-/* True if an expression parser should set yydebug.  */
-static bool parser_debug;
+/* See parser-defs.h.  */
+bool parser_debug;
 
 static void
 show_parserdebug (struct ui_file *file, int from_tty,
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index c13a56e0505..f5618f3a9ce 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -389,4 +389,7 @@ extern bool fits_in_type (int n_sign, const gdb_mpz &n, int type_bits,
 
 extern void parser_fprintf (FILE *, const char *, ...) ATTRIBUTE_PRINTF (2, 3);
 
+/* True if an expression parser should set yydebug.  */
+extern bool parser_debug;
+
 #endif /* GDB_PARSER_DEFS_H */
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 2be5eaa15a2..6659c5a41e5 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1320,7 +1320,9 @@ process_print_command_args (const char *args, value_print_options *print_opts,
 	 value, so invert it for parse_expression.  */
       parser_flags flags = 0;
       if (!voidprint)
-	flags = PARSER_VOID_CONTEXT;
+	flags |= PARSER_VOID_CONTEXT;
+      if (parser_debug)
+	flags |= PARSER_DEBUG;
       expression_up expr = parse_expression (exp, nullptr, flags);
       return expr->evaluate ();
     }
diff --git a/gdb/testsuite/gdb.base/exprs.exp b/gdb/testsuite/gdb.base/exprs.exp
index eb2d0e4bd8b..81e78e50e62 100644
--- a/gdb/testsuite/gdb.base/exprs.exp
+++ b/gdb/testsuite/gdb.base/exprs.exp
@@ -284,3 +284,15 @@ gdb_test "print v_short + " \
 # Test for a syntax error in the middle of an expression.
 gdb_test "print v_short =}{= 3" \
     "A syntax error in expression, near `\\}\\{= 3'\\."
+
+gdb_test_no_output "set debug parse 1"
+set saw_start 0
+gdb_test_multiple "print 23" "print with debugging" -lbl {
+    "Starting parse" {
+	set saw_start 1
+	exp_continue
+    }
+    -re ".$decimal = 23" {
+	gdb_assert $saw_start $gdb_test_name
+    }
+}
-- 
2.49.0

Re: [PATCH] Fix "set debug parser"

[Tom de Vries]

On 4/27/25 01:12, Tom Tromey wrote:
> +gdb_test_no_output "set debug parse 1"
> +set saw_start 0
> +gdb_test_multiple "print 23" "print with debugging" -lbl {
> +    "Starting parse" {
> +	set saw_start 1
> +	exp_continue
> +    }
> +    -re ".$decimal = 23" {
> +	gdb_assert $saw_start $gdb_test_name
> +    }
> +}

Hi Tom,

I looked at the test-case part, and noticed a few things:
- the "Starting parse" is without -re prefix (I didn't realize that was
   allowed)
- the gdb_test_multiple doesn't consume the prompt, which causes
   problems for subsequent tests (currently none)
- the matching strings allow sub-line matching.

I initially fixed the prompt consumption part by adding -wrap to '-re 
".$decimal = 23"', but that makes it a two-line pattern which doesn't 
work with line-by-line matching.

So, I get to this fixup patch:
...
diff --git a/gdb/testsuite/gdb.base/exprs.exp 
b/gdb/testsuite/gdb.base/exprs.exp
index 81e78e50e62..99bb277913c 100644
--- a/gdb/testsuite/gdb.base/exprs.exp
+++ b/gdb/testsuite/gdb.base/exprs.exp
@@ -287,12 +287,17 @@ gdb_test "print v_short =}{= 3" \

  gdb_test_no_output "set debug parse 1"
  set saw_start 0
+set saw_val 0
  gdb_test_multiple "print 23" "print with debugging" -lbl {
-    "Starting parse" {
+    -re "\r\nStarting parse(?=\r\n)" {
  	set saw_start 1
  	exp_continue
      }
-    -re ".$decimal = 23" {
-	gdb_assert $saw_start $gdb_test_name
+    -re "\r\n.$decimal = 23(?=\r\n)" {
+	set saw_val 1
+	exp_continue	
+    }
+    -re -wrap "" {
+	gdb_assert { $saw_start && $saw_val } $gdb_test_name
      }
  }
...

I suppose I would do:
...
set re_var [string_to_regexp "$"]$decimal
...
and use it here:
...
     -re "\r\n$re_var = 23(?=\r\n)" {
...
but I guess that's personal preference.

Thanks,
- Tom

Re: [PATCH] Fix "set debug parser"

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> - the gdb_test_multiple doesn't consume the prompt, which causes
Tom>   problems for subsequent tests (currently none)

I don't think I realized this.
gdb_test_multiple seems kind of hard to use TBH.

Tom> So, I get to this fixup patch:

I applied this, thanks.

I'm going to check this in since I was needing it to debug my other
series anyhow.

Tom

Re: [PATCH] Fix "set debug parser"

[Tom de Vries]

On 4/29/25 00:29, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> - the gdb_test_multiple doesn't consume the prompt, which causes
> Tom>   problems for subsequent tests (currently none)
> 
> I don't think I realized this.
> gdb_test_multiple seems kind of hard to use TBH.
> 

Yeah, it is.

I suppose you used it in this case to prevent problems with processing a 
lot of output using a gdb_test.

Another way of handling that is to use gdb_test_lines and related, which 
take care of the reading line-by-line part, and just require specifying 
the regexps you want to check for.

Thanks,
- Tom

> Tom> So, I get to this fixup patch:
> 
> I applied this, thanks.
> 
> I'm going to check this in since I was needing it to debug my other
> series anyhow.
> 
> Tom

Re: [PATCH] Fix "set debug parser"

[Simon Marchi]

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

Re: [PATCH] Fix "set debug parser"

[Simon Marchi]

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

Re: [PATCH] Fix "set debug parser"

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I think this introduced some tsan failures, since multiple threads write
Simon> yydebug without synchronization:

Sorry about that.  I'm afraid to say I didn't even consider this.

We should just revert this patch to cp-name-parser.y.  I'll send a patch
shortly.

Tom

Re: [PATCH] Fix "set debug parser"

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

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

Really for a pure parser, yydebug should not be a global.

IMNSHO yacc causes more problems than it solves.  I think gdb would be
much better off with hand-written parsers.  That's why I rewrote the
Rust parser.

Tom