Re: [PATCH] New annotation for threads

[Joel Brobecker <brobecker at adacore dot com>]

> 2008-05-18  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* annotate.c (annotate_new_thread): New function for new-thread
> 	annotation.
> 
> 	* annotate.h: (annotate_new_thread): New extern.
> 
> 	* thread.c (add_thread_with_info): Use it.
> 
> 	* Makefile.in (thread.o): Add dependency on annotate.h.

(note sure if the empty lines between each entry was intended - just
make sure to remove them when entering this in the ChangeLog).

I'm not terribly happy about this, but that's nonetheless a reasonable
compromise. So this part is OK.

> 2008-05-18  Nick Roberts  <nickrob@snap.net.nz>
> 
> 	* gdb.base/annota1.exp: Test for new annotation.

I have a few questions regarding this part:

> +set testfile "watch_thread_num"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +set gdb_prompt $old_gdb_prompt
> +
> +if { ![get_compiler_info ${binfile}] && [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] == "" } {
> +
> +    gdb_exit
> +    gdb_start
> +    gdb_reinitialize_dir $srcdir/$subdir
> +    gdb_load ${binfile}
> +    if { ![runto main] } then {
> +	fail "run to main"
> +	return

I think this will cause the test to exit before you had a chance
to restore the gdb_prompt.  I suggest you put the entire block
of code inside a function, and then call that function at the end
of the script.  That way, you can write the body of the function
as if it was its own script (the practice is to "if gdb_compile != ""
then return..." so that you don't need increasing levels of nesting.
but that's very minor, I don't mind the way you wrote it)

Another alternative would be to have a new specific script. It wouldn't
be any more work at all, and might be cleaner (although, you still do
have to be careful about the gdb_prompt).

> +    send_gdb "set annotate 2\n"
> +    gdb_expect {
> +	-re "set annotate 2\r\n$gdb_prompt$" {}
> +    }
> +
> +    send_gdb "next 2\n"
> +    gdb_expect {
> +	-re ".*\032\032new-thread" {
> +	    pass "new thread"
> +	}
> +	timeout { fail "new thread (timeout)" }
> +    }

I think you should be able to use gdb_test in this case, no?
I realize that you are only repeating the practice that is currently
used throughout the file, but we're trying to avoid the send_gdb/
gdb_expect sequence whenever we can, and use either gdb_test or
gdb_test_multiple.  For one thing, both already handle the various
error conditions, including the timeout.

-- 
Joel