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

[Simon Marchi <simark@simark.ca>]

On 11/11/22 10:21, Tom de Vries via Gdb-patches wrote:
> 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".

Thanks.  I think it's strange for these tests to emit an UNTESTED if
gdb_start_cmd fails.  Clearly, something is wrong if that happens.  I'll
send a patch that changes them to fail.

> 
>> 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.

Ah, that would be good too.  We wouldn't have to bake in the knowledge
of which languages use which operator.  But I'm also a bit scared of
other unintended consequences when looking up the main symbol, as I see
the current_language is involved in some places.  To be safe, I'll just
go with my naive patch.  Thanks for the suggestion.

Simon