Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

On 11/11/22 14:53, Simon Marchi wrote:
> On 11/11/22 07:37, Tom de Vries wrote:
>> On 11/8/22 22:20, Simon Marchi via Gdb-patches wrote:
>>> -      std::string arg = string_printf ("-qualified %s", main_name ());
>>> +      std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
>>> +                       current_inferior ()->num);
>>
>> Hi,
>>
>> it seems ada doesn't like the syntax, we get:
>> ...
>> (gdb) start ^M
>> Error in expression, near `1'.^M
>> (gdb) UNTESTED: gdb.ada/start.exp: start failed to land inside the right procedure
> 
> Huh, sorry, I missed it because it shows up as UNTESTED, which my CI job
> doesn't flag as a failure.
> 

I only noticed by glancing at gdb.log scrolling by, which got stuck 
waiting for "Starting program:" to appear.  Which I've just realized is 
a testsuite error, so I've fixed this with "[gdb/testsuite] Don't 
timeout on prompt in gdb_start_cmd".

> Here's a patch that fixes it in a rather naive way.  Ideally, we would
> implement proper inferior-specific breakpoints, but in any case we want
> un-break the tests sooner than that.
> 

It fixes the "UNTESTED" for me, and LGTM.

I did wonder if this could be fixed in a way that the expression is 
parsed independent of the current language, setting language to say C 
for the duration of the command.  And that does seem to work:
...
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index bf4a68e3557..f7b1d763838 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -430,7 +430,14 @@ run_command_1 (const char *args, int from_tty, enum 
run_
how run_how)
          spaces unrelated to this inferior.  */
        std::string arg = string_printf ("-qualified %s if $_inferior == 
%d", main_nam
e (),
                                        current_inferior ()->num);
-      tbreak_command (arg.c_str (), 0);
+      {
+       scoped_restore_current_language save_language;
+       scoped_restore save_language_mode
+         = make_scoped_restore (&language_mode);
+       language_mode = language_mode_manual;
+       current_language = language_def (language_c);
+       tbreak_command (arg.c_str (), 0);
+      }
      }

    exec_file = get_exec_file (0);
...

I'm not sure if this is a better solution: it's more intrusive.

Thanks,
- Tom

>  From 28f370e7dda4fb2f240ed29493416e78ed47f176 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Fri, 11 Nov 2022 07:58:35 -0500
> Subject: [PATCH] gdb: fix start breakpoint expression not working in some
>   languages
> 
> Commit 0be837be9fb4 ("gdb: make "start" breakpoint inferior-specific")
> regresses gdb.ada/start.exp:
> 
>      (gdb) start
>      Error in expression, near `1'.
>      (gdb) UNTESTED: gdb.ada/start.exp: start failed to land inside the right procedure
> 
> This is because in Ada, the equality operator is =, not ==.
> 
> I checked the other languages supported by GDB, these other languages
> use = for equality:
> 
>   - Pascal: tests like gdb.pascal/hello.exp are affected too
>   - Modula-2: I tried building a Modula-2 hello world using gm2, but it
>     seems like the generated DWARF doesn't specify the Modula-2 language
>     in the CUs, it's C++ and C, so the selected language isn't
>     "modula-2".  But if I manually do "set language modula-2" on a dummy
>     program and then "start", I get the same error.
> 
> Other languages all use ==.
> 
> So, a short term fix would be to use = or == in the expression, based on
> the current language.  If this was meant to be permanent, I would
> suggest adding something like an "equality_operator" method to
> language_defn, that returns the right equality operator for the
> language.  But the goal is to replace all this with proper
> inferior-specific breakpoints, so I hope all this is temporary.
> 
> Change-Id: Id4d38e14a80e6bbbb1ad2b2277f974dd55192969
> ---
>   gdb/infcmd.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index bf4a68e3557e..6f83949cc7c0 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -428,8 +428,13 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
>   	 have proper inferior-specific breakpoint support, in the breakpoint
>   	 machinery.  We could then avoid inserting a breakpoint in the program
>   	 spaces unrelated to this inferior.  */
> -      std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
> -				       current_inferior ()->num);
> +      const char *op
> +	= ((current_language->la_language == language_ada
> +	    || current_language->la_language == language_pascal
> +	    || current_language->la_language == language_m2) ? "=" : "==");
> +      std::string arg = string_printf
> +	("-qualified %s if $_inferior %s %d", main_name (), op,
> +	 current_inferior ()->num);
>         tbreak_command (arg.c_str (), 0);
>       }
>   
> 
> base-commit: 70b9d05b26e861524d70ee90dcd28cfd77032ddd