Re: [PATCHv9 02/14] gdb: create_breakpoint: asserts relating to extra_string/parse_extra

Andrew Burgess
2024-03-31T10:27:32Z

Andrew Burgess <aburgess@redhat.com> writes:

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

I've gone ahead and committed this patch.

Thanks,
Andrew