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

Aktemur, Tankut Baris
2023-12-05T07:40:04Z

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