[PATCH 0/1] Add functionality to disable test for specific architecture

[PATCH 0/1] Add functionality to disable test for specific architecture

[Stephan Rohr]

From: "Rohr, Stephan" <stephan.rohr@intel.com>

Hi all,

the proposed patch adds functionality to disable a test for an architecture
as specified by the user.  The current 'register_test_foreach_arch'
implementation skips all tests if the architecture is listed inside the
'skip_arch' function.  However, if the user wants to skip a limited set of
tests for a given architecture, this is currently not supported by the
interface.  The proposed patch extends the 'register_test_foreach_arch'
interface and allows the user to provide a set of architectures for which the
test shall not be executed, e.g.,

  selftests::register_test_foreach_arch ("test_name",
                                         function_name,
                                         std::set<std::string> ({"arch"}));


I tested this patch with the following targets:
  * unix
  * unix/-m32
  * native-gdbserver
  * native-extended-gdbserver

No regressions are identified.

Rohr, Stephan (1):
  gdb: Add functionality to disable test for specific architecture

 gdb/selftest-arch.c | 11 ++++++++---
 gdb/selftest-arch.h |  5 ++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

-- 
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

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

[Stephan Rohr]

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.
---
 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

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

[Andrew Burgess]

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.

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.

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.

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

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

[Rohr, Stephan]

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