This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [tests patchv2] FYI unwinder: tests/ update


On Sat, 2013-11-23 at 21:57 +0100, Jan Kratochvil wrote: 
> ./
> 2013-11-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* configure.ac (CC_BIARCH): Remove AS_IF for it.
> 
> m4/
> 2013-11-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* biarch.m4 (utrace_BIARCH): Call AC_MSG_WARN if !BIARCH.

OK.

> libdwfl/
> 2013-11-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* dwfl_frame.c (dwfl_pid, dwfl_getthreads): Use PROCESS_ATTACH_ERROR if
> 	PROCESS is NULL.
> 	* libdwflP.h (struct Dwfl): New field process_attach_error.
> 	* linux-core-attach.c (__libdwfl_attach_state_for_core): Rename to ...
> 	(attach_state_for_core): ... here, make it static, change return type,
> 	no longer use __libdwfl_seterrno.
> 	(__libdwfl_attach_state_for_core): New wrapper for it.

These were discussed and can be committed separately.

> tests/
> 2013-11-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* Makefile.am (check_PROGRAMS): Add backtrace, backtrace-child,
> 	backtrace-data and backtrace-dwarf.
> 	(BUILT_SOURCES, clean-local, backtrace-child-biarch): New.
> 	(TESTS): Add run-backtrace-native.sh, run-backtrace-data.sh,
> 	run-backtrace-dwarf.sh, run-backtrace-native-biarch.sh,
> 	run-backtrace-native-core.sh, run-backtrace-native-core-biarch.sh,
> 	run-backtrace-core-x86_64.sh and run-backtrace-core-i386.sh.
> 	<!BIARCH> Add export of ELFUTILS_DISABLE_BIARCH.
> 	(EXTRA_DIST): Add run-backtrace-data.sh, run-backtrace-dwarf.sh,
> 	cleanup-13.c, run-backtrace-native.sh, run-backtrace-native-biarch.sh,
> 	run-backtrace-native-core.sh, run-backtrace-native-core-biarch.sh,
> 	run-backtrace-core-x86_64.sh, run-backtrace-core-i386.sh,
> 	backtrace-subr.sh, backtrace.i386.core.bz2, backtrace.i386.exec.bz2,
> 	backtrace.x86_64.core.bz2, backtrace.x86_64.exec.bz2.
> 	(backtrace_LDADD, backtrace_child_CFLAGS, backtrace_child_LDFLAGS)
> 	(backtrace_data_LDADD, backtrace_dwarf_CFLAGS, backtrace_dwarf_LDADD):
> 	New.
> 	* backtrace-child.c: New file.
> 	* backtrace-data.c: New file.
> 	* backtrace-dwarf.c: New file.
> 	* backtrace-subr.sh: New file.
> 	* backtrace.c: New file.
> 	* cleanup-13.c: New file.
> 	* backtrace.i386.core.bz2: New file.
> 	* backtrace.i386.exec.bz2: New file.
> 	* backtrace.x86_64.core.bz2: New file.
> 	* backtrace.x86_64.exec.bz2: New file.
> 	* run-backtrace-core-i386.sh: New file.
> 	* run-backtrace-core-x86_64.sh: New file.
> 	* run-backtrace-native-biarch.sh: New file.
> 	* run-backtrace-native-core-biarch.sh: New file.
> 	* run-backtrace-native-core.sh: New file.
> 	* run-backtrace-native.sh: New file.
> 	* run-backtrace-data.sh: New file.
> 	* run-backtrace-dwarf.sh: New file.

