From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20764 invoked by alias); 14 Apr 2013 14:18:19 -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 20751 invoked by uid 89); 14 Apr 2013 14:18:18 -0000 X-Spam-SWARE-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ,TW_CP,TW_JC autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sun, 14 Apr 2013 14:18:17 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3EEIE8p019509 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 14 Apr 2013 10:18:14 -0400 Received: from host2.jankratochvil.net (ovpn-116-44.ams2.redhat.com [10.36.116.44]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3EEI77A006365 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Sun, 14 Apr 2013 10:18:10 -0400 Date: Mon, 15 Apr 2013 15:12:00 -0000 From: Jan Kratochvil To: Aleksandar Ristovski Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 8/8] Tests for validate symbol file using build-id. Message-ID: <20130414141807.GF23227@host2.jankratochvil.net> References: <1365521265-28870-1-git-send-email-ARistovski@qnx.com> <1365521265-28870-9-git-send-email-ARistovski@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1365521265-28870-9-git-send-email-ARistovski@qnx.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-04/txt/msg00417.txt.bz2 On Tue, 09 Apr 2013 17:27:45 +0200, Aleksandar Ristovski wrote: [...] > --- /dev/null > +++ b/gdb/testsuite/gdb.base/solib-mismatch.c > @@ -0,0 +1,68 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2013 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > + > +#include > +#include > +#include > +#include > +#include > + > +/* The following defines must correspond to solib-mismatch.exp */ > + > +#define DIRNAME "solib-mismatch_wd" > +#define LIB "./solib-mismatch.so" > + > +int main(int argc, char *argv[]) GNU Coding Standards: int main (int argc, char *argv[]) > +{ > + void *h; > + int (*foo)(void); GNU Coding Standards: int (*foo) (void); > + char buff[1024]; > + char *p; > + > + p = strstr (argv[0], DIRNAME); I find it overcomplicated, maybe even fragile, not sure how argv[0] is passed on various platforms. I was already suggesting before the way already used many times in the GDB testsuite: gdb_compile / prepare_for_testing / etc.: additional_flags=-DDIRNAME\=\"${binlibfiledirrun}\" And then you can just: if (chdir (DIRNAME) != 0) and that's all. > + > + if (p == NULL) > + { > + printf ("ERROR - %s could not be found in argv[0]\n", DIRNAME); > + return 1; > + } > + > + p += strlen (DIRNAME); > + > + memcpy (buff, argv[0], p - argv[0]); > + > + buff[p - argv[0]] = '\0'; > + > + if (chdir (buff) != 0) > + { > + printf ("ERROR - Could not cd to %s\n", buff); > + return 1; > + } > + > + h = dlopen(LIB, RTLD_NOW); GNU Coding Standards: h = dlopen (LIB, RTLD_NOW); > + > + if (h == NULL) > + { > + printf ("ERROR - could not open lib %s\n", LIB); > + return 1; > + } > + foo = dlsym(h, "foo"); /* set breakpoint 1 here */ GNU Coding Standards: foo = dlsym (h, "foo"); /* set breakpoint 1 here */ > + dlclose(h); GNU Coding Standards: dlclose (h); > + return 0; > +} > + > diff --git a/gdb/testsuite/gdb.base/solib-mismatch.exp b/gdb/testsuite/gdb.base/solib-mismatch.exp > new file mode 100644 > index 0000000..6d30632 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/solib-mismatch.exp > @@ -0,0 +1,144 @@ > +# Copyright 2013 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . */ > + > +standard_testfile > +set executable $testfile > + > +# Test overview: > +# generate two shared objects. One that will be used by the process > +# and another, modified, that will be found by gdb. Gdb should > +# detect the mismatch and refuse to use mismatched shared object. > + > +if { [get_compiler_info] } { > + untested "get_compiler_info failed." Missing: return -1 untested does not return on its own. > +} > + > +# First version of the object, to be loaded by ld > +set srclibfilerun ${testfile}-lib.c > + > +# Modified version of the object to be loaded by gdb > +# Code in -libmod.c is tuned so it gives a mismatch but > +# leaves .dynamic at the same point. > +set srclibfilegdb ${testfile}-libmod.c > + > +# So file name: > +set binlibfilebase ${testfile}.so > + > +# Setup run directory (where program is run from) > +# It contains executable and '-lib' version of the library. > +set binlibfiledirrun [standard_output_file ${testfile}_wd] > +set binlibfilerun ${binlibfiledirrun}/${binlibfilebase} > + > +# Second solib version is in current directory, '-libmod' version. > +set binlibfiledirgdb [standard_output_file ""] > +set binlibfilegdb ${binlibfiledirgdb}/${binlibfilebase} > + > +# Executeable typo: # Executable > +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 > + > +file mkdir "${binlibfiledirrun}" > + > +set exec_opts {} > + > +if { ![istarget "*-*-nto-*"] } { > + set exec_opts [list debug shlib_load] Rather lappend. > +} > + > +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. > + > +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? > + untested "gdb_compile_shlib failed." > + return -1 > +} > + > +proc solib_matching_test { solibfile symsloaded msg } { > + global gdb_prompt > + global testfile > + global executable > + global srcdir > + global subdir > + global binlibfiledirrun > + global binlibfiledirgdb > + global srcfile > + > + clean_restart ${binlibfiledirrun}/${executable} > + > + send_gdb "set solib-search-path \"${binlibfiledirgdb}\"\n" > + send_gdb "cd ${binlibfiledirgdb}\n" Empty line before a comment. > +# 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. > + 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 > + > + 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. > + > + gdb_run_cmd { ${binlibfiledirrun} } main() no longer uses argv[1] so you can remove this parameter here. > + 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 } > + send_gdb "sharedlibrary\n" > + gdb_test "" "" "" Again never use send_gdb. > + > + 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: 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. > +} > + > +# Copy binary to working dir so it pulls in the library from that dir > +# (by the virtue of $ORIGIN). > +file copy -force "${binlibfiledirgdb}/${executable}" \ > + "${binlibfiledirrun}/${executable}" > + > +# Test unstripped, .dynamic matching > +solib_matching_test "${binlibfilebase}" "No" \ > + "test unstripped, .dynamic matching" > + > +# Keep original so for debugging purposes > +file copy -force "${binlibfilegdb}" "${binlibfilegdb}-orig" > +set objcopy_program [transform objcopy] > +set result [catch "exec $objcopy_program --only-keep-debug ${binlibfilegdb}"] > +if {$result != 0} { > + untested "test --only-keep-debug" > + return -1 > +} > + > +# Test --only-keep-debug, .dynamic matching so > +solib_matching_test "${binlibfilebase}" "No" \ > + "test --only-keep-debug" > + > +# Keep previous so for debugging puroses > +file copy -force "${binlibfilegdb}" "${binlibfilegdb}-orig1" > + > +# Copy loaded so over the one gdb will find > +file copy -force "${binlibfilerun}" "${binlibfilegdb}" > + > +# Now test it does not mis-invalidate matching libraries > +solib_matching_test "${binlibfilebase}" "Yes" \ > + "test matching libraries" > + > + > + It is a nitpick but you leave in almost all files trailing empty lines. Thanks, Jan