[PATCH][gdb/testsuite/ada] Fix number-of-bp test in bp_inlined_func.exp

[PATCH][gdb/testsuite/ada] Fix number-of-bp test in bp_inlined_func.exp

[Tom de Vries]

Hi,

Atm bp_inlined_func.exp passes for a combined current gcc and gdb-binutils
repos build but fails for a build with system gcc (7.3.1) and ld (2.29.1).

It checks for 4 breakpoints on read_small:
...
gdb_test "break read_small" \
         "Breakpoint $decimal at $hex: read_small\\. \\(4 locations\\)" \
         "set breakpoint at read_small"
...
and fails because it gets 5 breakpoint locations instead:
...
(gdb) break read_small
Breakpoint 2 at 0x401f9a: read_small. (5 locations)
(gdb) FAIL: gdb.ada/bp_inlined_func.exp: set breakpoint at read_small
...

The 4 expected breakpoint locations are inlined versions of read_small, and
the 5th breakpoint location has this address:
...
(gdb) info breakpoint
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   <MULTIPLE>
1.1                         y     0x0000000000401f9a in b.read_small
                                                   at bp_inlined_func/b.adb:20
...
which is the read_small function itself:
...
(gdb) x 0x0000000000401f9a
0x401f9a <b__read_small+4>:     0x22f8058b
...

This patch updates the test to allow 5 breakpoint locations.

Tested on the configurations mentioned above.

OK for trunk?

Thanks,
- Tom

[gdb/testsuite/ada] Fix number-of-bp test in bp_inlined_func.exp

2018-06-17  Tom de Vries  <tdevries@suse.de>

	* gdb.ada/bp_inlined_func.exp: Allow 5 breakpoint locations.

---
 gdb/testsuite/gdb.ada/bp_inlined_func.exp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.ada/bp_inlined_func.exp b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
index 0f615f5d9b..79f9697124 100644
--- a/gdb/testsuite/gdb.ada/bp_inlined_func.exp
+++ b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
@@ -29,10 +29,10 @@ if ![runto_main] then {
 }
 
 # Check that inserting breakpoint on read_small inlined function inserts
-# 4 breakpoints.
+# 4 breakpoints (or posibbly 5, including the read_small function itself).
 
 gdb_test "break read_small" \
-         "Breakpoint $decimal at $hex: read_small\\. \\(4 locations\\)" \
+         "Breakpoint $decimal at $hex: read_small\\. \\(\[45\] locations\\)" \
          "set breakpoint at read_small"
 
 # We do not verify each breakpoint info, but use continue commands instead

Re: [PATCH][gdb/testsuite/ada] Fix number-of-bp test in bp_inlined_func.exp

[Joel Brobecker]

Hello,

> Atm bp_inlined_func.exp passes for a combined current gcc and gdb-binutils
> repos build but fails for a build with system gcc (7.3.1) and ld (2.29.1).
> 
> It checks for 4 breakpoints on read_small:
> ...
> gdb_test "break read_small" \
>          "Breakpoint $decimal at $hex: read_small\\. \\(4 locations\\)" \
>          "set breakpoint at read_small"
> ...
> and fails because it gets 5 breakpoint locations instead:
> ...
> (gdb) break read_small
> Breakpoint 2 at 0x401f9a: read_small. (5 locations)
> (gdb) FAIL: gdb.ada/bp_inlined_func.exp: set breakpoint at read_small
> ...
> 
> The 4 expected breakpoint locations are inlined versions of read_small, and
> the 5th breakpoint location has this address:
> ...
> (gdb) info breakpoint
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   <MULTIPLE>
> 1.1                         y     0x0000000000401f9a in b.read_small
>                                                    at bp_inlined_func/b.adb:20
> ...
> which is the read_small function itself:
> ...
> (gdb) x 0x0000000000401f9a
> 0x401f9a <b__read_small+4>:     0x22f8058b
> ...
> 
> This patch updates the test to allow 5 breakpoint locations.
> 
> Tested on the configurations mentioned above.
> 
> OK for trunk?

OK with me, with a few comments:

  - Can you include the description you provided above as part of
    the commit's revision log? I would avoid "Atm" and spell it out
    as well. This is a bit of a nitpicking, but it's an easy thing
    to be doing and can help readers less familiar with English.

  - Can you also include the platform itself on which you did the
    testing?

  - One spelling issue -- see below.

> [gdb/testsuite/ada] Fix number-of-bp test in bp_inlined_func.exp
> 
> 2018-06-17  Tom de Vries  <tdevries@suse.de>
> 
> 	* gdb.ada/bp_inlined_func.exp: Allow 5 breakpoint locations.
> 
> ---
>  gdb/testsuite/gdb.ada/bp_inlined_func.exp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.ada/bp_inlined_func.exp b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
> index 0f615f5d9b..79f9697124 100644
> --- a/gdb/testsuite/gdb.ada/bp_inlined_func.exp
> +++ b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
> @@ -29,10 +29,10 @@ if ![runto_main] then {
>  }
>  
>  # Check that inserting breakpoint on read_small inlined function inserts
> -# 4 breakpoints.
> +# 4 breakpoints (or posibbly 5, including the read_small function itself).

