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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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