Re: [PATCH] gdb/testsuite: rework bp-cond-failure to not depend on inlining

Keith Seitz
2024-09-20T15:35:55Z

Hi, Gwen,

On 9/19/24 5:42 AM, Guinevere Larsen wrote:
> The test gdb.base/bp-cond-failure is implicitly expecting that the
> function foo will be inlined twice and gdb will be able to find 2
> locations to place a breakpoint. When clang is used, gdb only finds
> one location which causes the test to fail. Since the test is not
> worried about handling breakpoints on inlined functions, but rather on
> the format of the message on a breakpoint condition fail, this seems
> like a false fail report.

Ah, more inline function fun! Great idea moving to overloaded functions.

> This commit reworks the test to be in c++, and uses function overloading
> to ensure that 2 locations will always be found. Empirical testing
> showed that, for clang, we will land on location 2 with the currest exp

Typo "currest".

> commands, no matter the order of the functions declared, whereas for gcc
> it depends on the order that functions were declared, so they are
> ordered to always land on the second location, this way we are able to
> hardcode it and check for it.

This LGTM with one other minor request (see below).

Reviewed-by: Keith Seitz <keiths@redhat.com>

Keith

> ---
> gdb/testsuite/gdb.base/bp-cond-failure.c | 14 +++++---
> gdb/testsuite/gdb.base/bp-cond-failure.exp | 37 ++++++++++++----------
> 2 files changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/bp-cond-failure.c b/gdb/testsuite/gdb.base/bp-cond-failure.c
> index ffab09873bc..b7421399792 100644
> --- a/gdb/testsuite/gdb.base/bp-cond-failure.c
> +++ b/gdb/testsuite/gdb.base/bp-cond-failure.c
> @@ -15,8 +15,14 @@
> You should have received a copy of the GNU General Public License
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> -static inline int __attribute__((__always_inline__))
> -foo ()
> +static int
> +foo (int x)
> +{
> + return 0;
> +}
> +
> +static int
> +foo (char c)
> {
> return 0; /* Multi-location breakpoint here. */
> }
> @@ -24,7 +30,7 @@ foo ()
> static int __attribute__((noinline))
> bar ()
> {
> - int res = foo (); /* Single-location breakpoint here. */
> + int res = foo ('1'); /* Single-location breakpoint here. */
>
> return res;
> }
> @@ -34,7 +40,7 @@ main ()
> {
> int res = bar ();
>
> - res = foo ();
> + res = foo (1);
>
> return res;
> }
> diff --git a/gdb/testsuite/gdb.base/bp-cond-failure.exp b/gdb/testsuite/gdb.base/bp-cond-failure.exp
> index a82cedd3e36..403e7db9032 100644
> --- a/gdb/testsuite/gdb.base/bp-cond-failure.exp
> +++ b/gdb/testsuite/gdb.base/bp-cond-failure.exp
> @@ -27,7 +27,7 @@
> standard_testfile
>
> if { [prepare_for_testing "failed to prepare" ${binfile} "${srcfile}" \
> - {debug}] == -1 } {
> + {debug c++}] == -1 } {
> return
> }
>
> @@ -44,7 +44,7 @@ if { [is_address_zero_readable] } {
> return
> }
>
> -proc run_test { cond_eval access_type lineno nloc } {
> +proc run_test { cond_eval access_type bpexpr nloc } {
> clean_restart ${::binfile}
>
> if { ![runto_main] } {
> @@ -56,23 +56,28 @@ proc run_test { cond_eval access_type lineno nloc } {
> }
>
> # Setup the conditional breakpoint and record its number.
> - gdb_breakpoint "${::srcfile}:${lineno} if (*(${access_type} *) 0) == 0"
> + gdb_breakpoint "${bpexpr} if (*(${access_type} *) 0) == 0"
> set bp_num [get_integer_valueof "\$bpnum" "*UNKNOWN*"]
>
> if { $nloc > 1 } {
> - set bp_num_pattern "${bp_num}.1"
> + gdb_test "continue" \
> + [multi_line \
> + "Continuing\\." \
> + "Error in testing condition for breakpoint ${bp_num}.2:" \
> + "Cannot access memory at address 0x0" \
> + "" \
> + "Breakpoint ${bp_num}.2, foo \\(c=49 ...\\) at \[^\r\n\]+:\[0-9\]+" \
> + "${::decimal}\\s+\[^\r\n\]+ breakpoint here\\. \[^\r\n\]+"]

In your commit log, you mention why it is appropriate to hard-code
location numbers. Would you mind adding some sort of explanation like
that here?

For the casual visitor to this file that has often been burned by
hard-coded line numbers or locations, it would, at least, save me some
grief.

> } else {
> - set bp_num_pattern "${bp_num}"
> + gdb_test "continue" \
> + [multi_line \
> + "Continuing\\." \
> + "Error in testing condition for breakpoint ${bp_num}:" \
> + "Cannot access memory at address 0x0" \
> + "" \
> + "Breakpoint ${bp_num}, bar \\(\\) at \[^\r\n\]+:\[0-9\]+" \
> + "${::decimal}\\s+\[^\r\n\]+ breakpoint here\\. \[^\r\n\]+"]
> }
> -
> - gdb_test "continue" \
> - [multi_line \
> - "Continuing\\." \
> - "Error in testing condition for breakpoint ${bp_num_pattern}:" \
> - "Cannot access memory at address 0x0" \
> - "" \
> - "Breakpoint ${bp_num_pattern}, \(foo\|bar\) \\(\\) at \[^\r\n\]+:${lineno}" \
> - "${::decimal}\\s+\[^\r\n\]+ breakpoint here\\. \[^\r\n\]+"]
> }
>
> # If we're using a remote target then conditions could be evaulated
> @@ -101,7 +106,7 @@ gdb_test_multiple "show breakpoint condition-evaluation" "" {
> }
>
> # Where the breakpoint will be placed.
> -set bp_line_multi_loc [gdb_get_line_number "Multi-location breakpoint here"]
> +set bp_line_multi_loc "foo"
> set bp_line_single_loc [gdb_get_line_number "Single-location breakpoint here"]
>
> foreach_with_prefix access_type { "char" "short" "int" "long long" } {
> @@ -110,7 +115,7 @@ foreach_with_prefix access_type { "char" "short" "int" "long long" } {
> run_test $cond_eval $access_type $bp_line_multi_loc 2
> }
> with_test_prefix "single-loc" {
> - run_test $cond_eval $access_type $bp_line_single_loc 1
> + run_test $cond_eval $access_type "${srcfile}:${bp_line_single_loc}" 1
> }
> }
> }