From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1715 invoked by alias); 2 Apr 2013 17:18:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 1654 invoked by uid 89); 2 Apr 2013 17:18:15 -0000 X-Spam-SWARE-Status: No, score=-4.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.1 Received: from na3sys009aog107.obsmtp.com (HELO na3sys009aog107.obsmtp.com) (74.125.149.197) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 02 Apr 2013 17:18:12 +0000 Received: from mx20.qnx.com ([72.1.200.103]) (using TLSv1) by na3sys009aob107.postini.com ([74.125.148.12]) with SMTP ID DSNKUVsS0jjv5+yF9+Cg/4sCaqwGqsLDzr2v@postini.com; Tue, 02 Apr 2013 10:18:12 PDT Received: by mx20.qnx.com (Postfix, from userid 500) id 3CF8D210FC; Tue, 2 Apr 2013 13:18:10 -0400 (EDT) Received: from exhts.ott.qnx.com (exch2 [10.222.2.136]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mx20.qnx.com (Postfix) with ESMTPS id B627120E4F; Tue, 2 Apr 2013 13:18:09 -0400 (EDT) Received: from [10.222.96.215] (10.222.2.5) by EXCH2.ott.qnx.com (10.222.2.136) with Microsoft SMTP Server (TLS) id 14.2.318.4; Tue, 2 Apr 2013 13:18:09 -0400 Message-ID: <515B12D1.7050505@qnx.com> Date: Tue, 02 Apr 2013 18:02:00 -0000 From: Aleksandar Ristovski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130308 Thunderbird/17.0.4 MIME-Version: 1.0 To: Jan Kratochvil CC: "gdb-patches@sourceware.org" Subject: Re: [patch] validate binary before use References: <20121227211328.GA5739@host2.jankratochvil.net> <50DCBBD1.7000707@qnx.com> <5107F591.304@qnx.com> <20130131063518.GA3027@host2.jankratochvil.net> <510A7EB0.90702@qnx.com> <51278A2A.9000802@qnx.com> <512E42D1.3040101@qnx.com> <514C58B2.6090701@qnx.com> <20130328183727.GA14798@host2.jankratochvil.net> <515B0632.1040502@qnx.com> <20130402165306.GA9479@host2.jankratochvil.net> In-Reply-To: <20130402165306.GA9479@host2.jankratochvil.net> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00056.txt.bz2 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