[PATCH 1/3] gdb: regenerate target-delegates.c

[PATCH 1/3] gdb: regenerate target-delegates.c

[Tankut Baris Aktemur]

I ran './make-target-delegates.py' and there as one minor difference,
where an older style declaration is converted to a newer
initialization style.  Add this change.
---
 gdb/target-delegates.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index c5540c366e4..a27b66df100 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -2291,9 +2291,9 @@ dummy_target::supports_set_thread_options (gdb_thread_options arg0)
 bool
 debug_target::supports_set_thread_options (gdb_thread_options arg0)
 {
-  bool result;
   gdb_printf (gdb_stdlog, "-> %s->supports_set_thread_options (...)\n", this->beneath ()->shortname ());
-  result = this->beneath ()->supports_set_thread_options (arg0);
+  bool result
+    = this->beneath ()->supports_set_thread_options (arg0);
   gdb_printf (gdb_stdlog, "<- %s->supports_set_thread_options (", this->beneath ()->shortname ());
   target_debug_print_gdb_thread_options (arg0);
   gdb_puts (") = ", gdb_stdlog);
-- 
2.34.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 2/3] gdb: refactor make-target-delegates.py's ARGTYPES

[Tankut Baris Aktemur]

Refactor the ARGTYPES regular expression in make-target-delegates.py
to eliminate '.*' for better control on what is matched.  This will be
helpful in the next patch where we recognize default argument values.
Also, simplify the "E" match group, for which the optional SYMBOL
becomes redundant because that case can be matched by the "T" group.

After applying this patch, running './make-target-delegates.py' does not
change anything in 'target-delegates.c'.
---
 gdb/make-target-delegates.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/gdb/make-target-delegates.py b/gdb/make-target-delegates.py
index 5bbe7c0b930..fd5f436a43d 100755
--- a/gdb/make-target-delegates.py
+++ b/gdb/make-target-delegates.py
@@ -73,17 +73,18 @@ METHOD = re.compile(
     + METHOD_TRAILER
 )
 
+# Space-separated symbols.
+CP_SYMBOLS = CP_SYMBOL + r"(\s+" + CP_SYMBOL + r")*"
+
 # Regular expression used to dissect argument types.
 ARGTYPES = re.compile(
     "^("
     + r"(?P<E>enum\s+"
     + SYMBOL
-    + r"\s*)("
-    + SYMBOL
-    + ")?"
-    + r"|(?P<T>.*(enum\s+)?"
-    + SYMBOL
-    + r".*(\s|\*|&))"
+    + r")"
+    + r"|(?P<T>"
+    + CP_SYMBOLS
+    + r"(\s|\*|&)+)"
     + SYMBOL
     + ")$"
 )
-- 
2.34.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 3/3] gdb: handle default argument in make-target-delegates.py

[Tankut Baris Aktemur]

The regular expression to dissect argument types in
make-target-delegates.py does not recognize default argument values.
For example, in target.h, we have

   virtual void rcmd (const char *command, struct ui_file *output)
     TARGET_DEFAULT_FUNC (default_rcmd);
   virtual const char *pid_to_exec_file (int pid)
     TARGET_DEFAULT_RETURN (NULL);

Suppose we change these to the following where we define default
values for the arguments.  The example for the default argument
of "pid" is deliberately chosen to contain a space character.

   virtual void rcmd (const char *command=0, struct ui_file *output = nullptr)
     TARGET_DEFAULT_FUNC (default_rcmd);
   virtual const char *pid_to_exec_file (int pid=sizeof n)
     TARGET_DEFAULT_RETURN (NULL);

After generating the target-delegates.c file via make-target-delegates.py
we obtain invalid code:

   void rcmd (const char *command=0 arg0, struct ui_file *output = nullptr arg1) override;
   const char *pid_to_exec_file (int pid=sizeof n arg0) override;
   ...
   void
   debug_target::rcmd (const char *command=0 arg0, struct ui_file *output = nullptr arg1)
   {
     ...
     target_debug_print_const_char_pcommand=0 (arg0);
     ...
     target_debug_print_struct_ui_file_poutput_=_nullptr (arg1);
   }
   ...
   const char *
   debug_target::pid_to_exec_file (int pid=sizeof n arg0)
   {
     ...
     target_debug_print_int_pid=sizeof_n (arg0);
     ...
   }

