This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] validate binary before use


On 13-04-02 12:53 PM, Jan Kratochvil wrote:
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.

[AR]
Ok, it's been a while since I looked at gdb.exp closely, and 'no-message' was added since. Changed the test as so:

@@ -93,15 +93,7 @@ proc solib_matching_test { solibfile symsloaded msg } {

   set bp_location [gdb_get_line_number "set breakpoint 1 here"]

-  send_gdb "tbreak ${srcfile}:${bp_location}\n"
-  gdb_expect {
-    -re "Temporary breakpoint.*${gdb_prompt} $" {
-    }
-    default {
-      untested "${msg} - Failed to set temp. breakpoint at ${bp_location}"
-      return -1
-    }
-  }
+  gdb_breakpoint ${srcfile}:${bp_location} temporary no-message

   gdb_run_cmd { ${binlibfiledirrun} }
   gdb_expect {



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.


[AR]
It does produce PASS (see gdb.exp, lines 476-488), but this comment is not relevant as test does not use 'run' any more, it uses gdb_run_cmd; it does not need to stop at main, just run to the line it wants.




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

[AR] How can it print >= 2 lines? I will augument regex to explicitly look for ${solibfile}

@@ -124,7 +124,7 @@ proc solib_matching_test { solibfile symsloaded msg } {
   }

   gdb_test "info sharedlibrary ${solibfile}" \
-    "From.*To.*Syms.*Read.*Shared.*\r\n.*${symsloaded}.*" \
+    "From.*To.*Syms.*Read.*Shared.*\r\n.*${symsloaded}.*${solibfile}.*" \
     "${msg} - Symbols for ${solibfile} loaded: expected '${symsloaded}'"
   return 0
 }



Thanks,

Aleksandar


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]