Re: [PATCH 8/8] Tests for validate symbol file using build-id.

Jan Kratochvil
2013-04-14T14:18:19Z

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 <http://www.gnu.org/licenses/>. */
> +
> +
> +#include <dlfcn.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <signal.h>
> +#include <string.h>
> +
> +/* 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 <http://www.gnu.org/licenses/>. */
> +
> +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