Re: [PATCH 8/8] Ptrace support for Aarch64 SVE

[Alan Hayward <Alan.Hayward@arm.com>]

>>>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
>>>> index 9381786fda..84c7a41f40 100644
>>>> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
>>>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
>>>> @@ -25,6 +25,13 @@
>>>> #include "aarch64-sve-linux-ptrace.h"
>>>> #include "arch/aarch64.h"
>>>> 
>>>> +#ifndef GDBSERVER
>>>> +#include "defs.h"
>>>> +#endif
>>>> +#include "regcache.h"
>>> 
>>> Hmm we try not add any more "#ifdef GDBSERVER" in the common code.
>>> 
>>> https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29
>>> 
>>> Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the
>>> common code will use, and that regcaches from GDB and GDBserver will implement.
>> 
>> I tried using common-defs.h, but gdb/regcache.h requires defines from
>> defs.h - RequireLongest and maybe others.
>> Putting defs.h at the top of gdb/regcache.h then broke in a weird way.
>> A lot of fiddling later, and I hadn’t found a way to make it work.
>> 
>> Creating common/common-regcache.h gets a bit odd because, the functions
>> I need for gdbserver (raw_supply, raw_collect and get_register_status)
>> on gdb come from:
>> 
>> 
>> class reg_buffer
>> ...
>>  enum register_status get_register_status (int regnum) const;
>> ...
>> 
>> 
>> class readable_regcache : public reg_buffer
>> ...
>> 
>> 
>> class detached_regcache : public readable_regcache
>> ...
>>  void raw_supply (int regnum, const void *buf);
>> ...
>> 
>> 
>> class regcache : public detached_regcache
>> ...
>>  void raw_collect (int regnum, void *buf) const;
>> ...
>> 
>> 
>> I don’t think that this would work:
>> class regcache : public detached_regcache, common_regcache
> 
> I did some quick hacking and it seems to work to have this in common/common-regcache.h
> 
> struct reg_buffer_common
> {
>  virtual ~reg_buffer_common () = default;
>  virtual void raw_supply (int regnum, const void *buf) = 0;
>  virtual void raw_collect (int regnum, void *buf) const = 0;
>  virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;
>  virtual register_status get_register_status (int regnum) const = 0;
> };
> 
> and make your code in nat/ take a pointer to reg_buffer_common.  gdb's reg_buffer
> and gdbserver's regcache should inherit from reg_buffer_common.  I  built
> it on other regcache refactor patches I have in the pipeline, so maybe some other
> changes in there are needed, maybe not.  I pushed the result on this branch, it's
> a bit raw but it should be enough to show the idea.
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/regcache-for-alan

Thanks for trying this out, however I tried adding your changes
into my build, but that results in the following:

../../src/binutils-gdb/gdb/linux-fork.c: In function ‘void fork_save_infrun_state(fork_info*, int)’:
../../src/binutils-gdb/gdb/linux-fork.c:298:75: error: invalid new-expression of abstract class type ‘readonly_detached_regcache’
   fp->savedregs = new readonly_detached_regcache (*get_current_regcache ());
                                                                           ^
In file included from ../../src/binutils-gdb/gdb/gdbarch.h:70:0,
                 from ../../src/binutils-gdb/gdb/defs.h:531,
                 from ../../src/binutils-gdb/gdb/linux-fork.c:20:
../../src/binutils-gdb/gdb/regcache.h:367:7: note:   because the following virtual functions are pure within ‘readonly_detached_regcache’:
 class readonly_detached_regcache : public readable_regcache
       ^
In file included from ../../src/binutils-gdb/gdb/regcache.h:23:0,
                 from ../../src/binutils-gdb/gdb/gdbarch.h:70,
                 from ../../src/binutils-gdb/gdb/defs.h:531,
                 from ../../src/binutils-gdb/gdb/linux-fork.c:20:
../../src/binutils-gdb/gdb/common/common-regcache.h:68:16: note: 	virtual void reg_buffer_common::raw_supply(int, const void*)
   virtual void raw_supply (int regnum, const void *buf) = 0;
                ^
../../src/binutils-gdb/gdb/common/common-regcache.h:69:16: note: 	virtual void reg_buffer_common::raw_collect(int, void*) const
   virtual void raw_collect (int regnum, void *buf) const = 0;


And then similar errors trying to create a register_dump_reg_buffer.


I could then add the following to readonly_detached_regcache:

  void raw_supply (int regnum, const void *buf) override
  { gdb_assert(false); }

  void raw_collect (int regnum, void *buf) const override
  { gdb_assert(false); }

...Or even make the methods in reg_buffer_common have a
{ gdb_assert(false); } body.

But that feels wrong, as it’s breaking the nice regcache abstractions.

Any thoughts?

Alan.