diff mbox

[4/7] Add ATTRIBUTE_UNUSED_RESULT to scoped_mmap

Message ID 20190227221814.17661-5-tromey@adacore.com
State New
Headers show

Commit Message

Tom Tromey Feb. 27, 2019, 10:18 p.m. UTC
This applies ATTRIBUTE_UNUSED_RESULT to scoped_mmap::release and fixes
a couple of spots to comply.

2019-02-27  Tom Tromey  <tromey@adacore.com>

	* nat/linux-btrace.c (linux_enable_bts, linux_enable_pt): Update.
	* common/scoped_mmap.h (class scoped_mmap) <release>: Add
	ATTRIBUTE_UNUSED_RESULT.
---
 gdb/ChangeLog            | 6 ++++++
 gdb/common/scoped_mmap.h | 2 +-
 gdb/nat/linux-btrace.c   | 9 +++------
 3 files changed, 10 insertions(+), 7 deletions(-)

Comments

Metzger, Markus T Feb. 28, 2019, 7:41 a.m. UTC | #1
> This applies ATTRIBUTE_UNUSED_RESULT to scoped_mmap::release and fixes a
> couple of spots to comply.
> 
> 2019-02-27  Tom Tromey  <tromey@adacore.com>
> 
> 	* nat/linux-btrace.c (linux_enable_bts, linux_enable_pt): Update.
> 	* common/scoped_mmap.h (class scoped_mmap) <release>: Add
> 	ATTRIBUTE_UNUSED_RESULT.

I don't really see the need for such a change but the patch LGTM.  Can we avoid
the warning by casting the result to void in case it becomes too awkward otherwise?
E.g.

	(void) data.release ();

In this case, for example, ...

> -  pt->header = header;
> +  pt->header = (struct perf_event_mmap_page *) data.release ();
> + gdb_assert (pt->header == header);

...we're storing the base pointer.  But we could also store &header-><some field>,
which would make it pretty awkward to use the result of release ().

Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, , Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Tom Tromey Feb. 28, 2019, 2:58 p.m. UTC | #2
>> I don't really see the need for such a change but the patch LGTM.  Can we avoid
>> the warning by casting the result to void in case it becomes too awkward otherwise?
>> E.g.

>> 	(void) data.release ();

I'm not sure if that works, but there are other ways to achieve the same
thing.

>> ...we're storing the base pointer.  But we could also store &header-><some field>,
>> which would make it pretty awkward to use the result of release ().

You can do &data.release().field.

I agree this could be awkward, but I don't anticipate it happening very
much.  Holding an interior pointer like that is pretty uncommon,
especially in the situation where one must "un-interior-ize" it in order
to free the resource.

Tom
diff mbox

Patch

diff --git a/gdb/common/scoped_mmap.h b/gdb/common/scoped_mmap.h
index 763f40e201c..0b504c96fec 100644
--- a/gdb/common/scoped_mmap.h
+++ b/gdb/common/scoped_mmap.h
@@ -57,7 +57,7 @@  public:
 
   DISABLE_COPY_AND_ASSIGN (scoped_mmap);
 
-  void *release () noexcept
+  ATTRIBUTE_UNUSED_RESULT void *release () noexcept
   {
     void *mem = m_mem;
     m_mem = MAP_FAILED;
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index b51e70fbaff..ef291ec2310 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -544,13 +544,11 @@  linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
 
   bts->bts.size = size;
   bts->bts.data_head = &header->data_head;
-  bts->bts.mem = (const uint8_t *) data.get () + data_offset;
+  bts->bts.mem = (const uint8_t *) data.release () + data_offset;
   bts->bts.last_head = 0ull;
   bts->header = header;
   bts->file = fd.release ();
 
-  data.release ();
-
   tinfo->conf.bts.size = (unsigned int) size;
   return tinfo.release ();
 }
@@ -667,11 +665,10 @@  linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   pt->pt.size = aux.size ();
   pt->pt.mem = (const uint8_t *) aux.release ();
   pt->pt.data_head = &header->aux_head;
-  pt->header = header;
+  pt->header = (struct perf_event_mmap_page *) data.release ();
+  gdb_assert (pt->header == header);
   pt->file = fd.release ();
 
-  data.release ();
-
   tinfo->conf.pt.size = (unsigned int) pt->pt.size;
   return tinfo.release ();
 }