[Intel-gfx] [igt-dev] [PATH i-g-t 2/2] core: Show backtrace from igt_skip_on_simulation

[Daniel Vetter]

On Fri, Sep 14, 2018 at 10:49:47AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-09-14 10:46:25)
> > On Fri, Sep 14, 2018 at 10:19:29AM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 14/09/2018 10:12, Daniel Vetter wrote:
> > > > On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote:
> > > > > From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > > > 
> > > > > igt_skip_on_simulation is called both directly from tests but also from
> > > > > library helpers. In the latter case especially the logged caller name is
> > > > > useless since it is always the helper itself. What we instead want to know
> > > > > is who is the caller.
> > > > > 
> > > > > Trivial approach would be to move the helper to a header as static inline,
> > > > > but due the longjmp in it it can never be inlined. Alternative option is
> > > > > to print a backtrace from it.
> > > > > 
> > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > > > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg at intel.com>
> > > > > ---
> > > > >   lib/igt_core.c | 20 ++++++++++++++++----
> > > > >   1 file changed, 16 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > > > index 23bb858fd886..990abc5a36b3 100644
> > > > > --- a/lib/igt_core.c
> > > > > +++ b/lib/igt_core.c
> > > > > @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void)
> > > > >    */
> > > > >   void igt_skip_on_simulation(void)
> > > > >   {
> > > > > + bool in_simulation;
> > > > > +
> > > > >           if (igt_only_list_subtests())
> > > > >                   return;
> > > > > + in_simulation = igt_run_in_simulation();
> > > > > +
> > > > >           if (!igt_can_fail()) {
> > > > > -         igt_fixture
> > > > > -                 igt_require(!igt_run_in_simulation());
> > > > > - } else
> > > > > -         igt_require(!igt_run_in_simulation());
> > > > > +         igt_fixture {
> > > > > +                 if (in_simulation) {
> > > > > +                         print_backtrace();
> > > > > +                         igt_require(!in_simulation);
> > > > > +                 }
> > > > > +         }
> > > > > + } else {
> > > > > +         if (in_simulation) {
> > > > > +                 print_backtrace();
> > > > > +                 igt_require(!in_simulation);
> > > > 
> > > > Hm, why don't we go right ahead and push this into igt_skip()? There's a
> > > > pile of other igt_require, and we tend to push a lot of these into the
> > > > library. So they have all the same problem.
> > > 
> > > Maybe.. I wasn't 100% this was a good idea to start with, or in another
> > > words, that other people would consider it a problem. Since the downside is
> > > test output gets more verbose on skips, or some could say more noisy.
> > > 
> > > So I basically floated the patch to see if it will provoke some responses.
> > > :)
> > 
> > We have the backtrace already in igt_fail, makes total sense to add it to
> > igt_skip too. Has my ack at least.
> 
> Skip is rather more frequent than fail, and more often than not,
> expected. So I am not entirely enjoying the prospect of a lot more noise,
> the requirement message was supposed to be sufficient to understand why
> we skipped. Maybe enforce that we have no igt_skip without a message?

Hm yeah, adding a const char * to igt_skip_on_simulation that explains why
we're skipping could be the pretty solution here. Would also serve as some
self-documentation.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch