Re: [PATCH 3/4] gdb/breakpoint: add a 'force_condition' parameter to 'create_breakpoint'

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-04-07 10:55 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> The 'create_breakpoint' function takes a 'parse_extra' argument that
> determines whether the condition, thread, and force-condition
> specifiers should be parsed from the extra string or be used from the
> function arguments.  However, for the case when 'parse_extra' is
> false, there is no way to pass the force-condition specifier.  This
> patch adds it as a new argument.
> 
> Also, in the case when parse_extra is false, the current behavior is
> as if the condition is being forced.  This is a bug.  The default
> behavior should reject the breakpoint.  See below for a demo of this
> incorrect behavior.  (The MI command '-break-insert' uses the
> 'create_breakpoint' function with parse_extra=0.)
> 
>   $ gdb -q --interpreter=mi2 /tmp/simple
>   =thread-group-added,id="i1"
>   =cmd-param-changed,param="history save",value="on"
>   =cmd-param-changed,param="auto-load safe-path",value="/"
>   ~"Reading symbols from /tmp/simple...\n"
>   (gdb)
>   -break-insert -c junk -f main
>   &"warning: failed to validate condition at location 1, disabling:\n  "
>   &"No symbol \"junk\" in current context.\n"
>   ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",cond="junk",times="0",original-location="main"},{number="1.1",enabled="N*",addr="0x000000000000114e",func="main",file="simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]}
>   (gdb)
>   break main if junk
>   &"break main if junk\n"
>   &"No symbol \"junk\" in current context.\n"
>   ^error,msg="No symbol \"junk\" in current context."
>   (gdb)
>   break main -force-condition if junk
>   &"break main -force-condition if junk\n"
>   ~"Note: breakpoint 1 also set at pc 0x114e.\n"
>   &"warning: failed to validate condition at location 1, disabling:\n  "
>   &"No symbol \"junk\" in current context.\n"
>   ~"Breakpoint 2 at 0x114e: file simple.c, line 2.\n"
>   =breakpoint-created,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",cond="junk",times="0",original-location="main"},{number="2.1",enabled="N*",addr="0x000000000000114e",func="main",file="simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]}
>   ^done
>   (gdb)
> 
> After applying this patch, we get the behavior below:
> 
>   (gdb)
>   -break-insert -c junk -f main
>   ^error,msg="No symbol \"junk\" in current context."

You can also mention that this restores the previous behavior (present
in existing releases).

> diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
> index 9d3d7ade6dc..ac13e4d1e09 100644
> --- a/gdb/testsuite/gdb.mi/mi-break.exp
> +++ b/gdb/testsuite/gdb.mi/mi-break.exp
> @@ -224,6 +224,19 @@ proc test_error {} {
>      mi_gdb_test "-break-insert -c i==4 \"callme if i < 4\"" \
>          ".*\\^error,msg=\"Garbage 'if i < 4' at end of location\"" \
>          "conditional breakpoint with garbage after location"
> +
> +    # Try using an invalid condition.
> +    mi_gdb_test "-break-insert -c bad callme" \
> +        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \
> +        "breakpoint with bad condition"
> +
> +    mi_gdb_test "-dprintf-insert -c bad callme 123" \
> +        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \
> +        "dprintf with bad condition"
> +
> +    mi_gdb_test "-break-condition 5 bad" \
> +        ".*\\^error,msg=\"No symbol \\\\\"bad\\\\\" in current context.\"" \
> +        "invalid condition"

Here, does the 5 refer to an existing breakpoint number created by
another test function?  It's a bit annoying when test functions rely on
the state left by other test functions, that makes them harder to debug
in isolation.  In my ideal world, each test_* function in this file
(except test_break, which is just the driver) would spawn a fresh GDB,
such that if you need to debug one of them, you can comment out all the
other ones.

The patch is still OK though, and thanks for taking the time to write a
test.

Simon