[PATCH 8/8] Ptrace support for Aarch64 SVE

[Simon Marchi]

On 2018-05-31 10:46 AM, Alan Hayward wrote:
> 
> 
>> On 31 May 2018, at 14:22, Simon Marchi <simon.marchi@ericsson.com> wrote:
>>
>> Hi Alan,
>>
>> Since there is a good number of macros copied from the Linux kernel, it might be
>> a good idea to isolate in their own file.  It would also be good to identify
>> precisely which file they come from (the path relative to the root of the linux
>> repo) and the git commit you used.
>>
>> Also, since the copied code is rather large and non-trivial, does it pose copyright
>> problems?  If we want to include it, maybe that separate file should not state that
>> the copyright is owned by the FSF, since it's not the case?  Maybe others have
>> experience with this kind of things, or maybe we should get an advice from the FSF directly.
> 
> A new file makes sense (especially with regards to copyright). However, the plan was
> to eventually remove the macros when "enough" time has passed - keeping everything
> in aarch64-sve-linux-ptrace.h made it simpler than removing files.

How come?  Removing a file is not complicated.

> Is there a minimum kernel version that gdb requires to link against in order to build?
> The relevant defines are in 4.15. The kernel is now on 4.16.9.
>
> I’m guessing this must have come up before when other targets added new kernel features?
> I didn’t want to suddenly destroy everyone’s aarch64/targetall build trees without
> warning.

I don't think there's a specific Linux kernel version we require to build GDB, but it's
good to support as far back as possible with a reasonable effort.  Since they have only
been added very recently, I would keep our copy of the macros for a while, if it's not
in the way of anything.

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

>> Since this returns allocated memory, could we return an RAII object?  Either
>> std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.
> 
> Yes, I’ll update with one of those. Is there any documentation explaining
> when to use which of these?

I don't think so, if you think it would be relevant we could add it to the wiki.

In short:

std::vector: Allocates and manages its own memory.  This is good when we allocate
the space ourselves in GDB, and is especially good for buffers that need to change
size over time.  By default, an std::vector<gdb_byte> would zero-out the buffer
on allocation.  To avoid that when we don't need it, we have a specialization,
gdb::byte_vector, that leaves the memory uninitialized.  You could

gdb::byte_vector buf (iovec.iov_len);
iovec.iov_base = buf.data ();

...

return buf;

std::unique_ptr: It is used to wrap a pointer to something that has been allocated
with "new", it will free it with "delete".  You could do

std::unique_ptr<gdb_byte> buf (new gdb_byte[iovec.iov_len]);
iovec.iov_base = buf.get ()

...

return buf;

gdb::unique_xmalloc_ptr: It is like std::unique_ptr, but frees the memory using
xfree.  It is useful to gradually add some RAII to GDB code that uses xmalloc.

For new code, I would either use std::vector or std::unique_ptr.

I just noticed that in the code in the current patch leaks the allocated memory
if ptrace fails and we call perror_with_name.  Using an object that manages
memory will fix that.

>> When fetching registers, won't it be usually to fill a shiny new, empty regcache?
>> In that case, won't it always fall into the "if" branch?
>>
>> In any case, should we check the status of the VG register to make sure it's REG_VALID
>> before we try to collect it?
> 
> I thought the same regcache was used each time registers needed reading?
> I’ll double check this.
> Either way, yes, should probably check REG_VALID

Hmm I'm not sure either.  Indeed it would make sense that we re-use the same regcache.
Thanks for double-checking.

Thanks,

Simon