Some comments here and there below. And see some of the comments in my
previous email. But in general after some small nitpicks are resolved
(or not if you think that isn't necessary) I think it is good to go now.

> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 4f8e9e4..15992ed 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -52,10 +52,26 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
>  		  test-flag-nobits dwarf-getstring rerequest_tag \
>  		  alldts md5-sha1-test typeiter typeiter2 low_high_pc \
>  		  test-elf_cntl_gelf_getshdr dwflsyms dwfllines \
> -		  dwfl-report-elf-align varlocs
> +		  dwfl-report-elf-align varlocs backtrace backtrace-child \
> +		  backtrace-data backtrace-dwarf

OK.

> +if BIARCH
> +BUILT_SOURCES = backtrace-child-biarch
> +endif

This makes sure that backtrace-child-biarch is built before any other
target if BIARCH. Why is that necessary? Could you use something like
check_PROGRAMS += backtrace-child-biarch instead or does that not work
with a custom target?

> +clean-local:
> +	$(RM) backtrace-child-biarch
> +
> +# Substitute $(COMPILE).
> +backtrace-child-biarch: backtrace-child.c
> +	$(CC_BIARCH) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) \
> +		     $(AM_CPPFLAGS) $(CPPFLAGS) \
> +		     $(AM_CFLAGS) $(CFLAGS) $(backtrace_child_CFLAGS) \
> +		     $(AM_LDFLAGS) $(LDFLAGS) $(backtrace_child_LDFLAGS) \
> +		     -o $@ $<

OK.

> TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \
>  	update1 update2 update3 update4 \
>  	run-show-die-info.sh run-get-files.sh run-get-lines.sh \
> @@ -89,7 +105,14 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \
>  	run-test-archive64.sh run-readelf-vmcoreinfo.sh \
>  	run-readelf-mixed-corenote.sh run-dwfllines.sh \
>  	run-dwfl-report-elf-align.sh run-addr2line-test.sh \
> -	run-addr2line-i-test.sh run-varlocs.sh
> +	run-addr2line-i-test.sh run-varlocs.sh run-backtrace-native.sh \
> +	run-backtrace-data.sh run-backtrace-dwarf.sh \
> +	run-backtrace-native-biarch.sh run-backtrace-native-core.sh \
> +	run-backtrace-native-core-biarch.sh run-backtrace-core-x86_64.sh \
> +	run-backtrace-core-i386.sh
> +if !BIARCH
> +export ELFUTILS_DISABLE_BIARCH = 1
> +endif

OK. But please insert a blank line between after the TESTS list.

> @@ -216,7 +239,13 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \
>  	     testfile_implicit_pointer.c testfile_implicit_pointer.bz2 \
>  	     testfile_parameter_ref.c testfile_parameter_ref.bz2 \
>  	     testfile_entry_value.c testfile_entry_value.bz2 \
> -	     testfile_implicit_value.c testfile_implicit_value.bz2
> +	     testfile_implicit_value.c testfile_implicit_value.bz2 \
> +	     run-backtrace-data.sh run-backtrace-dwarf.sh cleanup-13.c \
> +	     run-backtrace-native.sh run-backtrace-native-biarch.sh \
> +	     run-backtrace-native-core.sh run-backtrace-native-core-biarch.sh \
> +	     run-backtrace-core-x86_64.sh run-backtrace-core-i386.sh \
> +	     backtrace-subr.sh backtrace.i386.core.bz2 backtrace.i386.exec.bz2 \
> +	     backtrace.x86_64.core.bz2 backtrace.x86_64.exec.bz2

OK.

> @@ -345,6 +374,13 @@ dwflsyms_LDADD = $(libdw) $(libelf) $(libmudflap)
>  dwfllines_LDADD = $(libdw) $(libelf) $(libmudflap)
>  dwfl_report_elf_align_LDADD = $(libdw) $(libmudflap)
>  varlocs_LDADD = $(libdw) $(libelf) $(libmudflap)
> +backtrace_LDADD = $(libdw) $(libelf) $(libmudflap)
> +# backtrace-child-biarch also uses those *_CFLAGS and *_LDLAGS variables:
> +backtrace_child_CFLAGS = -fPIE
> +backtrace_child_LDFLAGS = -pie -pthread
> +backtrace_data_LDADD = $(libdw) $(libelf) $(libmudflap)
> +backtrace_dwarf_CFLAGS = -Wno-unused-parameter
> +backtrace_dwarf_LDADD = $(libdw) $(libelf) $(libmudflap)

OK.

> diff --git a/tests/backtrace-child.c b/tests/backtrace-child.c

I still have some questions about some small details in this file, as
asked in my previous email, but nothing that seems too important. Thanks
for all the comments, that really makes things easier to understand.

