gdb/testsuite: rework bp-cond-failure to not depend on inlining

  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.

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

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

On 9/20/24 12:35 PM, Keith Seitz wrote:
> 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.
At the top of the exp file, the comment that explains the purpose of the 
test ends with the following:

# We check that the correct breakpoint number appears in the error
# message, and that the error is reported at the correct source
# location.

Would you still like me to add the comment in context, or do you think 
that is enough?
>
>>       } 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
>>       }
>>       }
>>   }
>
  

On 9/20/24 8:56 AM, Guinevere Larsen wrote:
> On 9/20/24 12:35 PM, Keith Seitz wrote:
>>
>> 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.
> At the top of the exp file, the comment that explains the purpose of the 
> test ends with the following:
> 
> # We check that the correct breakpoint number appears in the error
> # message, and that the error is reported at the correct source
> # location.
> 
> Would you still like me to add the comment in context, or do you think 
> that is enough?

Okay, fully optional now, but please consider adding a comment referring
to that explanation, at least. Again, I missed it entirely. :-)

Keith
  

On 9/20/24 12:58 PM, Keith Seitz wrote:
> On 9/20/24 8:56 AM, Guinevere Larsen wrote:
>> On 9/20/24 12:35 PM, Keith Seitz wrote:
>>>
>>> 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.
>> At the top of the exp file, the comment that explains the purpose of 
>> the test ends with the following:
>>
>> # We check that the correct breakpoint number appears in the error
>> # message, and that the error is reported at the correct source
>> # location.
>>
>> Would you still like me to add the comment in context, or do you 
>> think that is enough?
>
> Okay, fully optional now, but please consider adding a comment referring
> to that explanation, at least. Again, I missed it entirely. :-)

I've given some more thought and I think a couple of comments could help 
anyway. Here's my suggestion, thought?

