Re: [patch] validate binary before use

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Aleksandar,

just some obvious issues of the testsuite first:


On Tue, 02 Apr 2013 18:24:18 +0200, Aleksandar Ristovski wrote:
> >>+  send_gdb "set verbose 1\n"
> >
> >Never (only in some exceptional cases) use send_gdb, it creates races wrt
> >syncing on end of the commands.  Use gdb_test or gdb_test_no_output.
> 
> [AR] I very much dislike using gdb_test unless I actually am doing a
> test. Otherwise, we end up with testcases that tend to have 30-40
> passes but only 2-3 relevant. Thus, when these 2-3 relevant ones
> start to FAIL it is easy to neglect that due to false cozy feeling
> that, well, *most* are still passing.

* Even a single PASS->FAIL can be a serious GDB regression.
* There are still many racy testcases (with "random" results).
* Therefore comparing any PASS/FAIL counts is irrelevant, only diff matters.

Besides that send_gdb really does not work, it does not read the "(gdb) "
response will confuse the later first test which does wait for a response.

If you do not like trivial testcase names then just use:
	gdb_test_no_output "command" ""
or
	gdb_test "command" "response" ""
GDB testsuite handles testcase name "" by omitting it from the output.


> >>+  send_gdb "tbreak ${srcfile}:${bp_location}\n"
> >
> >Do not use send_gdb and there is gdb_breakpoint function.
> 
> [AR] I am not testing setting  breakpoints. I do not want these to
> show up as PASS-es. These passes are irrelevant. The assumption is
> that breakpoints do work; there are other tests for breakponts.

gdb_breakpoint does not produce any PASS message when it succeeds.
But it will FAIL if a problem occured.


> >>+  send_gdb "run\r\n"
> >
> >Use runto_main.  And check its result code.
> 
> [AR] The same. I am not testing run to main. I am testing this
> particular feature. There are other tests that test runto_main.

Again, successful runto_main does not produce any PASS message.


> >>+  gdb_test "info sharedlibrary ${solibfile}" \
> >>+    ".*From.*To.*Syms.*Read.*Shared.*\r\n.*${symsloaded}.*" \
> >         ^^
> >BTW leading .* is excessive, gdb_test regex does not have anchored its start.
> 
> [AR] ok.
> 
> >
> >
> >>+    "Symbols for ${solibfile} loaded: expected '${symsloaded}'"
> >
> >Protect ${symsloaded} by [string_to_regexp $string] as user
> >may have regex-unsafe characters there.
> 
> [AR] symsloaded is argument passed to solib_matching_test, and the
> test is the only user. Ther eare no other users, and the string may
> contain only 'Yes' or 'No'.

OK, I did not notice, I agree string_to_regexp is not needed there.

But when you expect only one shared library make the expectation explicit,
both for a single line and for ${binlibfilebase}.

  gdb_test "info sharedlibrary ${solibfile}" \
    "From\[^\r\n\]*To\[^\r\n\]*Syms\[^\r\n\]*Read\[^\r\n\]*Shared\[^\r\n\]*\r\n\[^\r\n\]*${symsloaded}\[^\r\n\]*[string_to_regexp ${binlibfilebase}]" \
  (I did not test this regex.)

I can very well imagine GDB could print >= 2 lines or a line without
${binlibfilebase} there which could make false PASS.


Thanks,
Jan