> diff --git a/tests/backtrace-data.c b/tests/backtrace-data.c
> +/* Test program for unwinding of frames.
> +#ifndef __x86_64__
> +
> +int
> +main (void)
> +{
> +  return 77;
> +}
> +
> +#else /* __x86_64__ */

You might want to add a comment that this is test mimics what a native
ptrace based backend would do. And that the only arch specific code is
the set_initial_registers function. So that porters know how to adapt it
to their arch.

> +static pid_t
> +next_thread (Dwfl *dwfl, void *dwfl_arg __attribute__ ((unused)),
> +	     void **thread_argp)
> +{
> +  if (*thread_argp != NULL)
> +    return 0;
> +  /* Put arbitrary non-NULL value into *THREAD_ARGP.  */
> +  *thread_argp = thread_argp;
> +  return dwfl_pid (dwfl);
> +}

Why that *thread_argp assignment?

> /* Add module containing ADDR to the DWFL address space.  */
> 
> static Dwfl_Module *
> report_module (Dwfl *dwfl, pid_t child, Dwarf_Addr addr)
> {
>   GElf_Addr base;
>   char *long_name = maps_lookup (child, addr, &base);
>   Dwfl_Module *mod = dwfl_report_elf (dwfl, long_name, long_name, -1,
>                                       base, false /* add_p_vaddr */);
>   assert (mod);
>   free (long_name);
>   assert (dwfl_addrmodule (dwfl, addr) == mod);
>   return mod;
> }

I am not sure this is really a supported way of manipulating the Dwfl
on-the-fly. You should do the dwfl_report between dwfl_report_begin_add
and dwfl_report_end.

This being called from a Dwfl user callback is slightly iffy IMHO. If
you really cannot report the modules at the start before calling
dwfl_getthreads then please do add a comment/warning here that setting
up new Dwfl_Modules from a Dwfl user callback is risky.

> diff --git a/tests/backtrace-dwarf.c b/tests/backtrace-dwarf.c
> +/* Test program for unwinding of complicated DWARF expressions.

OK. The wrapper for cleanup-13.c.

> +static const char *executable = "/proc/self/exe";
> +
> +static int
> +find_elf (Dwfl_Module *mod, void **userdata, const char *modname,
> +	  Dwarf_Addr base, char **file_name, Elf **elfp)
> +{
> +  if (executable && modname != NULL
> +      && (strcmp (modname, "[exe]") == 0 || strcmp (modname, "[pie]") == 0))
> +    {
> +      char *executable_dup = strdup (executable);
> +      if (executable_dup)
> +	{
> +	  free (*file_name);
> +	  *file_name = executable_dup;
> +	  return -1;
> +	}
> +    }
> +  return dwfl_build_id_find_elf (mod, userdata, modname, base, file_name, elfp);
> +}
> +
> +static Dwfl *
> +dwfl_offline (void)
> +{
> +  static char *debuginfo_path;
> +  static const Dwfl_Callbacks offline_callbacks =
> +    {
> +      .find_debuginfo = dwfl_standard_find_debuginfo,
> +      .debuginfo_path = &debuginfo_path,
> +
> +      .section_address = dwfl_offline_section_address,
> +
> +      /* We use this table for core files too.  */
> +      .find_elf = find_elf,
> +    };
> +  Dwfl *dwfl = dwfl_begin (&offline_callbacks);
> +  if (dwfl == NULL)
> +    error (2, 0, "dwfl_begin: %s", dwfl_errmsg (-1));
> +  return dwfl;
> +}

I believe you don't need these two functions and the executable var,
since you don't do cores.

> +  if (symname && strcmp (symname, "main") == 0)
> +    {
> +      kill (dwfl_pid (dwfl), SIGKILL);
> +      exit (0);
> +    }

Sneaky :)

> +  Dwfl *dwfl = pid_to_dwfl (pid);
> +  dwfl_getthreads (dwfl, thread_callback, NULL);
> +  error (1, 0, "dwfl_getthreads: %s", dwfl_errmsg (-1));

Might want to add a comment here, for people like me that don't
immediately realize you exit (0) from deep inside the
frame/thread_callback.

