Re: [PATCH 1/5] btrace: prepare for throwing exceptions when enabling btrace

[Pedro Alves <palves at redhat dot com>]

Hi Markus,

A few quick comments on the utilities added by the patch.

On 01/25/2018 09:12 AM, Markus Metzger wrote:
> We indicate success or failure for enabling branch tracing via the pointer
> return value.  Depending on the type of error, errno may provide additional
> information.
> 
> Prepare for using exceptions with more descriptive error messages by using smart
> pointers and objects with automatic destruction to hold intermediate results.
> 
> 2018-01-25  Markus Metzger  <markus.t.metzger@intel.com>
> 
> gdb/
> 	* nat/linux-btrace.c (perf_event_pt_event_type): Use gdb_file_up.
> 	(scoped_close_fd, scoped_mmap): New.
> 	(linux_enable_bts, linux_enable_pt): Use gdb::unique_xmalloc_ptr,
> 	scoped_close_fd, and scoped_mmap.
> ---
>  gdb/nat/linux-btrace.c | 183 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 117 insertions(+), 66 deletions(-)
> 
> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
> index bbd0fe6..280d9f1 100644
> --- a/gdb/nat/linux-btrace.c
> +++ b/gdb/nat/linux-btrace.c
> @@ -179,17 +179,12 @@ perf_event_read_all (struct perf_event_buffer *pev, gdb_byte **data,
>  static int
>  perf_event_pt_event_type (int *type)
>  {
> -  FILE *file;
> -  int found;
> -
> -  file = fopen ("/sys/bus/event_source/devices/intel_pt/type", "r");
> -  if (file == NULL)
> +  gdb_file_up file =
> +    gdb_fopen_cloexec ("/sys/bus/event_source/devices/intel_pt/type", "r");

" = " goes on the next line.

> +  if (file.get () == nullptr)
>      return -1;

The ".get()" shouldn't be necessary here.

>  
> -  found = fscanf (file, "%d", type);
> -
> -  fclose (file);
> -
> +  int found = fscanf (file.get (), "%d", type);
>    if (found == 1)
>      return 0;
>    return -1;
> @@ -657,19 +652,95 @@ linux_supports_btrace (struct target_ops *ops, enum btrace_format format)
>    internal_error (__FILE__, __LINE__, _("Unknown branch trace format"));
>  }
>  
> +class scoped_close_fd

Missing intro comment.

> +{
> +  int m_fd;

Please put private data fields after the public interface.

> +
> +public:
> +  scoped_close_fd (int fd = -1) noexcept : m_fd (fd) {}

Explicit?

> +  ~scoped_close_fd () noexcept

Note: destructors are noexcept by default.

> +    {
> +      if (m_fd >= 0)
> +	close (m_fd);
> +    }
> +
> +  scoped_close_fd (const scoped_close_fd &) = delete;
> +  scoped_close_fd &operator = (const scoped_close_fd &) = delete;

Use DISABLE_COPY_AND_ASSIGN.

> +
> +  int release () noexcept
> +    {
> +      int fd = m_fd;
> +      m_fd = -1;
> +      return fd;
> +    }
> +
> +  operator int () const noexcept

That's unusual.  Do we gain much compared to an explicit
fd() or "get()" getter ?

If this is a smart-pointer-like wrapper for a file descriptor,
then maybe it would be better called scoped_fd instead of 
scoped_close_fd, just like the mmap one isn't called "scoped_unmmap"
instead.  But no biggie.

> +    {
> +      return m_fd;
> +    }
> +};
> +
> +template <typename object_type>
> +class scoped_mmap
> +{
> +  void *m_mem;
> +  size_t m_length;
> +
> +public:
> +  scoped_mmap () noexcept : m_mem (nullptr), m_length (0) {}
> +  scoped_mmap (void *addr, size_t length, int prot, int flags, int fd,
> +	       off_t offset) noexcept : m_mem (nullptr), m_length (length)
> +    {
> +      m_mem = mmap (addr, m_length, prot, flags, fd, offset);

If scoped_mmap is templated on object_type, why do we need
the length parameter, in bytes?  Or conversely, if the interface
is in terms of bytes, why do we need the template parameter?
(AFAICS, it's for operator->, but, is that really useful
compared to a mismatch in the API?  Seems like a layering
violation to me.)

> +    }
> +  ~scoped_mmap () noexcept
> +    {
> +      if (m_mem != nullptr)
> +	munmap (m_mem, m_length);
> +    }
> +
> +  scoped_mmap (const scoped_mmap &) = delete;
> +  scoped_mmap &operator = (const scoped_mmap &) = delete;
> +
> +  object_type *release () noexcept
> +    {
> +      void *mem = m_mem;
> +      m_mem = nullptr;
> +      return static_cast<object_type *> (mem);
> +    }
> +
> +  void reset (void *addr, size_t length, int prot, int flags, int fd,
> +	      off_t offset) noexcept
> +    {
> +      if (m_mem != nullptr)
> +	munmap (m_mem, m_length);
> +
> +      m_length = length;
> +      m_mem = mmap (addr, m_length, prot, flags, fd, offset);
> +    }
> +
> +  size_t size () const noexcept { return m_length; }
> +  uint8_t *raw () const noexcept { return static_cast<uint8_t *> (m_mem); }
> +
> +  operator void * () const noexcept { return m_mem; }
> +  object_type *operator -> () const noexcept
> +    {
> +      return static_cast<object_type *> (m_mem);
> +    }
> +};


Basically the same comments to scoped_close_fd apply here too.

You should add describing comments to the methods too.

These seem like general enough utilities that might be
better for the long run to put them straight in
gdb/common/.  (Unit tests would be nice.)

BTW, I find the indentation of the function-open "{"s a
little unusual.

Thanks,
Pedro Alves