Re: [patch] validate binary before use

[Aleksandar Ristovski <aristovski@qnx.com>]

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