RE: [PATCHv6 02/10] gdb: create_breakpoint: asserts relating to extra_string/parse_extra

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

On Saturday, December 2, 2023 11:42 AM, Andrew Burgess wrote:
> The goal of this commit is to better define the API for
> create_breakpoint especially around the use of extra_string and
> parse_extra.  This will be useful in the next commit when I plan to
> make some changes to create_breakpoint.
> 
> This commit makes one possibly breaking change: until this commit it
> was possible to create thread-specific dprintf breakpoint like this:
> 
>   (gdb) dprintf call_me, thread 1 "%s", "hello"
>   Dprintf 2 at 0x401152: file /tmp/hello.c, line 8.
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   2       dprintf        keep y   0x0000000000401152 in call_me at /tmp/hello.c:8 thread 1
>           stop only in thread 1
>           printf "%s", "hello"
>   (gdb)
> 
> this feature of dprintf was not documented, was not tested, and is

this -> This

> slightly different in syntax to how we create thread specific
> breakpoints and/or watchpoints -- the thread condition appears after
> the first ','.
> 
> I believe that this worked at all was simply by luck.  We happen to
> pass the parse_extra flag as true from dprintf_command to
> create_breakpoint.
> 
> So in this commit I made the choice change this.  We now pass
> parse_extra as false from dprintf_command to create_breakpoint.  With
> this done it is assumed that the only thing in the extra_string is the
> dprintf format and arguments.
> 
> Beyond this change I've updated the comment on create_breakpoint in
> breakpoint.h, and I've then added some asserts into
> create_breakpoint as well as moving around some of the error
> handling.
> 
>  - We now assert on the incoming argument values,
> 
>  - I've moved an error check to sit after the call to
>    find_condition_and_thread_for_sals, this ensures the extra_string
>    was parsed correctly,
> 
> In dprintf_command:
> 
>  - We now throw an error if there is no format string after the
>    dprintf location.  This error was already being thrown, but was
>    being caught later in the process.  With this change we catch the
>    missing string earlier,
> 
>  - And, as mentioned earlier, we pass parse_extra as false when
>    calling create_breakpoint,
> 
> In create_tracepoint_from_upload:
> 
>  - We now throw an error if the parsed location doesn't completely
>    consume the addr_str variable.  This error has now effectively
>    moved out of create_breakpoint.
> ---
>  gdb/breakpoint.c | 42 +++++++++++++++++++++++++++++-------------
>  gdb/breakpoint.h | 44 +++++++++++++++++++++++++++-----------------
>  2 files changed, 56 insertions(+), 30 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index dd415ff42f0..bd28236ce7d 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -9240,6 +9240,17 @@ create_breakpoint (struct gdbarch *gdbarch,
>    if (extra_string != NULL && *extra_string == '\0')
>      extra_string = NULL;
> 
> +  /* A bp_dprintf must always have an accompanying EXTRA_STRING containing
> +     the dprintf format and arguments -- PARSE_EXTRA should always be false
> +     in this case.
> +
> +     For all other breakpoint types, EXTRA_STRING should be nullptr unless
> +     PARSE_EXTRA is true.  */
> +  gdb_assert ((type_wanted == bp_dprintf
> +	       && extra_string != nullptr && !parse_extra)
> +	      || (type_wanted != bp_dprintf
> +		  && (extra_string == nullptr || parse_extra)));
> +

This could be a personal taste, but I'd find it easier to read if
it was written 

  if (type_wanted == bp_dprintf)
    gdb_assert ((extra_string != nullptr) && !parse_extra);
  else
    gdb_assert ((extra_string == nullptr) || parse_extra);

or

  gdb_assert ((type_wanted == bp_dprintf)
		 ? (extra_string != nullptr) && !parse_extra
		 : (extra_string == nullptr) || parse_extra));

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