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

Alan Hayward
2018-06-04T15:49:41Z

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