From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4585 invoked by alias); 16 Apr 2013 15:44:49 -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 4576 invoked by uid 89); 16 Apr 2013 15:44:49 -0000 X-Spam-SWARE-Status: No, score=-4.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_BJ autolearn=ham version=3.3.1 Received: from na3sys009aog130.obsmtp.com (HELO na3sys009aog130.obsmtp.com) (74.125.149.143) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 16 Apr 2013 15:44:47 +0000 Received: from mx10.qnx.com ([209.226.137.110]) (using TLSv1) by na3sys009aob130.postini.com ([74.125.148.12]) with SMTP ID DSNKUW1x6jqouEif42XC/Q7zz0TpM/3QEiRX@postini.com; Tue, 16 Apr 2013 08:44:47 PDT Received: by mx10.qnx.com (Postfix, from userid 500) id 72F8020E43; Tue, 16 Apr 2013 11:44:31 -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 mx10.qnx.com (Postfix) with ESMTPS id 23AA91FD6E; Tue, 16 Apr 2013 11:44:31 -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, 16 Apr 2013 11:44:30 -0400 Message-ID: <516D6AF8.3050103@qnx.com> Date: Tue, 16 Apr 2013 17:25:00 -0000 From: Aleksandar Ristovski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Jan Kratochvil CC: Subject: Re: [PATCH 8/8] Tests for validate symbol file using build-id. References: <1365521265-28870-1-git-send-email-ARistovski@qnx.com> <1365521265-28870-9-git-send-email-ARistovski@qnx.com> <20130414141807.GF23227@host2.jankratochvil.net> In-Reply-To: <20130414141807.GF23227@host2.jankratochvil.net> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00484.txt.bz2 On 13-04-14 10:18 AM, Jan Kratochvil wrote: > On Tue, 09 Apr 2013 17:27:45 +0200, Aleksandar Ristovski wrote: .... >> +#define LIB "./solib-mismatch.so" >> + >> +int main(int argc, char *argv[]) > > GNU Coding Standards: > int main (int argc, char *argv[]) Ok. > > >> +{ >> + void *h; > >> + int (*foo)(void); > GNU Coding Standards: > int (*foo) (void); Ok. > > ... > And then you can just: > if (chdir (DIRNAME) != 0) > and that's all. Ok. Also, LIB is now communicated the same way. > > ... >> + h = dlopen(LIB, RTLD_NOW); > > GNU Coding Standards: > h = dlopen (LIB, RTLD_NOW); > Ok. > ... > GNU Coding Standards: > foo = dlsym (h, "foo"); /* set breakpoint 1 here */ > Ok. > ... > GNU Coding Standards: > dlclose (h); > Ok. > ... >> + untested "get_compiler_info failed." > > Missing: > return -1 > > untested does not return on its own. Ok. > ... >> +# Executeable > > typo: > # Executable Ok. > > >> +set srcfile ${testfile}.c >> +set executable ${testfile} >> +set objfile [standard_output_file ${executable}.o] >> +set binfile [standard_output_file ${executable}] > > stcfile and binfile are already set by standard_testfile. > > > Here should be: > file delete -force -- "${binlibfiledirrun}" > otherwise the testcase errors out on: > rm -rf gdb.base/solib-mismatch_wd; touch gdb.base/solib-mismatch_wd; runtest gdb.base/solib-mismatch.exp Ok. > > >> + >> +file mkdir "${binlibfiledirrun}" >> + >> +set exec_opts {} >> + >> +if { ![istarget "*-*-nto-*"] } { >> + set exec_opts [list debug shlib_load] > > Rather lappend. > Ok. > >> +} >> + >> +if { [prepare_for_testing $testfile.exp $executable $srcfile $exec_opts] != 0 } { >> + return -1 >> +} > > I already wrote: > Use build_executable here as prepare_for_testing just additionally calls > clean_restart but you do prepare_for_testing on your own later. Ok. > > >> + >> +if { [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilerun}" "${binlibfilerun}" [list debug ldflags=-Wl,-soname,${binlibfilebase},--build-id]] != "" >> + || [gdb_gnu_strip_debug "${binlibfilerun}"] >> + || [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilegdb}" "${binlibfilegdb}" [list debug ldflags=-Wl,-soname,${binlibfilebase},--build-id]] != "" } { > > -soname is already set by gdb_compile_shlib, why do you set it yourself here? > I don't need to. Rearranged. > ... > > Empty line before a comment. Ok. > >> +# Do not auto load shared libraries, the test needs to have control >> +# over when the relevant output gets printed > > Missing final dot. > > Indent the comment by two spaces right to align with the code. Ok. > > >> + send_gdb "set auto-solib-add off\n" > > Already written before: > > 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. > > The testcase even does not run for me with send_gdb when I use the "read1" > reproducer catching such racy testcases behavior from: > reproducer for races of expect incomplete reads > http://sourceware.org/bugzilla/show_bug.cgi?id=12649 Ok. > > >> + >> + set bp_location [gdb_get_line_number "set breakpoint 1 here"] >> + >> + gdb_breakpoint ${srcfile}:${bp_location} temporary no-message > > I already explained in detail why no-message is inappropriate here. Ok. > > >> + >> + gdb_run_cmd { ${binlibfiledirrun} } > > main() no longer uses argv[1] so you can remove this parameter here. Ok. > > >> + gdb_test "" "set breakpoint 1 here.*" "" > > But all these commands here, specifically these: > > set bp_location [gdb_get_line_number "set breakpoint 1 here"] > gdb_breakpoint ${srcfile}:${bp_location} temporary no-message > gdb_run_cmd { ${binlibfiledirrun} } > gdb_test "" "set breakpoint 1 here.*" "" > > seem overcomplicated to me, it is enough to use: > > if ![runto "${srcfile}:[gdb_get_line_number "set breakpoint 1 here"]"] { > return > } Ok, thanks. > > >> + send_gdb "sharedlibrary\n" >> + gdb_test "" "" "" > > Again never use send_gdb. Ok. > > >> + >> + set nocrlf "\[^\r\n\]*" >> + set expected_header "From${nocrlf}To${nocrlf}Syms${nocrlf}Read${nocrlf}Shared${nocrlf}" >> + set expected_line "${symsloaded}${nocrlf}${solibfile}" >> + >> + gdb_test "info sharedlibrary ${solibfile}" \ >> + "${expected_header}\r\n.*${expected_line}.*" \ >> + "${msg} - Symbols for ${solibfile} loaded: expected '${symsloaded}'" > > Those .* around ${expected_line} destroy the whole purpose of ${nocrlf} as > they can match anything. To make it really single-line one can use for > example: I was aiming at matching ${symsloaded} and ${solibfile} on _the_same_ line, but ignore if there are extra lines. I believe this guards against e.g. ............. Yes ....... SomeOtherLibraryNotSureWhyMatchedTheRegexp ............. No ......... ${solibfile} In this case, if 'Yes' is expected, it would not match. > > set footer_line {\(\*\): Shared library is missing debugging information\.} > + > "${expected_header}\r\n${nocrlf}${expected_line}${nocrlf}(?:\r\n$footer_line)?" \ > > >> + return 0 > > The return value (0) is not used by any caller. And also just "return" at and > of proc is redundant, remove it. Actually, I was going to use it to communicate failures within the function - changed accordingly. If something goes wrong in one run of solib_matching_test, it will print UNTESTED and continue, I believe this would aid diagnostics of the issues. > > ... >> + >> + >> + > > It is a nitpick but you leave in almost all files trailing empty lines. Ok. I think I removed them. I didn't think they were evil :-) Thanks, Aleksandar