RE: [PATCH 11/16] test, btrace: add branch trace tests

["Metzger, Markus T" <markus.t.metzger@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2691 bytes --]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Wednesday, May 30, 2012 10:44 PM

Thanks for your review!

> >  *.log
> >  *.sum
> > +
> > +gdb/testsuite/gdb.btrace/list.x
>
> This is excessive.

Removed.


[...]

> > +++ b/gdb/testsuite/gdb.btrace/dec.c
> > @@ -0,0 +1,4 @@
>
> Missing copyright header.

Fixed.


> > +++ b/gdb/testsuite/gdb.btrace/list.s
>
> It should end with .S as it is used in GDB.  It should have i386 in its name 
> as it
> is x86-specific file.

I renamed it to x86-list.S. It is used for i386 and for x86_64.


[...]

> > +++ b/gdb/testsuite/gdb.btrace/list_function.c
> > @@ -0,0 +1,12 @@
>
> Missing copyright header.

Fixed.

[...]

> > +gdb_reinitialize_dir $srcdir/$subdir
> > +gdb_load $binfile
>
> Why not to use prepare_for_testing here?

Prepare_for_testing uses gcc for linking which pulls in the startup object.
I need more control over what gets linked since I want to be able to rely
on the exact addresses in the final executable.

I used it in list_function.exp, though. Thanks for the hint.

[...]

> > +    runto main
>
> Rather always check for 'runto' error codes, it sometimes happens and the
> rest of testfile does not recover well in such case.

Fixed.

> > +
> > +    gdb_test_multiple "btrace enable" "check btrace support" {
> > +        -re "You can't do that when your target is.*" {
>
> Missing termination:
>            -re "You can't do that when your target is.*\r\n$gdb_prompt $" {
>
> As otherwise it will be racy, the leftover prompt can be read in by next
> gdb_test.


Fixed.

> > +            unsupported "target does not support btrace"
> > +            set skip 1
>
> Here should be:
> 	xfail "check btrace support"

Fixed.


> > +        }
> > +        -re "Couldn't enable branch tracing.*Operation not supported"
> > + {
>
> Again.

Fixed.


> > +            unsupported "target does not support btrace"
> > +            set skip 1
>
> Here should be:
> 	xfail "check btrace support"

Fixed.


> > +        }
>
> > +        -re "$gdb_prompt $" {}
>
> One may disagree but I think this block should contain:
> 	pass "check btrace support"

I added it.


> > +    }
> > +    gdb_exit
> > +    remote_file build delete $testfile
> > +
> > +    return $skip
> > +}
> > +
> > +proc btrace_reset_trace {} {
> > +    gdb_test_no_output "btr disable"
> > +    gdb_test_no_output "btr enable"
> > +
> > +    gdb_test "btr list" "No trace." "reset btrace"
>
> For real correctness either
>        gdb_test "btr list" "No trace\\." "reset btrace"
> or:
>        gdb_test "btr list" {No trace\.} "reset btrace"

Changed to "No trace\.". "No trace\\." wouldn't compile.

Thanks,
Markus.

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 417 bytes --]

--------------------------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland 
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052