Re: [PATCH] New annotation for threads

[Nick Roberts <nickrob@snap.net.nz>]

Joel Brobecker writes:
 > > 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).

OK.

 > 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 restore it before this test so I don't think so.

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

Presumably it also means more return points.  Usually the test is done at
the start of the script where nesting would be more inconvenient.

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

I'll put it inside a function.  I don't want to create a separate script
because I think this test belongs with all the other tests for level 2
annotations.

 > > +    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.

Well, I can't get gdb_test to work.  It says in annota1.exp:

# NOTE: When this prompt is in use the gdb_test procedure cannot be used because
# it assumes that the last char after the gdb_prompt is a white space. This is not
# true with this annotated prompt. So we must use send_gdb and gdb_expect.
#

-- 
Nick                                           http://www.inet.net.nz/~nickrob