[00/40] C++ify target_ops, toward multi-target

Message ID 20180414190953.24481-1-palves@redhat.com
Headers show
Series
  • C++ify target_ops, toward multi-target
Related show

Message

Pedro Alves April 14, 2018, 7:09 p.m.
Here's another chunk from the multi-target branch.

This converts target_ops to a C++ class with virtual methods.

The reason this is part of the multi-target effort is that with
multi-target, we will want to be able to instantiate the remote, core,
etc. targets more than once at the same time.  In order to get there,
there are globals associated with these targets that will naturally
need to be made per-instance.  Perhaps naively, one of the first
things I did in the branch was to go ahead and convert target_ops to a
proper C++ class with virtual methods, thinking about how a C++ class
makes it easier to associate data with each instance, and also
thinking about how the current target_ops is really more the vtable
than the object, and it is preferable to avoid duplicating the whole
vtable in memory for each target instance...  In the end, I'm
convinced this paid off.

The hurdle is that doing this requires touching _every_ target in the
tree.  There's no partial transition possible.  I've worked on the
branch sitting on top of a version of this patch that only did a
partial transition, ignoring many native target_ops instances, but in
order to continue with upstreaming the multi-target work, I converted
all targets now, the best I could.  There are certainly many of native
targets here that I have no way to test.  My hope with posting this a
bit early (I'm missing some ChangeLogs) is that maybe someone will be
able to lend a hand with testing on their favorite port while I finish
that up and work through any review comments as they come.

There are actually three across-all-targets changes in this series.

  - target_ops function pointers -> virtual methods
  - target_ops use bool throughout
  - target_ops::open -> target factories (the last patch in the series)

I'm sending it all together thinking that that might save testing
rounds to those willing to lend a hand with testing.

And it goes without saying, but, testing help would be really
appreciated!

For testing and review convenience, I've pushed this to the
users/palves/target_ops-cxx branch, along with the few other related
patches that I posted this week that haven't been merged to master
yet.

The multi-target work will require at least one more round of touching
prototypes of some target methods, but I think that will be a smaller
patch (once I manage to split it cleanly from the rest of the branch).

After this, the main multi-target changes will be (in no particular
order):

 - per-inferior thread lists
 - a target stack per-inferior
 - use thread and inferior pointers more throughout instead of ptid_t
 - getting rid of more global state in the core and remote targets
 - actually instantiate multiple copies of core and remote targets
 - user interface, documentation, tests, etc.

Though that's still around 150 patches of fixes of fixes in the branch
to clean up, so it'll take a bit to get there.  Meanwhile, I'd
appreciate getting this chunk in, since it's a large burden to
maintain it off trunk.

Pedro Alves (40):
  Convert struct target_ops to C++
  make-target-delegates: line break between return type and function
    name
  target_ops/C++: exec target
  target_ops/C++: core target
  target_ops/C++: ctf/tfile targets
  target_ops/C++: spu-multiarch
  target_ops/C++: ravenscar-thread
  target_ops/C++: bsd-uthread
  target_ops/C++: bfd-target
  target_ops/C++: record targets
  target_ops/C++: remote/extended-remote targets
  target_ops/C++: target remote-sim
  target_ops/C++: GNU/Linux + x86/AMD64
  target_ops/C++: PPC/PPC64 GNU/Linux
  target_ops/C++: Solaris/procfs
  target_ops/C++: Windows targets
  target_ops/C++: macOS/Darwin target
  target_ops/C++: linux_trad_target, MIPS and Alpha GNU/Linux
  target_ops/C++: AIX target
  target_ops/C++: ARM GNU/Linux
  target_ops/C++: Aarch64 GNU/Linux
  target_ops/C++: HP-PA GNU/Linux
  target_ops/C++: IA-64 GNU/Linux
  target_ops/C++: m32r GNU/Linux
  target_ops/C++: m68k GNU/Linux
  target_ops/C++: s390 GNU/Linux
  target_ops/C++: SPARC GNU/Linux
  target_ops/C++: SPU/Linux
  target_ops/C++: Tile-Gx GNU/Linux
  target_ops/C++: Xtensa GNU/Linux
  target_ops/C++: Base FreeBSD target
  target_ops/C++: Generic i386/AMD64 BSD targets
  target_ops/C++: The rest of the BSD targets
  target_ops/C++: bsd_kvm_add_target, BSD libkvm target
  target_ops/C++: NTO/QNX, nto-procfs.c
  target_ops/C++: go32/DJGPP
  target_ops/C++: The Hurd
  target_ops: Use bool throughout
  linux_nat_target: More low methods
  target factories, target open and multiple instances of targets

 gdb/aarch64-fbsd-nat.c                             |   27 +-
 gdb/aarch64-linux-nat.c                            |  227 +-
 gdb/aix-thread.c                                   |  163 +-
 gdb/alpha-bsd-nat.c                                |   25 +-
 gdb/alpha-linux-nat.c                              |   17 +-
 gdb/amd64-bsd-nat.c                                |   24 +-
 gdb/amd64-bsd-nat.h                                |   44 +
 gdb/amd64-fbsd-nat.c                               |   26 +-
 gdb/amd64-linux-nat.c                              |   53 +-
 gdb/amd64-nat.h                                    |    5 -
 gdb/amd64-nbsd-nat.c                               |   10 +-
 gdb/amd64-obsd-nat.c                               |    6 +-
 gdb/arm-fbsd-nat.c                                 |   35 +-
 gdb/arm-linux-nat.c                                |  185 +-
 gdb/arm-nbsd-nat.c                                 |   27 +-
 gdb/auxv.c                                         |   13 +-
 gdb/auxv.h                                         |    3 +-
 gdb/avr-tdep.c                                     |    4 +-
 gdb/bfd-target.c                                   |  105 +-
 gdb/breakpoint.c                                   |    9 +-
 gdb/bsd-kvm.c                                      |  104 +-
 gdb/bsd-uthread.c                                  |  139 +-
 gdb/corefile.c                                     |   16 +-
 gdb/corelow.c                                      |  204 +-
 gdb/ctf.c                                          |  103 +-
 gdb/darwin-nat.c                                   |  146 +-
 gdb/darwin-nat.h                                   |   46 +-
 gdb/elfread.c                                      |   10 +-
 gdb/eval.c                                         |    2 +-
 gdb/exceptions.c                                   |    6 +-
 gdb/exec.c                                         |  131 +-
 gdb/fbsd-nat.c                                     |  218 +-
 gdb/fbsd-nat.h                                     |   70 +-
 gdb/frame.c                                        |    2 +-
 gdb/gdbarch-selftests.c                            |    4 +-
 gdb/gdbcore.h                                      |   10 +-
 gdb/gnu-nat.c                                      |  101 +-
 gdb/gnu-nat.h                                      |   34 +-
 gdb/gnu-v3-abi.c                                   |    2 +-
 gdb/go32-nat.c                                     |  181 +-
 gdb/hppa-linux-nat.c                               |   32 +-
 gdb/hppa-nbsd-nat.c                                |   30 +-
 gdb/hppa-obsd-nat.c                                |   25 +-
 gdb/i386-bsd-nat.c                                 |   24 +-
 gdb/i386-bsd-nat.h                                 |   22 +-
 gdb/i386-darwin-nat.c                              |   28 +-
 gdb/i386-fbsd-nat.c                                |   37 +-
 gdb/i386-gnu-nat.c                                 |   29 +-
 gdb/i386-linux-nat.c                               |   42 +-
 gdb/i386-nbsd-nat.c                                |    9 +-
 gdb/i386-obsd-nat.c                                |    5 +-
 gdb/ia64-linux-nat.c                               |  148 +-
 gdb/ia64-tdep.c                                    |    2 +-
 gdb/ia64-vms-tdep.c                                |    2 +-
 gdb/inf-child.c                                    |  327 +-
 gdb/inf-child.h                                    |  113 +-
 gdb/inf-ptrace.c                                   |  142 +-
 gdb/inf-ptrace.h                                   |   67 +-
 gdb/infcall.c                                      |    4 +-
 gdb/infcmd.c                                       |   36 +-
 gdb/infrun.c                                       |   12 +-
 gdb/linux-fork.c                                   |    2 +-
 gdb/linux-nat-trad.c                               |   54 +-
 gdb/linux-nat-trad.h                               |   24 +-
 gdb/linux-nat.c                                    |  636 +--
 gdb/linux-nat.h                                    |  217 +-
 gdb/linux-tdep.c                                   |   18 +-
 gdb/linux-thread-db.c                              |  157 +-
 gdb/m32r-linux-nat.c                               |   29 +-
 gdb/m68k-bsd-nat.c                                 |   25 +-
 gdb/m68k-linux-nat.c                               |   32 +-
 gdb/m88k-bsd-nat.c                                 |   25 +-
 gdb/make-target-delegates                          |  223 +-
 gdb/mi/mi-main.c                                   |   13 +-
 gdb/minsyms.c                                      |    4 +-
 gdb/mips-fbsd-nat.c                                |   25 +-
 gdb/mips-linux-nat.c                               |  192 +-
 gdb/mips-nbsd-nat.c                                |   25 +-
 gdb/mips64-obsd-nat.c                              |   23 +-
 gdb/nbsd-nat.c                                     |    2 +-
 gdb/nbsd-nat.h                                     |   10 +-
 gdb/nto-procfs.c                                   |  345 +-
 gdb/obsd-nat.c                                     |   32 +-
 gdb/obsd-nat.h                                     |   10 +-
 gdb/parse.c                                        |    2 +-
 gdb/ppc-fbsd-nat.c                                 |   26 +-
 gdb/ppc-linux-nat.c                                |  208 +-
 gdb/ppc-linux-tdep.c                               |   10 +-
 gdb/ppc-nbsd-nat.c                                 |   26 +-
 gdb/ppc-obsd-nat.c                                 |   26 +-
 gdb/procfs.c                                       |  316 +-
 gdb/ravenscar-thread.c                             |  240 +-
 gdb/record-btrace.c                                |  566 +--
 gdb/record-full.c                                  |  618 +--
 gdb/record.c                                       |   18 +-
 gdb/regcache.c                                     |   64 +-
 gdb/remote-sim.c                                   |  233 +-
 gdb/remote.c                                       | 1386 +++---
 gdb/rs6000-nat.c                                   |   95 +-
 gdb/rs6000-tdep.c                                  |    2 +-
 gdb/s390-linux-nat.c                               |  162 +-
 gdb/s390-tdep.c                                    |    2 +-
 gdb/sh-nbsd-nat.c                                  |   24 +-
 gdb/sol-thread.c                                   |  173 +-
 gdb/solib-aix.c                                    |    2 +-
 gdb/solib-darwin.c                                 |    2 +-
 gdb/solib-dsbt.c                                   |    4 +-
 gdb/solib-spu.c                                    |    6 +-
 gdb/solib-svr4.c                                   |   18 +-
 gdb/solib-target.c                                 |    2 +-
 gdb/sparc-linux-nat.c                              |   24 +-
 gdb/sparc-nat.c                                    |   39 +-
 gdb/sparc-nat.h                                    |   41 +-
 gdb/sparc-nbsd-nat.c                               |    5 +-
 gdb/sparc-tdep.c                                   |    2 +-
 gdb/sparc64-fbsd-nat.c                             |    9 +-
 gdb/sparc64-linux-nat.c                            |   33 +-
 gdb/sparc64-nbsd-nat.c                             |    6 +-
 gdb/sparc64-obsd-nat.c                             |    5 +-
 gdb/sparc64-tdep.c                                 |    4 +-
 gdb/spu-linux-nat.c                                |   78 +-
 gdb/spu-multiarch.c                                |  164 +-
 gdb/spu-tdep.c                                     |   30 +-
 gdb/symfile.c                                      |    2 +-
 gdb/target-debug.h                                 |    6 +
 gdb/target-delegates.c                             | 5194 +++++++++-----------
 gdb/target-descriptions.c                          |    4 +-
 gdb/target-memory.c                                |    4 +-
 gdb/target.c                                       |  954 ++--
 gdb/target.h                                       |  926 ++--
 gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |    4 +-
 gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp     |    4 +-
 gdb/tilegx-linux-nat.c                             |   34 +-
 gdb/tracefile-tfile.c                              |  122 +-
 gdb/tracefile.c                                    |   38 +-
 gdb/tracefile.h                                    |   16 +-
 gdb/tracepoint.c                                   |    2 +-
 gdb/valops.c                                       |    4 +-
 gdb/valprint.c                                     |    2 +-
 gdb/value.c                                        |    2 +-
 gdb/vax-bsd-nat.c                                  |   25 +-
 gdb/windows-nat.c                                  |  223 +-
 gdb/windows-tdep.c                                 |    2 +-
 gdb/x86-bsd-nat.c                                  |   27 +-
 gdb/x86-bsd-nat.h                                  |   21 +-
 gdb/x86-linux-nat.c                                |   91 +-
 gdb/x86-linux-nat.h                                |   62 +-
 gdb/x86-nat.c                                      |   64 +-
 gdb/x86-nat.h                                      |   79 +-
 gdb/xtensa-linux-nat.c                             |   32 +-
 150 files changed, 9106 insertions(+), 9305 deletions(-)
 create mode 100644 gdb/amd64-bsd-nat.h

-- 
2.14.3

Comments

Tom Tromey April 27, 2018, 3:47 p.m. | #1
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Here's another chunk from the multi-target branch.
Pedro> This converts target_ops to a C++ class with virtual methods.

Pedro> The reason this is part of the multi-target effort is that with
Pedro> multi-target, we will want to be able to instantiate the remote, core,
Pedro> etc. targets more than once at the same time.
[...]
Pedro> In the end, I'm
Pedro> convinced this paid off.

As you know I worked on this a bit, without success, a few years ago.  I
think your approach is definitely the way to go.

Tom
Simon Marchi April 29, 2018, 3:22 p.m. | #2
On 2018-04-14 03:09 PM, Pedro Alves wrote:
> Here's another chunk from the multi-target branch.


Hi Pedro,

It's kind of difficult to properly review this series, because the important
non-mechanical bits are lost in a sea of mechanical changes.  But I didn't
find anything fundamental I would change, and all you wrote in commit logs
made sense to me.

Maybe one little comment about 40/40 (because I just finished looking at that
patch): it might be good to add an assertion (the_native_target == nullptr)
in set_native_target and one in add_target
(target_factories.find (&t) == target_factories.end ()).  I think it could
help catch problems if somebody is trying to add a new target or change
existing ones.

Otherwise, I would suggest not waiting too long before merging it to
avoid having to resolve too many conflicts.  Thanks a lot for doing this!

Simon
Pedro Alves May 2, 2018, 10:51 p.m. | #3
On 04/29/2018 04:22 PM, Simon Marchi wrote:
> On 2018-04-14 03:09 PM, Pedro Alves wrote:

>> Here's another chunk from the multi-target branch.

> 

> Hi Pedro,

> 

> It's kind of difficult to properly review this series, because the important

> non-mechanical bits are lost in a sea of mechanical changes.  


Yeah, sorry about that.

> But I didn't

> find anything fundamental I would change, and all you wrote in commit logs

> made sense to me.

> 

> Maybe one little comment about 40/40 (because I just finished looking at that

> patch): it might be good to add an assertion (the_native_target == nullptr)

> in set_native_target and one in add_target

> (target_factories.find (&t) == target_factories.end ()).  I think it could

> help catch problems if somebody is trying to add a new target or change

> existing ones.


Good idea.  I'm adding this to patch #40:

 gdb/target.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/gdb/target.c b/gdb/target.c
index b957769a3f..824855e7df 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -352,7 +352,11 @@ add_target (const target_info &t, target_open_ftype *func,
 {
   struct cmd_list_element *c;
 
-  target_factories[&t] = func;
+  auto &func_slot = target_factories[&t];
+  if (func_slot != nullptr)
+    internal_error (__FILE__, __LINE__,
+		    "target already added (\"%s\").", t.shortname);
+  func_slot = func;
 
   if (targetlist == NULL)
     add_prefix_cmd ("target", class_run, target_command, _("\
@@ -2447,6 +2451,11 @@ static target_ops *the_native_target;
 void
 set_native_target (target_ops *target)
 {
+  if (the_native_target != NULL)
+    internal_error (__FILE__, __LINE__,
+		    _("native target already set (\"%s\")."),
+		    the_native_target->longname ());
+
   the_native_target = target;
 }

> Otherwise, I would suggest not waiting too long before merging it to

> avoid having to resolve too many conflicts.  Thanks a lot for doing this!

Alright, I wrote the missing ChangeLogs today, and will proceed with
squashing the patches that need to be squashed and getting it all in.

Thanks,
Pedro Alves
Pedro Alves May 2, 2018, 10:55 p.m. | #4
On 04/27/2018 04:47 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> Here's another chunk from the multi-target branch.

> Pedro> This converts target_ops to a C++ class with virtual methods.

> 

> Pedro> The reason this is part of the multi-target effort is that with

> Pedro> multi-target, we will want to be able to instantiate the remote, core,

> Pedro> etc. targets more than once at the same time.

> [...]

> Pedro> In the end, I'm

> Pedro> convinced this paid off.

> 

> As you know I worked on this a bit, without success, a few years ago.  I

> think your approach is definitely the way to go.


Thanks Tom.  I wouldn't say without success at all -- you finished
important groundwork at the time, like for example all the
delegation cleaning up and normalization.  Remember that 200 patch
series? :-)  And of course the ideas on my branch draw lessons from
your earlier experiments.

Thanks,
Pedro Alves