"posibbly" -> "possibly".

>  gdb_test "break read_small" \
> -         "Breakpoint $decimal at $hex: read_small\\. \\(4 locations\\)" \
> +         "Breakpoint $decimal at $hex: read_small\\. \\(\[45\] locations\\)" \
>           "set breakpoint at read_small"
>  
>  # We do not verify each breakpoint info, but use continue commands instead

Thank you,
-- 
Joel

Re: [PATCH][gdb/testsuite/ada] Fix number-of-bp test in bp_inlined_func.exp

[Tom de Vries]

On 06/18/2018 02:11 AM, Joel Brobecker wrote:
> Hello,
> 
>> Atm bp_inlined_func.exp passes for a combined current gcc and gdb-binutils
>> repos build but fails for a build with system gcc (7.3.1) and ld (2.29.1).
>>
>> It checks for 4 breakpoints on read_small:
>> ...
>> gdb_test "break read_small" \
>>          "Breakpoint $decimal at $hex: read_small\\. \\(4 locations\\)" \
>>          "set breakpoint at read_small"
>> ...
>> and fails because it gets 5 breakpoint locations instead:
>> ...
>> (gdb) break read_small
>> Breakpoint 2 at 0x401f9a: read_small. (5 locations)
>> (gdb) FAIL: gdb.ada/bp_inlined_func.exp: set breakpoint at read_small
>> ...
>>
>> The 4 expected breakpoint locations are inlined versions of read_small, and
>> the 5th breakpoint location has this address:
>> ...
>> (gdb) info breakpoint
>> Num     Type           Disp Enb Address            What
>> 1       breakpoint     keep y   <MULTIPLE>
>> 1.1                         y     0x0000000000401f9a in b.read_small
>>                                                    at bp_inlined_func/b.adb:20
>> ...
>> which is the read_small function itself:
>> ...
>> (gdb) x 0x0000000000401f9a
>> 0x401f9a <b__read_small+4>:     0x22f8058b
>> ...
>>
>> This patch updates the test to allow 5 breakpoint locations.
>>
>> Tested on the configurations mentioned above.
>>
>> OK for trunk?
> 
> OK with me, with a few comments:
> 
>   - Can you include the description you provided above as part of
>     the commit's revision log?

I had to do this manually until now, and forgot it one time, but I've
now modified my precommit and submission scripts to handle the rationale
as part of the commit log. Indeed the submission email draft was
generated from the commit log containing the rationale.

> I would avoid "Atm" and spell it out
>     as well. This is a bit of a nitpicking, but it's an easy thing
>     to be doing and can help readers less familiar with English.
> 

I'm not a native speaker either, so nitpicks on language issues are
welcome ;)

>   - Can you also include the platform itself on which you did the
>     testing?
> 

Done.

>   - One spelling issue -- see below.
> 

Fixed.

Thanks for the review.

- Tom

>> [gdb/testsuite/ada] Fix number-of-bp test in bp_inlined_func.exp
>>
>> 2018-06-17  Tom de Vries  <tdevries@suse.de>
>>
>> 	* gdb.ada/bp_inlined_func.exp: Allow 5 breakpoint locations.
>>
>> ---
>>  gdb/testsuite/gdb.ada/bp_inlined_func.exp | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.ada/bp_inlined_func.exp b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
>> index 0f615f5d9b..79f9697124 100644
>> --- a/gdb/testsuite/gdb.ada/bp_inlined_func.exp
>> +++ b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
>> @@ -29,10 +29,10 @@ if ![runto_main] then {
>>  }
>>  
>>  # Check that inserting breakpoint on read_small inlined function inserts
>> -# 4 breakpoints.
>> +# 4 breakpoints (or posibbly 5, including the read_small function itself).
> 
> "posibbly" -> "possibly".
> 
>>  gdb_test "break read_small" \
>> -         "Breakpoint $decimal at $hex: read_small\\. \\(4 locations\\)" \
>> +         "Breakpoint $decimal at $hex: read_small\\. \\(\[45\] locations\\)" \
>>           "set breakpoint at read_small"
>>  
>>  # We do not verify each breakpoint info, but use continue commands instead
> 
> Thank you,
> 

Re: [PATCH][gdb/testsuite/ada] Fix number-of-bp test in bp_inlined_func.exp

[Joel Brobecker]

> >   - Can you also include the platform itself on which you did the
> >     testing?
> > 
> 
> Done.
> 
> >   - One spelling issue -- see below.
> > 
> 
> Fixed.
> 
> Thanks for the review.

You are welcome.

In scanning quickly the commit, I noticed you said "x86_64" as
the platform. No need to change anything  now, but for your next
submissions, it's better to include the OS as well. Typically,
you'll see people say "tested on x86_64-linux" or "x86-windows",
or "ppc-elf".  Many times, the OS doesn't matter, but it's always
good to have it, because the behavior does often depend on the OS.

Thanks for the patch, though. GDB is the better for it, and this is
what _really_ matters ;-).

-- 
Joel