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

[Keith Seitz <keiths@redhat.com>]

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