> diff --git a/tests/backtrace-subr.sh b/tests/backtrace-subr.sh
> +. $srcdir/test-subr.sh
> +
> +# Verify one of the backtraced threads contains function 'main'.
> +check_main()
> +{
> +  if grep -w main $1; then
> +    return
> +  fi
> +  echo >&2 $2: no main
> +  false
> +}

OK.

> +# Without proper ELF symbols resolution we could get inappropriate weak
> +# symbol "gsignal" with the same address as the correct symbol "raise".
> +# It was fixed by GIT commit 78dec228b3cfb2f9300cd0b682ebf416c9674c91 .
> +# [patch] Improve ELF symbols preference (global > weak)
> +# https://lists.fedorahosted.org/pipermail/elfutils-devel/2012-October/002624.html
> +check_gsignal()
> +{
> +  if ! grep -w gsignal $1; then
> +    return
> +  fi
> +  echo >&2 $2: found gsignal
> +  false
> +}

OK. Probably should have been its own test, but OK to include here.
Thanks for the reference explaining the issue.

> +# Verify the STDERR output does not contain unexpected errors.
> +# In some cases we cannot reliably find out we got behind _start as some
> +# operating system do not properly terminate CFI by undefined PC.
> +# Ignore it here as it is a bug of OS, not a bug of elfutils.
> +check_err()
> +{
> +  if [ $(egrep -v <$1 '/backtrace: dwfl_thread_getframes: (No DWARF information found|no matching address range)$' \
> +         | wc -c) \
> +       -eq 0 ]
> +  then
> +    return
> +  fi
> +  echo >&2 $2: neither empty nor just out of DWARF
> +  false
> +}

OK. But you might want the egrep on /backtrace(-[a-z]+)?: (see below).

> +check_all()
> +{
> +  bt=$1
> +  err=$2
> +  testname=$3
> +  check_main $bt $testname
> +  check_gsignal $bt $testname
> +  check_err $err $testname
> +}

OK.

> +check_unsupported()
> +{
> +  err=$1
> +  testname=$2
> +  if grep -q ': Unwinding not supported for this architecture$' $err; then
> +    echo >&2 $testname: arch not supported
> +    exit 77
> +  fi
> +}

OK.

> +check_core()
> +{
> +  arch=$1
> +  testfiles backtrace.$arch.{exec,core}
> +  tempfiles backtrace.$arch.{bt,err}
> +  echo ./backtrace ./backtrace.$arch.{exec,core}
> +  testrun ${abs_builddir}/backtrace ./backtrace.$arch.{exec,core} 1>backtrace.$arch.bt 2>backtrace.$arch.err || true
> +  cat backtrace.$arch.{bt,err}
> +  check_all backtrace.$arch.{bt,err} backtrace.$arch.core
> +}

OK.

Maybe you also want a check_native() (see below)