When the fix in this patch is applied, the default argument values
are correctly detected and we obtain the same generated result for
target-delegates.c, which is

   void rcmd (const char *arg0, struct ui_file *arg1) override;
   const char *pid_to_exec_file (int arg0) override;
   ...
   void
   debug_target::rcmd (const char *arg0, struct ui_file *arg1)
   {
     ...
     target_debug_print_const_char_p (arg0);
     ...
     target_debug_print_struct_ui_file_p (arg1);
   }
   ...
   const char *
   debug_target::pid_to_exec_file (int arg0)
   {
     ...
     target_debug_print_int (arg0);
     ...
   }

Currently, target.h does not contain any arguments with default
values.  The goal of this patch is to address potential future and
downstream cases; we (Intel) have cases with default argument values.
---
 gdb/make-target-delegates.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/make-target-delegates.py b/gdb/make-target-delegates.py
index fd5f436a43d..2758f900c9b 100755
--- a/gdb/make-target-delegates.py
+++ b/gdb/make-target-delegates.py
@@ -76,6 +76,8 @@ METHOD = re.compile(
 # Space-separated symbols.
 CP_SYMBOLS = CP_SYMBOL + r"(\s+" + CP_SYMBOL + r")*"
 
+DEFAULT_ARG_VALUE = r"(\s*\=\s*[^\s].*)?"
+
 # Regular expression used to dissect argument types.
 ARGTYPES = re.compile(
     "^("
@@ -86,6 +88,7 @@ ARGTYPES = re.compile(
     + CP_SYMBOLS
     + r"(\s|\*|&)+)"
     + SYMBOL
+    + DEFAULT_ARG_VALUE
     + ")$"
 )
 
-- 
2.34.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/3] gdb: regenerate target-delegates.c

[Pedro Alves]

On 2023-11-14 09:41, Tankut Baris Aktemur wrote:
> I ran './make-target-delegates.py' and there as one minor difference,
> where an older style declaration is converted to a newer
> initialization style.  Add this change.
> ---
>  gdb/target-delegates.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
> index c5540c366e4..a27b66df100 100644
> --- a/gdb/target-delegates.c
> +++ b/gdb/target-delegates.c
> @@ -2291,9 +2291,9 @@ dummy_target::supports_set_thread_options (gdb_thread_options arg0)
>  bool
>  debug_target::supports_set_thread_options (gdb_thread_options arg0)
>  {
> -  bool result;
>    gdb_printf (gdb_stdlog, "-> %s->supports_set_thread_options (...)\n", this->beneath ()->shortname ());
> -  result = this->beneath ()->supports_set_thread_options (arg0);
> +  bool result
> +    = this->beneath ()->supports_set_thread_options (arg0);
>    gdb_printf (gdb_stdlog, "<- %s->supports_set_thread_options (", this->beneath ()->shortname ());
>    target_debug_print_gdb_thread_options (arg0);
>    gdb_puts (") = ", gdb_stdlog);


Whoops, my fault, missed the need to regenerate before pushing.

Approved-By: Pedro Alves <pedro@palves.net>

Re: [PATCH 3/3] gdb: handle default argument in make-target-delegates.py

[Pedro Alves]

On 2023-11-14 09:41, Tankut Baris Aktemur wrote:

> Currently, target.h does not contain any arguments with default
> values.  The goal of this patch is to address potential future and
> downstream cases; we (Intel) have cases with default argument values.

Hmm, but default arguments in virtual methods is generally considered a bad idea,
because default arguments are a static property which doesn't propagate when
you override a method.  Like:

#include <stdio.h>

struct A
{
  virtual void func (int arg = 1) { printf ("%d\n", arg); }
};

struct B : A
{
  virtual void func (int arg = 2) { printf ("%d\n", arg); }
};

struct C : A
{
  virtual void func (int arg) { printf ("%d\n", arg); }
};

int
main ()
{
   B *b = new B ();
   A *a = b;
   a->func ();  // prints "1".
   b->func ();  // prints "2".

   C *c = new C ();
   c->func ();  // doesn't even compile.
}

Instead, it's better practice to put the default argument in a non-virtual method that
then calls the virtual method.  We don't have those in target_ops, but we have instead
the global target_foo wrapper functions.  So instead of putting the default argument here:

 -   virtual void rcmd (const char *command=0, struct ui_file *output)
 +   virtual void rcmd (const char *command=0, struct ui_file *output = nullptr)
       TARGET_DEFAULT_FUNC (default_rcmd);

You should put the default here:

 -extern void target_rcmd (const char *command, struct ui_file *outbuf);
 +extern void target_rcmd (const char *command, struct ui_file *outbuf = nullptr);

RE: [PATCH 3/3] gdb: handle default argument in make-target-delegates.py

[Aktemur, Tankut Baris]

On Tuesday, November 14, 2023 12:31 PM, Pedro Alves wrote:
> On 2023-11-14 09:41, Tankut Baris Aktemur wrote:
> 
> > Currently, target.h does not contain any arguments with default
> > values.  The goal of this patch is to address potential future and
> > downstream cases; we (Intel) have cases with default argument values.
> 
> Hmm, but default arguments in virtual methods is generally considered a bad idea,
> because default arguments are a static property which doesn't propagate when
> you override a method.  Like:

This is an important point that I missed.  Thanks.

This would make Patch 3/3 obsolete.  I'd still like to keep
Patch 2/3 for review (I'd update its commit message, though).
I think Patch 2/3 makes the regexp easier to read and has a value.

> #include <stdio.h>
> 
> struct A
> {
>   virtual void func (int arg = 1) { printf ("%d\n", arg); }
> };
> 
> struct B : A
> {
>   virtual void func (int arg = 2) { printf ("%d\n", arg); }
> };
> 
> struct C : A
> {
>   virtual void func (int arg) { printf ("%d\n", arg); }
> };
> 
> int
> main ()
> {
>    B *b = new B ();
>    A *a = b;
>    a->func ();  // prints "1".
>    b->func ();  // prints "2".
> 
>    C *c = new C ();
>    c->func ();  // doesn't even compile.
> }
> 
> Instead, it's better practice to put the default argument in a non-virtual method
> that
> then calls the virtual method.  We don't have those in target_ops, but we have
> instead
> the global target_foo wrapper functions.  So instead of putting the default argument
> here:
> 
>  -   virtual void rcmd (const char *command=0, struct ui_file *output)
>  +   virtual void rcmd (const char *command=0, struct ui_file *output = nullptr)
>        TARGET_DEFAULT_FUNC (default_rcmd);
> 
> You should put the default here:
> 
>  -extern void target_rcmd (const char *command, struct ui_file *outbuf);
>  +extern void target_rcmd (const char *command, struct ui_file *outbuf = nullptr);

These wrappers at the GDB side are global functions and they make
calling target ops easier.  E.g.:

  target_foo (...);

instead of

  current_inferior ()->top_target ()->foo (...);

Now comes my side question:

At the gdbserver side, we have macros instead.  E.g.:

  #define target_create_inferior(program, program_args)   \
    the_target->create_inferior (program, program_args)

These macros look like they remained from the C time and not
every target op consistently has a wrapper macro.  A macro
is not used consistently even when it exists.

Is there a general direction about them, like

1. They shall stay and be used; we shall fix the lack of consistency.

2. They shall be converted into global wrapper functions; we shall
   use the wrappers consistently.

3. They shall be removed and replaced by the actual method call.
   E.g.:
     the_target->foo (...);
   instead of
     target_foo (...);

(FWIW, option #3 makes the most sense to me.)

Thanks,
-Baris




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 3/3] gdb: handle default argument in make-target-delegates.py

[Pedro Alves]

On 2023-11-14 12:48, Aktemur, Tankut Baris wrote:
> On Tuesday, November 14, 2023 12:31 PM, Pedro Alves wrote:
>> On 2023-11-14 09:41, Tankut Baris Aktemur wrote:
>>
>>> Currently, target.h does not contain any arguments with default
>>> values.  The goal of this patch is to address potential future and
>>> downstream cases; we (Intel) have cases with default argument values.
>>
>> Hmm, but default arguments in virtual methods is generally considered a bad idea,
>> because default arguments are a static property which doesn't propagate when
>> you override a method.  Like:
> 
> This is an important point that I missed.  Thanks.
> 

> This would make Patch 3/3 obsolete.  I'd still like to keep
> Patch 2/3 for review (I'd update its commit message, though).
> I think Patch 2/3 makes the regexp easier to read and has a value.

I will take a look.


> At the gdbserver side, we have macros instead.  E.g.:
> 
>   #define target_create_inferior(program, program_args)   \
>     the_target->create_inferior (program, program_args)
> 
> These macros look like they remained from the C time and not
> every target op consistently has a wrapper macro.  A macro
> is not used consistently even when it exists.
> 
> Is there a general direction about them, like
> 
> 1. They shall stay and be used; we shall fix the lack of consistency.
> 
> 2. They shall be converted into global wrapper functions; we shall
>    use the wrappers consistently.
> 
> 3. They shall be removed and replaced by the actual method call.
>    E.g.:
>      the_target->foo (...);
>    instead of
>      target_foo (...);
> 
> (FWIW, option #3 makes the most sense to me.)

I'd go with #2, for a few reasons:

  - so you have a place to put the default arguments.
  - so that we have some consistency with the GDB side.  It's even the case that some
    target_foo wrapper methods are shared between GDB and GDBserver -- see gdb/target/target.h.
  - it'd help if we decided to add a target stack concept to GDBserver too.

Re: [PATCH 2/3] gdb: refactor make-target-delegates.py's ARGTYPES

[Pedro Alves]

Hi Baris,

On 2023-11-14 09:41, Tankut Baris Aktemur wrote:
> Refactor the ARGTYPES regular expression in make-target-delegates.py
> to eliminate '.*' for better control on what is matched.  This will be
> helpful in the next patch where we recognize default argument values.
> Also, simplify the "E" match group, for which the optional SYMBOL
> becomes redundant because that case can be matched by the "T" group.
> 
> After applying this patch, running './make-target-delegates.py' does not
> change anything in 'target-delegates.c'.

With the tweak to the commit log to not mention default arguments:

  Approved-By: Pedro Alves <pedro@palves.net>

Re: [PATCH 3/3] gdb: handle default argument in make-target-delegates.py

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> Hmm, but default arguments in virtual methods is generally
Pedro> considered a bad idea, because default arguments are a static
Pedro> property which doesn't propagate when you override a method.

I wonder if we have any of these already, and/or if there's a compiler
warning we could enable.

Tom

Re: [PATCH 1/3] gdb: regenerate target-delegates.c

[Tom Tromey]

>>>>> "Tankut" == Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:

Tankut> I ran './make-target-delegates.py' and there as one minor difference,
Tankut> where an older style declaration is converted to a newer
Tankut> initialization style.  Add this change.

FAOD, I think it's fine to treat regenerations like this as obvious.

I suppose the exception would be if you have some reason to believe the
change to one of the inputs was incorrect, so regenerating would
introduce a bug.  But, I think that's pretty rare.

thanks,
Tom

RE: [PATCH 3/3] gdb: handle default argument in make-target-delegates.py

[Aktemur, Tankut Baris]

> >>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> Hmm, but default arguments in virtual methods is generally
> Pedro> considered a bad idea, because default arguments are a static
> Pedro> property which doesn't propagate when you override a method.
> 
> I wonder if we have any of these already, and/or if there's a compiler
> warning we could enable.

I couldn't find such a warning option in GCC or Clang:

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
https://clang.llvm.org/docs/DiagnosticsReference.html

-Baris


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 3/3] gdb: handle default argument in make-target-delegates.py

[Tom Tromey]

>>>>> Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> writes:

>> I wonder if we have any of these already, and/or if there's a compiler
>> warning we could enable.

> I couldn't find such a warning option in GCC or Clang:

> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
> https://clang.llvm.org/docs/DiagnosticsReference.html

Thanks for looking.

Tom