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

[Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>]

Hi,

When applying this patch, I have:

    Applying: BFD error message suppression test case
    [...]/rebase-apply/patch:269: trailing whitespace.
    # in the dynamic linker/loader due to using a mangled shared object. 
    warning: 1 line adds whitespace errors.

Also, on my systems (ubuntu 22.04 and ubuntu 20.04), the test fails:

    (gdb) catch load bfd-errors-lib
    Catchpoint 1 (load)
    (gdb) PASS: gdb.base/bfd-errors.exp: catch load bfd-errors-lib
    run 
    Starting program: .../gdb/testsuite/outputs/gdb.base/bfd-errors/bfd-errors-main 
    .../gdb/testsuite/outputs/gdb.base/bfd-errors/bfd-errors-main: symbol lookup error: .../gdb/testsuite/outputs/gdb.base/bfd-errors/bfd-errors-main: undefined symbol: foo4
    [Inferior 1 (process 19165) exited with code 0177]
    (gdb) FAIL: gdb.base/bfd-errors.exp: run to catchpoint (the program exited)
    PASS: gdb.base/bfd-errors.exp: consolidated bfd errors
    FAIL: gdb.base/bfd-errors.exp: print all unique bfd error

I have not really looked at the errors in detail.

I also have minor comments inlined in the patch.

On Fri, Sep 02, 2022 at 05:47:59PM -0700, 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.
> ---
>  gdb/testsuite/gdb.base/bfd-errors-lib.c  |  44 +++++
>  gdb/testsuite/gdb.base/bfd-errors-main.c |  31 ++++
>  gdb/testsuite/gdb.base/bfd-errors.exp    | 211 +++++++++++++++++++++++
>  3 files changed, 286 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/bfd-errors-lib.c
>  create mode 100644 gdb/testsuite/gdb.base/bfd-errors-main.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-main.c b/gdb/testsuite/gdb.base/bfd-errors-main.c
> new file mode 100644
> index 00000000000..153f29704eb
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/bfd-errors-main.c
> @@ -0,0 +1,31 @@
> +/* 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>
> +
> +extern void foo1();
> +extern void foo2();
> +extern void foo3();
> +extern void foo4();
> +
> +int main ()
> +{
> +  printf ("in main\n");
> +  foo1 ();
> +  foo2 ();
> +  foo3 ();
> +  foo4 ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/bfd-errors.exp b/gdb/testsuite/gdb.base/bfd-errors.exp
> new file mode 100644
> index 00000000000..6d5a9428dc0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/bfd-errors.exp
> @@ -0,0 +1,211 @@
> +# 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 sets a catchpoint on the shared library load
> +# operation and GDB is run to that catchpoint.  On the way to running
> +# to the catchpoint, the test observes and records the BFD errors that
> +# were output.  Finally, data collected while running to the
> +# catchpoint 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 [list debug ldflags=-Wl,-Bsymbolic]
> +
> +# Main executable names and flags:
> +set main_basename ${::gdb_test_file_name}-main
> +set srcfile ${srcdir}/${subdir}/${main_basename}.c
> +set binfile [standard_output_file ${main_basename}]
> +set bin_flags [list debug shlib=${binfile_lib}]
> +
> +# Compile shared library and main executable:
> +if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
> +     || [gdb_compile ${srcfile} ${binfile} executable $bin_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
> +}
> +
> +# 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
> +}
> +
> +# 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
> +
> +# 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 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
> +gdb_load ${binfile}
> +gdb_load_shlib $binfile_lib
> +
> +# We don't run to main because doing so will sometimes cause a SIGSEGV
> +# in the dynamic linker/loader due to using a mangled shared object. 
> +# Instead, we'll stop at the shared library load.
> +gdb_test "catch load ${lib_basename}" "Catchpoint.*load.*"
> +
> +gdb_run_cmd
> +
> +# Count number of distinct BFD error messages via 'bfd_error_count'
> +# array while running to the catchpoint.
> +set testname "run to catchpoint"
> +gdb_test_multiple "" $testname {
> +    -re "(BFD:\[^\r\n\]*)\[\r\n\]+" {
> +	incr bfd_error_count($expect_out(1,string))
> +	exp_continue
> +    }
> +    -re "Catchpoint .*${lib_basename}.*$gdb_prompt $" {
> +	pass $testname

You could use $gdb_test_name here.  This would avoid the need of the
testname variable.

> +    }
> +}
> +
> +# Examine counts recorded in the 'bfd_error_count' array to see if any
> +# message was printed multiple times.
> +set testname "consolidated bfd errors"
> +set more_than_one 0
> +foreach index [array names bfd_error_count] {
> +    if { $bfd_error_count($index) > 1 } {
> +	incr more_than_one
> +    }
> +}
> +if { $more_than_one > 0 } {
> +    fail $testname
> +} else {
> +    pass $testname
> +}
> +

I guess you could use

    gdb_assert { $more_than_one > 0 } "consolidated bfd errors"

> +# There should have been at least two distinct BFD errors printed.
> +set testname "print all unique bfd errors"
> +if { [array size bfd_error_count] >= 2 } {
> +    pass $testname
> +} else {
> +    fail $testname
> +}

Similarly, I think gdb_assert would be more concise.

Best,
Lancelot.

> +
> +gdb_exit
> +
> +return 0
> -- 
> 2.37.2
>