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

  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(+)
  

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

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
  

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.
  

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

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

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