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

[Guinevere Larsen <guinevere@redhat.com>]

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

-- 
Cheers,
Guinevere Larsen
She/Her/Hers