[PATCH 0/7] Use warn_unused_result on release methods

[PATCH 0/7] Use warn_unused_result on release methods

[Tom Tromey]

This applies the warn_unused_result attribute to various "release"
methods in gdb.  The idea here is to avoid the bug where "release" is
used when "reset (nullptr)" is meant.

This caught one bug, so I consider it a success.  Perhaps the new
attribute can be used elsewhere as well.

Tested by the buildbot.
Let me know what you think.

thanks,
Tom

[PATCH 2/7] Add ATTRIBUTE_UNUSED_RESULT to parser_state

[Tom Tromey]

This applies ATTRIBUTE_UNUSED_RESULT to parser_state::release.

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

	* parser-defs.h (struct parser_state) <release>: Add
	ATTRIBUTE_UNUSED_RESULT.
---
 gdb/ChangeLog     | 5 +++++
 gdb/parser-defs.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index 0d4bb820d7b..5d2ee331c05 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -48,7 +48,7 @@ struct parser_state
 
   /* Resize the allocated expression to the correct size, and return
      it as an expression_up -- passing ownership to the caller.  */
-  expression_up release ();
+  ATTRIBUTE_UNUSED_RESULT expression_up release ();
 
   /* The size of the expression above.  */
 
-- 
2.20.1

[PATCH 6/7] Add ATTRIBUTE_UNUSED_RESULT to scoped_remote_fd::release

[Tom Tromey]

This applies ATTRIBUTE_UNUSED_RESULT to scoped_remote_fd::release.

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

	* remote.c (class scoped_remote_fd) <release>: Add
	ATTRIBUTE_UNUSED_RESULT.
---
 gdb/ChangeLog | 5 +++++
 gdb/remote.c  | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 36136e3e3ee..c98c5cb2ebf 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12410,7 +12410,7 @@ public:
   DISABLE_COPY_AND_ASSIGN (scoped_remote_fd);
 
   /* Release ownership of the file descriptor, and return it.  */
