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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Mark,

updates are in:
jankratochvil/unwindx86


On Fri, 29 Nov 2013 17:02:26 +0100, Mark Wielaard wrote:
> But it should still be clear what they are testing. If they fail after
> some code update someone still needs to be able to debug them. And since
> this is for a new feature people will take a look at them just to see
> how the new feature is used (so then, at least some comment saying
> whether or not the usage is normal or exceptional/strange - just to test
> a corner case, would be good).

OK.


> OK. I understand that it is used to make sure they aren't
> omitted/inlined completely. But why are they called in the first place?

They are called just so that they do not have to be marked also with
__attribute__((unused)).


> And why are they in the test source code?

Added there:
  /* These dummy* functions are there so that each of their surrounding
     functions has some unrelated code around.  The purpose of some of the
     tests is verify unwinding the very first / after the very last instruction 
     does not inappropriately slip into the unrelated code around.  */


> >    --run is always required.
[...]
> It doesn't explain why --run is always required, that seems odd to me.

So that user randomly running binaries in tests/ by hand does not get confused
by behavior of a binary which is not intended to be run outside of ./backtrace
parent.

I have removed the --run option everywhere now.


> > > (This is really confusing, why the difference between x86_64 and the
> > > rest?)
> > 
> > 1. Changing inferior PC is arch-dependent (PTRACE_POKEUSER on x86_64).
> > 2. Some of the arch-independent tests like this one I find sufficient to run
> >    only on one arch.  And everyone has x86_64.
> > 3. I found it good enough.  One could port it too all the other archs but see
> >    item 2.
> 
> OK. I don't think it necessarily needs to be ported to each arch, but
> having enough documentation in the test to explain what is arch
> dependent and why is good.

Added to the initial tests/backtrace-child.c comment:
   On x86_64 only:
     [...]
     The tested functionality is arch-independent but the code reproducing it 
     has to be arch-specific.


> I am not really clear on why the changing the PC is needed though.

It is part of the test how to test unwinding when asynchronous signal happened
during execution of the very first instruction of a function.  There is the
risk of accidentally using unwind info for the previous (PC-located before,
with lower PC addresses) function instead.

Easier method how to test it is welcome.


> > > There is also an assumption that if the process is build on x86_64 and
> > > ptraced and a SIGUSR1 signal is caught by the tracee that the PC will be
> > > adjusted to call the sigusr2 handler (this is also makes this test case
> > > somewhat hard to follow IMHO).
> > 
> > Yes.
> 
> But why is that? What does it add to the test?

See the paragraph above.


> > I have removed the .plt testing code now.
> 
> I don't really understand why it was there in the first place, so cannot
> tell whether that is good or bad.

I have already explained that it was a way how to test non-trivial DWARF
expressions.

$ readelf -wf /bin/bash|grep DW_CFA_def_cfa_expression
  DW_CFA_def_cfa_expression (DW_OP_breg7 (rsp): 8; DW_OP_breg16 (rip): 0; DW_OP_lit15; DW_OP_and; DW_OP_lit11; DW_OP_ge; DW_OP_lit3; DW_OP_shl; DW_OP_plus)

This line is from .plt CFI.  In normal executables DWARF expression is never
used anywhere else.

But as tests/cleanup-13.c is now much more thorough test I could drop the .plt
DWARF expression exploit.

Originally I expected integrating cleanup-13.c into the elfutils testsuite
will be too complicated.  In the end it was easier than the original exploit
of .plt but I did not know that initially.


> I just didn't understand
> what precisely the .plt part was testing and it looked very complicated.

I hope I have answered it above.


> So I couldn't really say whether the complexity was worth it or not. If
> you think the thing you wanted to test by including the .plt testing is
> covered now by the new test that is good.

Yes.


> > I have put there:
> >       // Find inferior symbol named "jmp".
> 
> OK. But that doesn't really explain what it is used for and why it is
> x86_64 specific.

It is a part of the test of the asynchronous signal happening during the very
first instruction of a function.

I have added new 3 lines to the initial comment of tests/backtrace-child.c:

    On x86_64 only:
      PC will get changed to function 'jmp' by backtrace.c function
      prepare_thread.  Then SIGUSR2 will be signalled to backtrace-child
      which will invoke function sigusr2.
+     This is all done so that signal interrupts execution of the very first
+     instruction of a function.  Properly handled unwind should not slip into
+     the previous unrelated function.
      The tested functionality is arch-independent but the code reproducing it 
      has to be arch-specific.
    On non-x86_64:
      sigusr2 gets called by normal function call from function stdarg.


> > > This really could use some documentation or better argument parsing IMHO.
> > > Why not have simpler arguments:
> > > 
> > > --backtrace-child
> > > --backtrace-pid <pid>
> > > --backtrace-core <corefile>
> > > --backtrace-exe <exefile>
> > > --backtrace-exe-core <exefile> <corefile>
[...]
> Not really, because I don't see the logic of the current argument
> parsing. But at least the command line syntax is now documented. It is
> hard to see this as some general backtracing tool since it has so many
> x86_64 specific hacks in it.

So there is now argp option:
      --backtrace-exec=EXEC  Run executable

but the other combinations are accessed via standard -e and --core.
Is it OK this way?  All theoptions above would duplicate dwfl_standard_argp ().

So far in most of the cases ./backtrace only displayed the backtrace but not
verified each of the frames matches the expections.  This has been fixed now.


> > Without 'true' the script will fail due to 'set -e'.
> > 
> > After program dumps a core its exit code is not zero.  'true' overrides it
> > there.
> 
> Aha. How inconvenient. So you cannot see the difference between whether
> the core file was actually generated or not because of some failure?

In such case the backtrace frames will not match the expectation.


Updated patch will be sent only after the real review in the other mail.


Thanks,
Jan