Re: [PATCH v2 2/2] BFD error message suppression test case

[Simon Marchi <simark@simark.ca>]

On 9/7/22 13:30, Kevin Buettner via Gdb-patches wrote:
> This commit adds a GDB test case which tests GDB's BFD error handler
> hook for suppressing output of all but the first identical messages.
>
> See the comment at the beginning of bfd-errors.exp for details about
> this new test.
>
> I've tested this test for both 32- and 64-bit ELF files and also
> on both little endian and big endian machines.  It also works for
> both native and remote targets.  The only major restriction is that
> it only works for ELF targets.

I admire your committment at writing such a test case!  LGTM, just some
nits below.

> ---
>  gdb/testsuite/gdb.base/bfd-errors-lib.c |  44 ++++++
>  gdb/testsuite/gdb.base/bfd-errors.exp   | 187 ++++++++++++++++++++++++
>  2 files changed, 231 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/bfd-errors-lib.c
>  create mode 100644 gdb/testsuite/gdb.base/bfd-errors.exp
>
> diff --git a/gdb/testsuite/gdb.base/bfd-errors-lib.c b/gdb/testsuite/gdb.base/bfd-errors-lib.c
> new file mode 100644
> index 00000000000..2b582d14c58
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/bfd-errors-lib.c
> @@ -0,0 +1,44 @@
> +/* Copyright 2022 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 <stdio.h>
> +
> +void
> +foo1 ()
> +{
> +  printf ("foo1 in lib\n");
> +  return;
> +}
> +
> +void
> +foo2()
> +{
> +  printf ("foo2 in lib\n");
> +  return;
> +}
> +
> +void
> +foo3()
> +{
> +  printf ("foo3 in lib\n");
> +  return;
> +}
> +
> +void
> +foo4()
> +{
> +  printf ("foo4 in lib\n");
> +  return;
> +}
> diff --git a/gdb/testsuite/gdb.base/bfd-errors.exp b/gdb/testsuite/gdb.base/bfd-errors.exp
> new file mode 100644
> index 00000000000..141ff6a0765
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/bfd-errors.exp
> @@ -0,0 +1,187 @@
> +# Copyright 2022 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/>.
> +
> +# Tools which use the BFD library will output error messages of the
> +# form "BFD: some messsage" when a problem with the file upon which it
> +# operating is found.  E.g. an actual message (modulo some shortening
> +# of the pathname) from this test is:
> +#
> +# BFD: bfd-errors-lib.so: invalid string offset 1154 >= 154 for section `.dynstr'
> +#
> +# For some problems with executable files or libraries, BFD will
> +# attempt to output many identical messages.  Code has been added to
> +# GDB to suppress messages which are identical to earlier messages
> +# that have already been printed.
> +#
> +# This test makes sure that (all but the first) identical BFD messages
> +# are suppresssed and also that differing messages are output at least
> +# once.
> +#
> +# To accomplish this, a shared object with at least four symbols is
> +# created.  The .dynsym section is extracted and offsets which should
> +# refer to strings in the .dynstr section are changed to be
> +# larger than the size of the .dynstr section.  Only two (different)
> +# offsets are used; thus BFD will attempt to output at least two pairs
> +# of identical messages.  (And it would do this too if not intercepted
> +# by the hook placed by GDB.)  After modifying the extracted section,
> +# the mangled section is placed back into the shared object.
> +#
> +# This test then loads the shared library's symbol table (and other
> +# debug info) using the 'add-symbol-file' command.  While doing this,
> +# the test observes and records the BFD errors that were output.
> +# Finally, data collected while adding the shared library symbols are
> +# examined to make sure that identical messages were suppressed while
> +# also making sure that at least two messages have been printed.
> +
> +# This test can't be run on targets lacking shared library support
> +# or for non-ELF targets.
> +if { [skip_shlib_tests] || ![is_elf_target] } {
> +    return 0
> +}
> +
> +# Library file names and flags:
> +set lib_basename ${::gdb_test_file_name}-lib
> +set srcfile_lib ${srcdir}/${subdir}/${lib_basename}.c
> +set binfile_lib [standard_output_file ${lib_basename}.so]
> +set lib_flags debug
> +
> +# Compile shared library:
> +if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +# Open the shared library and determine some basic facts.  The key
> +# things that we need to learn are 1) whether the solib is 32-bit or
> +# 64-bit ELF file, and 2) the endianness.
> +set solib_fp [open ${binfile_lib} r]
> +fconfigure $solib_fp -translation binary
> +
> +# Read and check EI_MAG to verify that it's really an ELF file.
> +set data [read $solib_fp 4]
> +if { ![string equal $data "\x7fELF"] } {
> +    untested "shared library is not an ELF file"
> +    return -1
> +}

Should you close $solib_fp in this error path?

> +
> +# Read EI_CLASS for ELF32 versus ELF64.
> +set data [read $solib_fp 1]
> +set is_elf64 [string equal $data "\x02"]
> +
> +# Read EI_DATA to determine data encoding (byte order).
> +set data [read $solib_fp 1]
> +set is_big_endian [string equal $data "\x02"]
> +
> +close $solib_fp
> +
> +set objcopy_program [gdb_find_objcopy]
> +
> +# Extract the .dynsym and .dynstr section from the shared object.
> +if {[run_on_host \
> +	"objcopy dump-section" \
> +	${objcopy_program} \
> +	"--dump-section .dynsym=${binfile_lib}.dynsym --dump-section .dynstr=${binfile_lib}.dynstr ${binfile_lib}"]} {
> +    untested "failed objcopy dump-section"
> +    return -1
> +}

Properly supporting remote-host testing is a pain (mostly because nobody
seems to have such a setup today), but I thought I'd mention it anyway:
here, you use run_on_host, but the "open" above runs on the build.  So
one or the other will not work if the build machine != the host machine.

> +
> +# Determine length of .dynstr.  We'll use the length for creating invalid
> +# offsets into .dynstr.
> +set dynstr_fp [open ${binfile_lib}.dynstr r]
> +fconfigure $dynstr_fp -translation binary
> +set dynstr_len [string length [read $dynstr_fp]]
> +close $dynstr_fp

Would "file size" work?

https://www.tcl.tk/man/tcl8.4/TclCmd/file.html#M34

> +
> +# Open the file containing .dynsym and determine its length.  In this
> +# case, we want to know the length in order to compute the total number
> +# of symbols that it contains.  We also leave the file open for a
> +# while so that we can write invalid offsets to it.
> +set dynsym_fp [open ${binfile_lib}.dynsym r+]
> +fconfigure $dynsym_fp -translation binary
> +set dynsym_len [string length [read $dynsym_fp]]
> +
> +# SZ is the size of the Elf32_Sym / Elf64_Sym struct.  OFF is the
> +# offset into the file.  CNT is one greater than the number of symbols
> +# left to mangle.  Note that, in the loop below, the first symbol is
> +# skipped.  This is intentional since the first symbol is defined by
> +# the ELF specification to be the undefined symbol.
> +set off 0
> +if { $is_elf64 } {
> +    set sz 24
> +} else {
> +    set sz 16
> +}
> +set cnt [expr $dynsym_len / $sz]
> +
> +# Create 32-bit patterns (bad offsets) to write into the st_name area.
> +if { $is_big_endian } {
> +    set pat(0) [binary format I [expr $dynstr_len + 1000]]
> +    set pat(1) [binary format I [expr $dynstr_len + 2000]]
> +} else {
> +    set pat(0) [binary format i [expr $dynstr_len + 1000]]
> +    set pat(1) [binary format i [expr $dynstr_len + 2000]]
> +}
> +
> +# Mangle st_name for the symbols following the first (STN_UNDEF) entry.
> +while { [incr cnt -1] > 0 } {
> +    seek $dynsym_fp [incr off $sz]
> +    puts $dynsym_fp $pat([expr $cnt % 2])
> +}
> +
> +close $dynsym_fp
> +
> +# Replace .dynsym section in shared object with the mangled version.
> +if {[run_on_host \
> +	"objcopy update-section" \
> +	${objcopy_program} \
> +	"--update-section .dynsym=${binfile_lib}.dynsym ${binfile_lib}"]} {
> +    untested "failed objcopy update-section"
> +    return -1
> +}
> +
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir

Use clean_restart.

Simon