> diff --git a/tests/backtrace.c b/tests/backtrace.c
> +/* Test program for unwinding of frames.

See also some of my comments in my previous email. But since it is at
least somewhat simplified I think it is OK as test now.

> +static void
> +selfdump_callback (pid_t tid, unsigned frameno, Dwarf_Addr pc,
> +		   const char *symname, Dwfl *dwfl, void *data)
> +{
> +  pid_t check_tid = (intptr_t) data;
> +  bool is_x86_64 = check_tid < 0;
> +  if (is_x86_64)
> +    check_tid = -check_tid;
> +  static bool seen_main = false;
> +  if (symname && *symname == '.')
> +    symname++;

This seems new. I assume it is for the fake ppc function entry symbols.
Seems we should do this dot-prefix cleanup in dwfl_module_getsym () so
that all arches produce normal function symbol names.

> diff --git a/tests/cleanup-13.c b/tests/cleanup-13.c

Nice to have this. Thanks for including the origin URL.

> diff --git a/tests/run-backtrace-core-i386.sh b/tests/run-backtrace-core-i386.sh
> [...]
> +. $srcdir/backtrace-subr.sh
> +
> +check_core i386
> diff --git a/tests/run-backtrace-core-x86_64.sh b/tests/run-backtrace-core-x86_64.sh
> [...]
> +. $srcdir/backtrace-subr.sh
> +
> +check_core x86_64

Might want to have one run-backtrace-core.sh file with simply:
check_core i386
check_core x86_64

> diff --git a/tests/run-backtrace-data.sh b/tests/run-backtrace-data.sh
> +. $srcdir/test-subr.sh
> +
> +testrun ${abs_builddir}/backtrace-data

This might need the same treatment as run-backtrace-native.sh because it
is mostly a native test, checking against a native libc.so. So it might
need at least the check_err on some systems.

> diff --git a/tests/run-backtrace-dwarf.sh b/tests/run-backtrace-dwarf.sh
> +. $srcdir/test-subr.sh
> +
> +testrun ${abs_builddir}/backtrace-dwarf

OK.

> diff --git a/tests/run-backtrace-native-biarch.sh b/tests/run-backtrace-native-biarch.sh
> [...
> +if test -n "$ELFUTILS_DISABLE_BIARCH"; then
> +  exit 77
> +fi
> +
> +. $srcdir/backtrace-subr.sh
> +
> +child=backtrace-child-biarch
> +
> +# Backtrace live process.
> +# Do not abort on non-zero exit code due to some warnings of ./backtrace
> +# - see function check_err.
> +tempfiles $child.{bt,err}
> +(set +ex; testrun ${abs_builddir}/backtrace ${abs_builddir}/$child 1>$child.bt 2>$child.err; true)
> +cat $child.{bt,err}
> +check_unsupported $child.err $child
> +check_all $child.{bt,err} $child

OK.

> diff --git a/tests/run-backtrace-native-core-biarch.sh b/tests/run-backtrace-native-core-biarch.sh
> [...]
> +if test -n "$ELFUTILS_DISABLE_BIARCH"; then
> +  exit 77
> +fi
> +
> +. $srcdir/backtrace-subr.sh
> +
> +child=backtrace-child-biarch
> +
> +# Backtrace core file.
> +core="core.`ulimit -c unlimited; set +ex; testrun ${abs_builddir}/$child --gencore --run; true`"
> +# Do not abort on non-zero exit code due to some warnings of ./backtrace
> +# - see function check_err.
> +tempfiles $core{,.{bt,err}}
> +(set +ex; testrun ${abs_builddir}/backtrace ${abs_builddir}/$child $core 1>$core.bt 2>$core.err; true)
> +cat $core.{bt,err}
> +check_unsupported $core.err $child-$core
> +check_all $core.{bt,err} $child-$core

OK.

> diff --git a/tests/run-backtrace-native-core.sh b/tests/run-backtrace-native-core.sh
> [...]
> +. $srcdir/backtrace-subr.sh
> +
> +child=backtrace-child
> +
> +# Backtrace core file.
> +core="core.`ulimit -c unlimited; set +ex; testrun ${abs_builddir}/$child --gencore --run; true`"
> +# Do not abort on non-zero exit code due to some warnings of ./backtrace
> +# - see function check_err.
> +tempfiles $core{,.{bt,err}}
> +(set +ex; testrun ${abs_builddir}/backtrace ${abs_builddir}/$child $core 1>$core.bt 2>$core.err; true)
> +cat $core.{bt,err}
> +check_unsupported $core.err $child-$core
> +check_all $core.{bt,err} $child-$core

OK, but maybe wrap this in a check_native() in backtrace-subr.sh to
share with run-backtrace-data.sh and run-backtrace-child.sh?

> diff --git a/tests/run-backtrace-native.sh b/tests/run-backtrace-native.sh
> [...]
> +. $srcdir/backtrace-subr.sh
> +
> +child=backtrace-child
> +
> +# Backtrace live process.
> +# Do not abort on non-zero exit code due to some warnings of ./backtrace
> +# - see function check_err.
> +tempfiles $child.{bt,err}
> +(set +ex; testrun ${abs_builddir}/backtrace ${abs_builddir}/$child 1>$child.bt 2>$child.err; true)
> +cat $child.{bt,err}
> +check_unsupported $child.err $child
> +check_all $child.{bt,err} $child

OK, but maybe wrap this in a check_native() in backtrace-subr.sh?

Thanks,

Mark


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]