RE: [PATCH 1/1] gdb: Add functionality to disable test for specific architecture

["Rohr, Stephan" <stephan.rohr@intel.com>]

Hi Andrew,

Thanks for you feedback.  Please see my answers below.

Thanks
Stephan

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Thursday, February 23, 2023 6:51 PM
> To: Rohr, Stephan <stephan.rohr@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb: Add functionality to disable test for specific
> architecture
> 
> Stephan Rohr via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > From: "Rohr, Stephan" <stephan.rohr@intel.com>
> >
> > A set of not supported architectures can be provided to the
> > 'register_test_for_earch_arch' function such that this test
> > is skipped for all architectures contained in this set.
> 
> Generally we avoid having unused code in upstream.  That doesn't mean
> this patch couldn't go in, but I think you'd need to make a compelling
> argument for it in the commit message.
> 

We have a few self-tests where some tests pass whilst other fails for
our internal architecture.  I understand this use case is currently not
relevant for the GDB master.  I created this patch to prepare for our
upstreaming work and it might also become relevant for others.

I understand if the recommendation is to postpone this patch until
there is a real need on the master.  I'd appreciate additional feedback.

> Also, I'd be interested to understand more why this is needed.  The
> current skip_arch seems to cover a set of architectures that GDB just
> doesn't support, so we wouldn't expect any tests to work.
> 

I agree here, the current skip_arch implementation allows to skip all
tests for architectures that are not supported by GDB.  However, if some
tests are supported whilst others are not, this use case is currently not
supported by GDB.

> This change would seem to indicate you have an architecture for which
> some tests pass, but others don't.  Yet you feel that's not a bug in
> either the test, or how GDB handles the architecture.  It would be great
> to give some examples of this in the commit message.
> 

Your understanding is correct, we have some self-tests passing whilst others
are known to fail.  I will update the commit message is there is a consent on
the acceptance of the patch.

> Thanks,
> Andrew
> 
> 
> 
> 
> > ---
> >  gdb/selftest-arch.c | 11 ++++++++---
> >  gdb/selftest-arch.h |  5 ++++-
> >  2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/gdb/selftest-arch.c b/gdb/selftest-arch.c
> > index 691c40c14d7..d8fe49a1859 100644
> > --- a/gdb/selftest-arch.c
> > +++ b/gdb/selftest-arch.c
> > @@ -47,7 +47,8 @@ static bool skip_arch (const char *arch)
> >
> >  static std::vector<selftest>
> >  foreach_arch_test_generator (const std::string &name,
> > -			     self_test_foreach_arch_function *function)
> > +			     self_test_foreach_arch_function *function,
> > +			     const std::set<std::string> &disabled_arch)
> >  {
> >    std::vector<selftest> tests;
> >    std::vector<const char *> arches = gdbarch_printable_names ();
> > @@ -57,6 +58,9 @@ foreach_arch_test_generator (const std::string
> &name,
> >        if (skip_arch (arch))
> >  	continue;
> >
> > +      if (disabled_arch.count (arch) > 0)
> > +	continue;
> > +
> >        struct gdbarch_info info;
> >        info.bfd_arch_info = bfd_scan_arch (arch);
> >        info.osabi = GDB_OSABI_NONE;
> > @@ -96,11 +100,12 @@ foreach_arch_test_generator (const std::string
> &name,
> >
> >  void
> >  register_test_foreach_arch (const std::string &name,
> > -			    self_test_foreach_arch_function *function)
> > +			    self_test_foreach_arch_function *function,
> > +			    const std::set<std::string> &disabled_arch)
> >  {
> >    add_lazy_generator ([=] ()
> >  		      {
> > -		        return foreach_arch_test_generator (name, function);
> > +			return foreach_arch_test_generator (name,
> function, disabled_arch);
> >  		      });
> >  }
> >
> > diff --git a/gdb/selftest-arch.h b/gdb/selftest-arch.h
> > index a925e5a00dc..70b7f5224f8 100644
> > --- a/gdb/selftest-arch.h
> > +++ b/gdb/selftest-arch.h
> > @@ -19,6 +19,8 @@
> >  #ifndef SELFTEST_ARCH_H
> >  #define SELFTEST_ARCH_H
> >
> > +#include <set>
> > +
> >  typedef void self_test_foreach_arch_function (struct gdbarch *);
> >
> >  namespace selftests
> > @@ -28,7 +30,8 @@ namespace selftests
> >
> >  extern void
> >    register_test_foreach_arch (const std::string &name,
> > -			      self_test_foreach_arch_function *function);
> > +			      self_test_foreach_arch_function *function,
> > +			      const std::set<std::string> &disabled_arch = {});
> >  }
> >
> >  #endif /* SELFTEST_ARCH_H */
> > --
> > 2.25.1
> >
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> > Chairperson of the Supervisory Board: Nicole Lau
> > Registered Office: Munich
> > Commercial Register: Amtsgericht Muenchen HRB 186928

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928