From: Pedro Alves <palves@redhat.com>
To: Markus Metzger <markus.t.metzger@intel.com>, gdb-patches@sourceware.org
Subject: Re: [PATCH 1/5] btrace: prepare for throwing exceptions when enabling btrace
Date: Thu, 25 Jan 2018 14:46:00 -0000 [thread overview]
Message-ID: <a8c43c5a-873f-4bab-8c22-3d1a8d5a2426@redhat.com> (raw)
In-Reply-To: <1516871526-32129-2-git-send-email-markus.t.metzger@intel.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
next prev parent reply other threads:[~2018-01-25 14:46 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-25 9:12 [PATCH 0/5] improve btrace enable error reporting Markus Metzger
2018-01-25 9:12 ` [PATCH 5/5] btrace: check perf_event_paranoid Markus Metzger
2018-01-25 9:12 ` [PATCH 2/5] btrace, gdbserver: use exceptions to convey btrace enable/disable errors Markus Metzger
2018-01-25 9:12 ` [PATCH 4/5] btrace: improve enable error messages Markus Metzger
2018-01-25 9:12 ` [PATCH 1/5] btrace: prepare for throwing exceptions when enabling btrace Markus Metzger
2018-01-25 14:46 ` Pedro Alves [this message]
2018-01-26 13:55 ` Metzger, Markus T
2018-01-25 9:12 ` [PATCH 3/5] btrace, gdbserver: remove the to_supports_btrace target method Markus Metzger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a8c43c5a-873f-4bab-8c22-3d1a8d5a2426@redhat.com \
--to=palves@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=markus.t.metzger@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).