This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 4/4] Poison XNEW and friends for types that should use new/delete


On 2017-11-23 10:02, Pedro Alves wrote:
On 11/22/2017 04:41 PM, Simon Marchi wrote:
From: Simon Marchi <simon.marchi@polymtl.ca>

This patch (finally!) makes it so that trying to use XNEW with a type
that requires "new" will cause a compilation error.

Yay!

  The criterion I
initially used to allow a type to use XNEW (which calls malloc in the
end) was std::is_trivially_constructible, but then realized that gcc 4.8
did not have it.  Instead, I went with:


Yeah, in the memset poisoning I just disabled the poisoning on older
compilers.  This is the "#if HAVE_IS_TRIVIALLY_COPYABLE" check in
poison.h.

I'm not sure if you are suggesting adding HAVE_IS_TRIVIALLY_CONSTRUCTIBLE, but since we can use std::is_pod easily, I think it's simpler just to use that (even if it's a bit more strict than needed).

  using IsMallocatable = std::is_pod<T>;


Nit, I think "malloc-able" is more common than "malloc-atable".
At least, the latter sounds odd to me.  Or sounds like
"m-allocatable", which is a different thing.  :-)

Agreed.

which is just a bit more strict, which doesn't hurt. A similar thing is
done for macros that free instead of allocated, the criterion is:

using IsFreeable = gdb::Or<std::is_trivially_destructible<T>, std::is_void<T>>;

For simplicity, we could also do for std::is_pod for IsFreeable as well,
if you prefer.

I chose to put static_assert in the functions, instead of using
gdb::Requires in the template as SFINAE, because it allows to put a
message, which I think makes the compiler error more understandable.
With gdb::Requires, the error is:

Right, I think static_assert is what I had suggest initially too.
SFINAE alone wouldn't sound right to me here.

The right alternative to static_assert would be SFINAE+"=delete;",
which is what we do for the memcpy/memcpy poisoning, and what
you're doing to "free".

=delete keeps the function/overload in the overload set, serving
as "honeypot", so you get an error about trying to call a
deleted function.

You could do that to xfree instead
of the static_assert, but I guess inlining xfree ends up being
a good thing.

Ah yeah, it would probably make the message a bit clearer. Instead of saying "I can't find a function that matches", it would say "I found a function that matches, but you can't use it". I'll just keep static_assert for xfree as well, since it ends up clearer anyway.

I think the first one is more likely to just make people yell at their
screen, especially those less used to C++.

Yes, don't do that.  :-)


Generated-code-wise, it adds one more function call (xnew<T>) when using
XNEW and building with -O0, but it all goes away with optimizations
enabled.

Good.


gdb/ChangeLog:

	* common/common-utils.h: Include poison.h.
	(xfree): Remove declaration, add definition with static_assert.
	* common/common-utils.c (xfree): Remove.
	* common/poison.h (IsMallocatable): Define.
	(IsFreeable): Define.
	(free): Delete for non-freeable types.
	(xnew): New.
	(XNEW): Undef and redefine.
	(xcnew): New.
	(XCNEW): Undef and redefine.
	(xdelete): New.
	(XDELETE): Undef and redefine.
	(xnewvec): New.
	(XNEWVEC): Undef and redefine.
	(xcnewvec): New.
	(XCNEWVEC): Undef and redefine.
	(xresizevec): New.
	(XRESIZEVEC): Undef and redefine.
	(xdeletevec): New.
	(XDELETEVEC): Undef and redefine.
	(xnewvar): New.
	(XNEWVAR): Undef and redefine.
	(xcnewvar): New.
	(XCNEWVAR): Undef and redefine.
	(xresizevar): New.
	(XRESIZEVAR): Undef and redefine.
---
 gdb/common/common-utils.c |   7 ---
 gdb/common/common-utils.h |  14 ++++-
gdb/common/poison.h | 132 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+), 8 deletions(-)

diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 7139302..66d6161 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -95,13 +95,6 @@ xzalloc (size_t size)
 }

 void
-xfree (void *ptr)
-{
-  if (ptr != NULL)
-    free (ptr);		/* ARI: free */
-}
-
-void
 xmalloc_failed (size_t size)
 {
   malloc_failure (size);
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 4926a32..feb4790 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -23,6 +23,8 @@
 #include <string>
 #include <vector>

+#include "poison.h"
+
 /* If possible, define FUNCTION_NAME, a macro containing the name of
    the function being defined.  Since this macro may not always be
defined, all uses must be protected by appropriate macro definition
@@ -47,7 +49,17 @@
 /* Like xmalloc, but zero the memory.  */
 void *xzalloc (size_t);

-void xfree (void *);
+template <typename T>
+static void
+xfree (T *ptr)
+{
+ static_assert (IsFreeable<T>::value, "Trying to use xfree with a non-POD \
+data type.  Use operator delete instead.");
+
+  if (ptr != NULL)
+    free (ptr);		/* ARI: free */
+}
+

 /* Like asprintf and vasprintf, but return the string, throw an error
    if no memory.  */
diff --git a/gdb/common/poison.h b/gdb/common/poison.h
index 37dd35e..de4cefa 100644
--- a/gdb/common/poison.h
+++ b/gdb/common/poison.h
@@ -84,4 +84,136 @@ void *memmove (D *dest, const S *src, size_t n) = delete;

 #endif /* HAVE_IS_TRIVIALLY_COPYABLE */

+/* Poison XNEW and friends to catch usages of malloc-style allocations on
+   objects that require new/delete.  */
+
+template<typename T>
+using IsMallocatable = std::is_pod<T>;
+
+template<typename T>
+using IsFreeable = gdb::Or<std::is_trivially_destructible<T>, std::is_void<T>>;
+
+template <typename T, typename = gdb::Requires<gdb::Not<IsFreeable<T>>>>
+void free (T *ptr) = delete;
+
+template<typename T>
+static T *
+xnew ()
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XNEW with a non-POD \
+data type.  Use operator new instead.");
+  return XNEW (T);
+}
+
+#undef XNEW
+#define XNEW(T) xnew<T>()
+
+template<typename T>
+static T *
+xcnew ()
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XCNEW with a non-POD \
+data type.  Use operator new instead.");
+  return XCNEW (T);
+}
+
+#undef XCNEW
+#define XCNEW(T) xcnew<T>()
+
+template<typename T>
+static void
+xdelete (T *p)
+{
+ static_assert (IsFreeable<T>::value, "Trying to use XDELETE with a non-POD \
+data type.  Use operator delete instead.");
+  XDELETE (p);
+}
+
+#undef XDELETE
+#define XDELETE(P) xdelete (p)
+
+template<typename T>
+static T*

Missing space after T in several of these functions.

Oops, thanks.

+xnewvec (size_t n)
+{
+ static_assert (IsMallocatable<T>::value, "Trying to use XNEWVEC with a \
+non-POD data type.  Use operator new[] (or std::vector) instead.");
+  return XNEWVEC (T, n);
+}
+

Other than the formatting, this LGTM.  Thanks a lot for doing this.

Thanks, I'm glad that I won't have to carry that branch on the side anymore :)

Simon


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]