Re: ping: [patch 4/6] testsuite: Unify to lib/prelink-support.exp

[Joel Brobecker <brobecker at adacore dot com>]

> gdb/testsuite/
> 2010-03-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/attach-pie-misread.exp: Load prelink-support.exp.  Replace
> 	build_executable by build_executable_own_libs.  Replace "prelink -R"
> 	execution by a call of prelink_yes.  Comment why "prelink -r" needs no
> 	change.
> 	* gdb.base/break-interp.exp: Load prelink-support.exp.  Rename calls of
> 	copy to file_copy.  Move setting opts --dynamic-linker and -rpath,
> 	mkdir $dir and ldd its parsing and copying to lib/prelink-support.exp.
> 	Replace build_executable by build_executable_own_libs's function
> 	build_executable_own_libs.
> 	(prelinkNO): Create new stub to call prelink_no.
> 	(prelinkYES): Create new stub to call prelink_yes.
> 	(test_attach): Rename calls of copy to file_copy.
> 	(section_get, prelinkNO_run, prelinkNO, prelinkYES, symlink_resolve)
> 	(copy): Move to ...
> 	* lib/prelink-support.exp: ... a new file.  Rename prelinkNO to
> 	prelink_no, prelinkYES to prelink_yes, copy to file_copy.
> 	* gdb.base/prelink.exp: Disable testcase also for is_remote and
> 	skip_shlib_tests.  Load prelink-support.exp.  Replace gdb_compile with
> 	special flags by gdb_compile_shlib.  Replace second gdb_compile by
> 	build_executable_own_libs.  Replace "prelink -R" execution by a call of
> 	prelink_yes.  Replace "prelink -u" and second "prelink -R" execution by
> 	a second call of prelink_yes.  Replace restart commands by
> 	clean_restart.
> 	(prelink): Rename to ...
> 	(seen displacement message): ... this test.  Extend its expectation
> 	strictness.

This is OK, but I think it could have been split into smaller patches,
and that might have made the review process a little easier (this patch
took me 90 mins to review).

Just some editorial comments. I realize some of the comments were already
there, but since I'm looking at them, I thought I throw the suggestion in
now, rather than never.

> +    # $prelink_args are not need for "-r".  Moreover it would relocated all the
> +    # copied libraries making the problem no longer reproducible.
>      set command "exec /usr/sbin/prelink -q -N --no-exec-shield -r $shifted_offset $binfile"

Sorry, I don't understand what you are trying to explain here.

> +		    # This $dir is assumed in build_executable_own_libs.
> +		    set dir ${exec}.d

I wonder if this should just be passed as an argument to
build_executable_own_libs to avoid this assumption (just a thought).

> +# Use -soname so that it is listed with " => " by ldd and this testcase makes
> +# a copy of ${libfile} for each prelink variant.

I think it's useful to explain why it needs to be listed with "=>" by ldd.
Also, I think that the fact that the testcase makes a copy of the library
for each prelink variand is unrelated to the -soname trick, is that correct?
If it is, then we need to make it clear, somehow. Right now, having the
two ideas in the same sentence and and "and" seems to indicate that they
are in fact related.

> +# Return 0 byte terminated string read from section SECTION of EXEC.  Return ""
> +# if no such section or 0 byte terminated string was found.  Function is useful
> +# for section ".interp" or ".gnu_debuglink".

IMO, the lines are two long. I don't exactly know what the standard max
length is, but it should be 78 or less.

Small corrections: "0-byte-terminated" or maybe simpler "nul-terminated".
Also, English -> "This function" for section*s*...

> +# Copy SRC to DEST resolving any symlinks of SRC.  Protect script being aborted
> +# if the copy fails.  Return non-zero on success, zero on failure.

Long lines... Also, suggest:

  # Copy SRC to DEST, resolving any symlinks in SRC.  Return nonzero iff
  # the copy was succesful.
  # 
  # This function is guaranteed to never raise any exception, even when
  # the copy fails.

> +# Wrap function build_executable so that the resulting executable is fully
> +# self-sufficient without dependencies on system libraries.  The reason is that
> +# making an executable properly prelinked requires all its dynamically linked
> +# libraries must be also prelinked.  If some of the system libraries is
> +# currently not prelinked we may have no right to prelink (modify it) at its
> +# current system place.  Parameter INTERP may specify different ld.so to use
> +# than the default system one.  Libraries are copied into directory
> +# `${objdir}/${subdir}/${executable}.d'.

Some minor corrections (can you also spot me for errors, in particular
for the return value which wasn't documented in your patch?):

# Wrap function build_executable so that the resulting executable is fully
# self-sufficient (without dependencies on system libraries).  Parameter
# INTERP may be used to specify a loader (ld.so) to be used that is
# different from the default system one.  Libraries on which the executable
# depends are copied into directory `${objdir}/${subdir}/${executable}.d'.
#
# In case of success, return a string containing the arguments to be used
# in order to perform a prelink of the executable obtained.  Return the
# empty string in case of failure.
#
# This can be useful when trying to prelink an executable which might
# depend on system libraries.  To properly prelink an executable, all
# of its dynamically linked libraries must be prelinked as well.  If
# the executable depends on some system libraries, we may not have
# sufficient write priviledges on these files to perform the prelink.
# This is why we make a copy of these shared libraries, and link the
# executable against these copies instead.

> +    # Try to unprelink it first so that if it has been already prelinked before
> +    # we get different address now and the result is not affected by the
> +    # previous $arg state..

Missing commas:

    # Try to unprelink it first so that, if it has been already prelinked
    # before, we get a different address now, making the new result unaffected
    # by any previous prelinking.


> +    # `--no-exec-shield' is for i386 where prelink in the exec-shield mode is
> +    # forced to push all the libraries tight together to fit into the first two
> +    # memory areas (either the ASCII Shield area or at least below the executable).
> +    # In this case its -R option cannot be applied and we falsely FAIL here as if
> +    # the system is already prelinked prelink has no choice how to randomize the
> +    # single new unprelinked library address without wasting the first one/two
> +    # memory areas.  We do not care of the efficiency of loading such resulting
> +    # exec-shield unfriendly prelinked library.

IMO: The sentences go on for too long without punctuation.   I suggest:

# `--no-exec-shield' is for i386, where prelink in the exec-shield mode is
# forced to push all the libraries tight together, in order to fit into
# the first two memory areas (either the ASCII Shield area or at least
# below the executable).  If the prelink was performed in exec-shield mode,
# we can have false prelink failures when the system is already prelinked
# (due to the use of the -R switch), because prelink has no choice on how
# to randomize the single new unprelinked library address without wasting
# the first one/two memory areas.  To prevent this from happening, we use
# the --no-exec-shield switch.  This may have some consequences in terms
# of loading performance, but we do not care in our case.
# 
-- 
Joel