-  int release () noexcept
+  ATTRIBUTE_UNUSED_RESULT int release () noexcept
   {
     int fd = m_fd;
     m_fd = -1;
-- 
2.20.1

[PATCH 3/7] Add ATTRIBUTE_UNUSED_RESULT to scoped_fd

[Tom Tromey]

This applies ATTRIBUTE_UNUSED_RESULT to scoped_fd::release.

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

	* common/scoped_fd.h (class scoped_fd) <release>: Add
	ATTRIBUTE_UNUSED_RESULT.
---
 gdb/ChangeLog          | 5 +++++
 gdb/common/scoped_fd.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/common/scoped_fd.h b/gdb/common/scoped_fd.h
index bdd22f595a5..c4a494b7018 100644
--- a/gdb/common/scoped_fd.h
+++ b/gdb/common/scoped_fd.h
@@ -56,7 +56,7 @@ public:
 
   DISABLE_COPY_AND_ASSIGN (scoped_fd);
 
-  int release () noexcept
+  ATTRIBUTE_UNUSED_RESULT int release () noexcept
   {
     int fd = m_fd;
     m_fd = -1;
-- 
2.20.1

[PATCH 1/7] Introduce ATTRIBUTE_UNUSED_RESULT and use it

[Tom Tromey]

This introduces the new ATTRIBUTE_UNUSED_RESULT define, and applies it
to gdb_argv::release.

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

	* utils.h (class gdb_argv) <release>: Add
	ATTRIBUTE_UNUSED_RESULT.
	* common/common-defs.h (ATTRIBUTE_UNUSED_RESULT): Define.
---
 gdb/ChangeLog            | 6 ++++++
 gdb/common/common-defs.h | 6 ++++++
 gdb/utils.h              | 2 +-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index 732693d65cc..6b1f004ab1b 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -93,6 +93,12 @@
 #undef ATTRIBUTE_PRINTF
 #define ATTRIBUTE_PRINTF _GL_ATTRIBUTE_FORMAT_PRINTF
 
+#if GCC_VERSION >= 3004
+#define ATTRIBUTE_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
+#else
+#define ATTRIBUTE_UNUSED_RESULT
+#endif
+
 #include "libiberty.h"
 #include "pathmax.h"
 #include "gdb/signals.h"
diff --git a/gdb/utils.h b/gdb/utils.h
index 896feb973c9..9dbd6386c68 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -188,7 +188,7 @@ public:
   /* Return the underlying array, transferring ownership to the
      caller.  */
 
-  char **release ()
+  ATTRIBUTE_UNUSED_RESULT char **release ()
   {
     char **result = m_argv;
     m_argv = NULL;
-- 
2.20.1

[PATCH 7/7] Add ATTRIBUTE_UNUSED_RESULT to ref_ptr::release

[Tom Tromey]

This applies ATTRIBUTE_UNUSED_RESULT to ref_ptr::release and updates a
few spots to comply.  I believe one use in install_default_visualizer
was in error, fixed by this patch.

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

	* varobj.c (update_dynamic_varobj_children): Update.
	(install_default_visualizer): Use reset, not release.
	* value.c (set_internalvar): Update.
	* dwarf2loc.c (value_of_dwarf_reg_entry): Update.
	* common/gdb_ref_ptr.h (class ref_ptr) <release>: Add
	ATTRIBUTE_UNUSED_RESULT.
---
 gdb/ChangeLog            |  9 +++++++++
 gdb/common/gdb_ref_ptr.h |  2 +-
 gdb/dwarf2loc.c          |  3 +--
 gdb/value.c              | 10 +++++-----
 gdb/varobj.c             |  4 ++--
 5 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/gdb/common/gdb_ref_ptr.h b/gdb/common/gdb_ref_ptr.h
index b104556cc5f..c31c9a2e58b 100644
--- a/gdb/common/gdb_ref_ptr.h
+++ b/gdb/common/gdb_ref_ptr.h
@@ -135,7 +135,7 @@ class ref_ptr
   /* Return this instance's referent, and stop managing this
      reference.  The caller is now responsible for the ownership of
      the reference.  */
-  T *release ()
+  ATTRIBUTE_UNUSED_RESULT T *release ()
   {
     T *result = m_obj;
 
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 9c974a11133..29d289b4d0c 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1472,9 +1472,8 @@ value_of_dwarf_reg_entry (struct type *type, struct frame_info *frame,
 					       target_type, caller_frame,
 					       caller_per_cu);
 
-  release_value (target_val).release ();
   val = allocate_computed_value (type, &entry_data_value_funcs,
-				 target_val /* closure */);
+				 release_value (target_val).release ());
 
   /* Copy the referencing pointer to the new computed value.  */
   memcpy (value_contents_raw (val), value_contents_raw (outer_val),
diff --git a/gdb/value.c b/gdb/value.c
index 95b6a56f32e..50e47a936aa 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -2274,20 +2274,20 @@ set_internalvar (struct internalvar *var, struct value *val)
 
     default:
       new_kind = INTERNALVAR_VALUE;
-      new_data.value = value_copy (val);
-      new_data.value->modifiable = 1;
+      struct value *copy = value_copy (val);
+      copy->modifiable = 1;
 
       /* Force the value to be fetched from the target now, to avoid problems
 	 later when this internalvar is referenced and the target is gone or
 	 has changed.  */
-      if (value_lazy (new_data.value))
-       value_fetch_lazy (new_data.value);
+      if (value_lazy (copy))
+	value_fetch_lazy (copy);
 
       /* Release the value from the value chain to prevent it from being
 	 deleted by free_all_values.  From here on this function should not
 	 call error () until new_data is installed into the var->u to avoid
 	 leaking memory.  */
-      release_value (new_data.value).release ();
+      new_data.value = release_value (copy).release ();
 
       /* Internal variables which are created from values with a dynamic
          location don't need the location property of the origin anymore.
diff --git a/gdb/varobj.c b/gdb/varobj.c
index b03307068f3..3715bb6a7df 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -760,7 +760,7 @@ update_dynamic_varobj_children (struct varobj *var,
 	  /* Release vitem->value so its lifetime is not bound to the
 	     execution of a command.  */
 	  if (item != NULL && item->value != NULL)
-	    release_value (item->value).release ();
+	    item->value = release_value (item->value).release ();
 	}
 
       if (item == NULL)
@@ -1127,7 +1127,7 @@ install_default_visualizer (struct varobj *var)
 	}
 
       if (pretty_printer == Py_None)
-	pretty_printer.release ();
+	pretty_printer.reset (nullptr);
   
       install_visualizer (var->dynamic, NULL, pretty_printer.release ());
     }
-- 
2.20.1

Re: [PATCH 0/7] Use warn_unused_result on release methods

[John Baldwin]

On 2/27/19 2:18 PM, Tom Tromey wrote:
> This applies the warn_unused_result attribute to various "release"
> methods in gdb.  The idea here is to avoid the bug where "release" is
> used when "reset (nullptr)" is meant.
> 
> This caught one bug, so I consider it a success.  Perhaps the new
> attribute can be used elsewhere as well.
> 
> Tested by the buildbot.
> Let me know what you think.

Looks good to me.

-- 
John Baldwin

[PATCH 4/7] Add ATTRIBUTE_UNUSED_RESULT to scoped_mmap

[Tom Tromey]

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

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 ();
 }
-- 
2.20.1

RE: [PATCH 4/7] Add ATTRIBUTE_UNUSED_RESULT to scoped_mmap

[Metzger, Markus T]

> 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

Re: [PATCH 4/7] Add ATTRIBUTE_UNUSED_RESULT to scoped_mmap

[Tom Tromey]

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

[PATCH 5/7] Add ATTRIBUTE_UNUSED_RESULT to macro_buffer

[Tom Tromey]

This applies ATTRIBUTE_UNUSED_RESULT to macro_buffer::release.

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

	* macroexp.c (struct macro_buffer) <release>: Add
	ATTRIBUTE_UNUSED_RESULT.
---
 gdb/ChangeLog  | 5 +++++
 gdb/macroexp.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index 0e8e85cdf23..a588cc836fe 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -130,7 +130,7 @@ struct macro_buffer
 
   /* Release the text of the buffer to the caller, which is now
      responsible for freeing it.  */
-  char *release ()
+  ATTRIBUTE_UNUSED_RESULT char *release ()
   {
     gdb_assert (! shared);
     gdb_assert (size);
-- 
2.20.1

Re: [PATCH 0/7] Use warn_unused_result on release methods

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> This applies the warn_unused_result attribute to various "release"
Tom> methods in gdb.  The idea here is to avoid the bug where "release" is
Tom> used when "reset (nullptr)" is meant.

Tom> This caught one bug, so I consider it a success.  Perhaps the new
Tom> attribute can be used elsewhere as well.

I'm checking this in now.

Tom