[3/3] gdb: handle default argument in make-target-delegates.py
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
Commit Message
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(+)
Comments
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
@@ -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
+ ")$"
)