RE: [PATCHv6 06/10] gdb: parse pending breakpoint thread/task immediately

[Andrew Burgess <aburgess@redhat.com>]

"Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:

> On Saturday, December 2, 2023 11:42 AM, Andrew Burgess wrote:
>> diff --git a/gdb/break-cond-parse.c b/gdb/break-cond-parse.c
>> new file mode 100644
>> index 00000000000..b59bd7aeeec
>> --- /dev/null
>> +++ b/gdb/break-cond-parse.c
>> @@ -0,0 +1,463 @@
>> +/* Copyright (C) 2023 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#include "defs.h"
>> +#include "gdbsupport/gdb_assert.h"
>> +#include "gdbsupport/selftest.h"
>> +#include "test-target.h"
>> +#include "scoped-mock-context.h"
>> +#include "break-cond-parse.h"
>> +#include "tid-parse.h"
>> +#include "ada-lang.h"
>> +
>> +/* When parsing tokens from a string, which direction are we parsing?
>> +
>> +   Given the following string and pointer 'ptr':
>> +
>> +	ABC DEF GHI JKL
>> +	       ^
>> +	       ptr
>> +
>> +   Parsing 'forward' will return the token 'GHI' and update 'ptr' to point
>> +   between GHI and JKL.  Parsing 'backward' will return the token 'DEF' and
>> +   update 'ptr' to point between ABC and DEF.
>> +*/
>> +
>> +enum class parse_direction
>> +{
>> +  /* Parse the next token forwards.  */
>> +  forward,
>> +
>> +  /* Parse the previous token backwards.  */
>> +  backward
>> +};
>> +
>> +struct token
>> +{
>> +  /* Create a new token.  START points to the first character of the new
>> +     token, while END points at the last character of the new token.
>> +
>> +     Neither START or END can be nullptr, and both START and END must point
>> +     into the same C style string (i.e. there should be no null character
>> +     between START and END).  END must be equal to, or greater than START,
>> +     that is, it is not possible to create a zero length token.  */
>> +
>> +  token (const char *start, const char *end)
>
> Have you considered using std::string_view?
>
>> +    : m_start (start),
>> +      m_end (end)
>> +  {
>> +    gdb_assert (m_end >= m_start);
>> +    gdb_assert (m_start + strlen (m_start) > m_end);
>> +  }
>> +
>> +  /* Return a pointer to the first character of this token.  */
>> +  const char *start () const
>> +  {
>> +    return m_start;
>> +  }
>> +
>> +  /* Return a pointer to the last character of this token.  */
>> +  const char *end () const
>> +  {
>> +    return m_end;
>> +  }
>> +
>> +  /* Return the length of the token.  */
>> +  size_t length () const
>> +  {
>> +    /* The + 1 is because the character at m_end is part of the token.  */
>> +    return m_end - m_start + 1;
>> +  }
>> +
>> +  /* Return true if this token matches STR.  */
>> +  bool matches (const char *str) const
>> +  {
>> +    return strncmp (m_start, str, length ()) == 0;
>> +  }
>> +
>> +private:
>> +  /* The first character of this token.  */
>> +  const char *m_start;
>> +
>> +  /* The last character of this token.  */
>> +  const char *m_end;
>> +};
>> +
>> +/* Find the next token in DIRECTION from *CURR.  */
>> +
>> +static token
>> +find_next_token (const char **curr, parse_direction direction)
>> +{
>> +  const char *tok_start, *tok_end;
>> +
>> +  gdb_assert (**curr != '\0');
>> +
>> +  if (direction == parse_direction::forward)
>> +    {
>> +      *curr = skip_spaces (*curr);
>> +      tok_start = *curr;
>> +      *curr = skip_to_space (*curr);
>> +      tok_end = *curr - 1;
>> +    }
>> +  else
>> +    {
>> +      gdb_assert (direction == parse_direction::backward);
>> +
>> +      while (isspace (**curr))
>> +	--(*curr);
>> +
>> +      tok_end = *curr;
>> +
>> +      while (!isspace (**curr))
>> +	--(*curr);
>> +
>> +      tok_start = (*curr) + 1;
>> +    }
>> +
>> +  return token (tok_start, tok_end);
>> +}
>> +
>> +/* See break-cond-parse.h.  */
>> +
>> +void
>> +create_breakpoint_parse_arg_string
>> +  (const char *tok, gdb::unique_xmalloc_ptr<char> *cond_string,
>> +   int *thread, int *inferior, int *task,
>> +   gdb::unique_xmalloc_ptr<char> *rest, bool *force)
>> +{
>> +  /* Set up the defaults.  */
>> +  cond_string->reset ();
>> +  rest->reset ();
>> +  *thread = -1;
>> +  *inferior = -1;
>> +  *task = -1;
>> +  *force = false;
>> +
>> +  if (tok == nullptr)
>> +    return;
>> +
>> +  /* The "-force-condition" flag gets some special treatment.  If this
>> +     token is immediately after the condition string then we treat this as
>> +     part of the condition string rather than a separate token.  This is
>> +     just a quirk of how this token used to be parsed, and has been
>> +     retained for backwards compatibility.  Maybe this should be updated
>> +     in the future.  */
>
> (I was the author of the -force-condition patch.)
> The quirks about the -force-condition flag were not a deliberate feature.
> For whatever it's worth, I think it's OK to de-prioritize backwards
> compatibility for this flag.  I doubt that it would impact many users.
>
> In fact, in a post-merge comment, Pedro and Tom had suggested converting
> the flag to an option rather than a keyword:
>
> https://sourceware.org/pipermail/gdb-patches/2020-October/172952.html
> https://sourceware.org/pipermail/gdb-patches/2020-December/173802.html
> https://sourceware.org/pipermail/gdb-patches/2020-December/173999.html
>
> I (embarrassingly) did not have the opportunity to get back to it.  Maybe
> it's now the correct time to address the concerns?  I can gladly help.
> What do you think?

I think this sounds like a good idea, but ....

... I took a quick look at how this might be done, and the problem I see
is that the current 'break' option parsing is buried within
string_to_explicit_location_spec, which is called from
string_to_location_spec, which is called from multiple commands.

Which makes sense, currently, all of the options for 'break' are about
specifying the location.

In contrast, -force-condition has nothing to do with the location, but
relates (as you know) to the condition.

We could handle -force-condition in string_to_explicit_location_spec,
but then commands like 'edit', which take a location, but not a
condition would accept -force-condition, and we'd have to have some
mechanism to the 'force' flag true/false value back out of
string_to_explicit_location_spec, either passing a container around, or
storing the 'force' state within the locspec... this all sounds like the
wrong approach.

Better I think would be to pull the option parsing out of
string_to_explicit_location_spec, and move it into the individual
commands.  What we'd ideally want is to convert the current bespoke
option parsing (for location spec options) to use GDB's generic option
parsing mechanism.  It's possible to bind multiple
gdb::option::option_def into a single gdb::option::option_def_group, so
we can imagine that the 'break' command would use a common
gdb::option::option_def from location.h, which defines all the location
spec arguments, and then a separate gdb::option::option_def which adds
the -force-condition flag.  The only issue then is that we'd need to
pass the location spec related arguments into
string_to_explicit_location_spec somehow....

Phew.  Now the next problem is that the gdb::option::option_def contains
a list of gdb::option::*_option_def objects, the precise type of which
defines how the options are parsed.  It we look at how the arguments to
things like -function are parsed, these get special language specific
handling, which the current option mechanism doesn't support, so I think
we'd need to add that, which will mean at least extending the option
process, or maybe even some reworking of the option handling...

All that is to say that I agree with Pedro that this would be better
done as an real option ... but I really don't want to tie this work to
that refactoring if at all possible...

Thanks,
Andrew

>
> ...
>>    try
>>      {
>>        ops->create_sals_from_location_spec (locspec, &canonical);
>> @@ -9285,6 +9122,13 @@ create_breakpoint (struct gdbarch *gdbarch,
>>  	throw;
>>      }
>> 
>> +  /* The only bp_dprintf should have anything in EXTRA_STRING_COPY by this
>
> This sentence sounds odd.  Did you mean "Only the bp_dprintf type should have..."?
>
>> +     point.  For all other breakpoints this indicates an error.  We could
>> +     place this check earlier in the function, but we prefer to see errors
>> +     from the location spec parser before we see this error message.  */
>> +  if (type_wanted != bp_dprintf && extra_string_copy.get () != nullptr)
>> +    error (_("Garbage '%s' at end of command"), extra_string_copy.get ());
>> +
>>    if (!pending && canonical.lsals.empty ())
>>      return 0;
>> 
>
> Regards
> -Baris
>
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928