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

[Mark Wielaard <mjw at redhat dot com>]

On Sun, 2013-12-01 at 18:50 +0100, Jan Kratochvil wrote:
> It seems ready to me, there were not many/any parts for further discussion

Yes. I didn't have anything that looked like it must be changed. However
you resolve any remaining issues is fine with me.

> > 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.
> 
> Primarily it probably works because:
> 
> dwfl_report_begin_add (Dwfl *dwfl __attribute__ ((unused)))
> { 
>   /* The lookup table will be cleared on demand, there is nothing we need
>      to do here.  */
> } 
> 
> But good you have found it.

Yes, it works currently. But I rather not have other people rely on
being able to mix report calls with other dwfl calls. So thanks for
adding the comment that it violates the libdwfl interface. Now when
people see this code and do something similar they have at least been
warned it might break in the future.

> > > 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
> 
> Sorry but this seems a bit inconsistent to me.  First you wanted to split the
> single backtrace testfile.  Now you want to merge some of the testfiles back.
> 
> If one should really see different parts of elfutils code to have effect on
> different testfiles then each arch really should be separate.  i386 vs. x86_64
> test tests different backend implementation.
> 
> So please justify why some testcases should be merged and some should be
> separate.  I do not see the rule/reason.

There is some middle ground between one giant testcase that does
everything and lots of little test files that only differ in one line. I
am happy you split the tests a bit and added comments explaining what
which part does. But I think you overdone it in this particular case.
Because these test the same thing. A backtrace over a core file. I would
have grouped those together. But if you feel this split is easier then
go with it. It just isn't how I would have done it.

> > > 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.
> 
> I do not think so.  The reason for check_err is that ./backtrace tries to
> unwind through main() also _start and it is difficult to find the termination
> there.
> 
> Contrary to it run-backtrace-data.sh stops already at main() so it is safe.

Maybe I am reading backtrace-data.c wrongly. But I don't see it stop for
main. If it would then indeed it doesn't seem necessary.
backtrace-dwarf.c does seem to have this "stop at main" logic though. So
maybe we are talking about different tests?

> > > 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?
> 
> There is no run-backtrace-child.sh.  run-backtrace-data.sh is trivial and I do
> not see there an opportunity for code merging.
> 
> I have merged the normal+biarch code into check_native() and
> check_native_core().
> 
> Merging check_native() and check_native_core() IMO does not make sense, they
> are already mostly merged thanks to check_unsupported() and check_all().

Yes, sorry. I confused some tests.

Thanks,

Mark