diff --git a/gdb/testsuite/gdb.base/bp-cond-failure.exp 
b/gdb/testsuite/gdb.base/bp-cond-failure.exp
index 403e7db9032..b4c046c4bd6 100644
--- a/gdb/testsuite/gdb.base/bp-cond-failure.exp
+++ b/gdb/testsuite/gdb.base/bp-cond-failure.exp
@@ -57,9 +57,18 @@ proc run_test { cond_eval access_type bpexpr nloc } {

      # Setup the conditional breakpoint and record its number.
      gdb_breakpoint "${bpexpr} if (*(${access_type} *) 0) == 0"
+
+    # This test aims to test that GDB displays the correct breakpoint 
number
+    # and location when there is an error testing a breakpoint condition,
+    # so it is important to hardcode the breakpoint number into the regex,
+    # along with the location, if applicable.
      set bp_num [get_integer_valueof "\$bpnum" "*UNKNOWN*"]

      if { $nloc > 1 } {
+       # We hardcode location 2 because, for some reason, Clang will always
+       # order the debug information so we hit the second location.  For
+       # simplicity the .c is ordered in such a way that GCC will also 
order
+       # the debug info to have us land on location 2.
         gdb_test "continue" \
             [multi_line \
                  "Continuing\\." \
  

On 9/20/24 9:20 AM, Guinevere Larsen wrote:
> 
> I've given some more thought and I think a couple of comments could help 
> anyway. Here's my suggestion, thought?

I am pleased to have an explanation there. Thank you!

Keith
  

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> On 9/20/24 9:20 AM, Guinevere Larsen wrote:
>> I've given some more thought and I think a couple of comments could
>> help anyway. Here's my suggestion, thought?

Keith> I am pleased to have an explanation there. Thank you!

FWIW I read through this and it all seems fine to me as well.
Thanks for your review Keith.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
  

On 9/20/24 3:55 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> Keith> On 9/20/24 9:20 AM, Guinevere Larsen wrote:
>>> I've given some more thought and I think a couple of comments could
>>> help anyway. Here's my suggestion, thought?
> Keith> I am pleased to have an explanation there. Thank you!
>
> FWIW I read through this and it all seems fine to me as well.
> Thanks for your review Keith.
>
> Approved-By: Tom Tromey <tom@tromey.com>
>
> Tom
>
Thanks, pushed!
  

On Friday, September 20, 2024 9:11 PM, Guinevere Larsen wrote:
> On 9/20/24 3:55 PM, Tom Tromey wrote:
> >>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> > Keith> On 9/20/24 9:20 AM, Guinevere Larsen wrote:
> >>> I've given some more thought and I think a couple of comments could
> >>> help anyway. Here's my suggestion, thought?
> > Keith> I am pleased to have an explanation there. Thank you!
> >
> > FWIW I read through this and it all seems fine to me as well.
> > Thanks for your review Keith.
> >
> > Approved-By: Tom Tromey <tom@tromey.com>
> >
> > Tom
> >
> Thanks, pushed!

I'm sorry to chime in late.  Just two comments, in case you'd want to consider
further edits.

>> +      {debug c++}] == -1 } {

It may make sense to rename the file extension to .cc.

>> +  int res = foo ('1');    /* Single-location breakpoint here. */

For a quick reader, changing the argument to 'A' may make it more 
apparent that this is a char.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

"Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:

> On Friday, September 20, 2024 9:11 PM, Guinevere Larsen wrote:
>> On 9/20/24 3:55 PM, Tom Tromey wrote:
>> >>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
>> > Keith> On 9/20/24 9:20 AM, Guinevere Larsen wrote:
>> >>> I've given some more thought and I think a couple of comments could
>> >>> help anyway. Here's my suggestion, thought?
>> > Keith> I am pleased to have an explanation there. Thank you!
>> >
>> > FWIW I read through this and it all seems fine to me as well.
>> > Thanks for your review Keith.
>> >
>> > Approved-By: Tom Tromey <tom@tromey.com>
>> >
>> > Tom
>> >
>> Thanks, pushed!
>
> I'm sorry to chime in late.  Just two comments, in case you'd want to consider
> further edits.
>
>>> +      {debug c++}] == -1 } {
>
> It may make sense to rename the file extension to .cc.

Also, maybe if the test is now just C++, wouldn't moving it to gdb.cp/
make sense?

Thanks,
Andrew


>
>>> +  int res = foo ('1');    /* Single-location breakpoint here. */
>
> For a quick reader, changing the argument to 'A' may make it more 
> apparent that this is a char.
>
> Thanks
> -Baris
>
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
  

On Monday, September 23, 2024 12:41 PM, Andrew Burgess wrote:
> "Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:
> 
> > On Friday, September 20, 2024 9:11 PM, Guinevere Larsen wrote:
> >> On 9/20/24 3:55 PM, Tom Tromey wrote:
> >> >>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> >> > Keith> On 9/20/24 9:20 AM, Guinevere Larsen wrote:
> >> >>> I've given some more thought and I think a couple of comments could
> >> >>> help anyway. Here's my suggestion, thought?
> >> > Keith> I am pleased to have an explanation there. Thank you!
> >> >
> >> > FWIW I read through this and it all seems fine to me as well.
> >> > Thanks for your review Keith.
> >> >
> >> > Approved-By: Tom Tromey <tom@tromey.com>
> >> >
> >> > Tom
> >> >
> >> Thanks, pushed!
> >
> > I'm sorry to chime in late.  Just two comments, in case you'd want to consider
> > further edits.
> >
> >>> +      {debug c++}] == -1 } {
> >
> > It may make sense to rename the file extension to .cc.
> 
> Also, maybe if the test is now just C++, wouldn't moving it to gdb.cp/
> make sense?

FWIW, the test is using C++ only to be able to create two breakpoint locations
via function overloading.  A particular C++ feature is not being tested.  From
that perspective, IMHO it is fine that the test stays in the current folder.
But I also understand that it may be more practical to keep C++ tests in one
folder as much as possible.  There are 7 other .cc files in gdb.base.  If this
one is to be moved, maybe move those others, too?

-Baris



Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928