[PATCH 0/4] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[PATCH 0/4] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

Here's a version of the hack at
<https://sourceware.org/ml/gdb-patches/2017-04/msg00357.html> that I'm
not shy about proposing to master.

The original idea was to catch invalid initialization of non-POD types
with memset, at compile time.  After posting the above, I thought it'd
be good to catch invalid uses of memcpy/memmove too, in a similar way.

Unlike the version linked above, this version does not use macros.
Instead it simply defines deleted memset/memcpy/memmove overloads.
Yay C++11!

Patch #1 adds the poisoning of invalid memset/memcpy/memmove.

Patches 2-4^W 2-5 [1] fix problems the poisoning detected, except the
"struct inferior", which is already fixed by this other series:
https://sourceware.org/ml/gdb-patches/2017-04/msg00298.html

[1] another instance just made it into the tree, so I do think it's a
good idea to add the poisoning to master.

Tested on x86-64 Fedora 23, along with the struct inferior fix.

Pedro Alves (5):
  Poison non-POD memset & non-trivially-copyable memcpy/memmove
  Don't memcpy non-trivially-copyable types: Make enum_flags triv.
    copyable
  Don't memset non-POD types: struct bp_location
  Don't memset non-POD types: struct btrace_insn
  Don't memset non-POD types: struct breakpoint

 gdb/ada-lang.c             |  19 +++----
 gdb/breakpoint.c           |  23 ++-------
 gdb/breakpoint.h           | 125 +++++++++++++++++++++++----------------------
 gdb/btrace.c               |  19 ++++---
 gdb/common/common-defs.h   |   1 +
 gdb/common/enum-flags.h    |  15 +++---
 gdb/common/function-view.h |  40 ++-------------
 gdb/common/poison.h        |  83 ++++++++++++++++++++++++++++++
 gdb/common/traits.h        |  55 ++++++++++++++++++++
 9 files changed, 234 insertions(+), 146 deletions(-)
 create mode 100644 gdb/common/poison.h

-- 
2.5.5

[PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable

[Pedro Alves]

The delete-memcpy-with-non-trivial-types patch exposed many instances
of this problem:

  src/gdb/btrace.h: In function ‘btrace_insn_s* VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const btrace_insn_s*, const char*, unsigned int)’:
  src/gdb/common/vec.h:948:62: error: use of deleted function ‘void* memmove(T*, const U*, size_t) [with T = btrace_insn; U = btrace_insn; <template-parameter-1-3> = void; size_t = long unsigned int]’
     memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T));    \
								^
  src/gdb/common/vec.h:436:1: note: in expansion of macro ‘DEF_VEC_FUNC_O’
   DEF_VEC_FUNC_O(T)         \
   ^
  src/gdb/btrace.h:84:1: note: in expansion of macro ‘DEF_VEC_O’
   DEF_VEC_O (btrace_insn_s);
   ^
[...]
  src/gdb/common/vec.h:1060:31: error: use of deleted function ‘void* memcpy(T*, const U*, size_t) [with T = btrace_insn; U = btrace_insn; <template-parameter-1-3> = void; size_t = long unsigned int]’
	  sizeof (T) * vec2_->num);       \
				 ^
  src/gdb/common/vec.h:437:1: note: in expansion of macro ‘DEF_VEC_ALLOC_FUNC_O’
   DEF_VEC_ALLOC_FUNC_O(T)         \
   ^
  src/gdb/btrace.h:84:1: note: in expansion of macro ‘DEF_VEC_O’
   DEF_VEC_O (btrace_insn_s);
   ^

So, VECs (given it's C roots) rely on memcpy/memcpy of VEC elements to
be well defined, in order to grow/reallocate its internal elements
array.  This means that we can only put trivially copyable types in
VECs.  E.g., if a type requires using a custom copy/move ctor to
relocate, then we can't put it in a VEC (so we use std::vector
instead).  But, as shown above, we're violating that requirement.

btrace_insn is currently not trivially copyable, because it contains
an enum_flags field, and that is itself not trivially copyable.  This
patch corrects that.

Note that std::vector relies on std::is_trivially_copyable too to know
whether it can reallocate its elements with memcpy/memmove instead of
having to call copy/move ctors and dtors, so if we have types in
std::vectors that weren't trivially copyable because of enum_flags,
this will make such vectors more efficient.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* common/enum-flags.h (enum_flags): Define copy/move ctors/op= as
	defaulted.
---
 gdb/common/enum-flags.h | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/gdb/common/enum-flags.h b/gdb/common/enum-flags.h
index e63c8a4..bea0ad5 100644
--- a/gdb/common/enum-flags.h
+++ b/gdb/common/enum-flags.h
@@ -120,15 +120,12 @@ public:
     : m_enum_value ((enum_type) 0)
   {}
 
-  enum_flags (const enum_flags &other)
-    : m_enum_value (other.m_enum_value)
-  {}
-
-  enum_flags &operator= (const enum_flags &other)
-  {
-    m_enum_value = other.m_enum_value;
-    return *this;
-  }
+  /* Define copy/move ctor/op= as defaulted so that enum_flags is
+     trivially copyable.  */
+  enum_flags (const enum_flags &other) = default;
+  enum_flags (enum_flags &&) noexcept = default;
+  enum_flags &operator= (const enum_flags &other) = default;
+  enum_flags &operator= (enum_flags &&) = default;
 
   /* If you get an error saying these two overloads are ambiguous,
      then you tried to mix values of different enum types.  */
-- 
2.5.5

[PATCH 3/5] Don't memset non-POD types: struct bp_location

[Pedro Alves]

struct bp_location is not a POD, so we shouldn't be using memset to
initialize it.

Caught like this:

  src/gdb/breakpoint.c: In function ‘bp_location** get_first_locp_gte_addr(CORE_ADDR)’:
  src/gdb/breakpoint.c:950:53: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = bp_location; <template-parameter-1-2> = void; size_t = long unsigned int]’
     memset (&dummy_loc, 0, sizeof (struct bp_location));
						       ^
  In file included from src/gdb/defs.h:28:0,
		   from src/gdb/breakpoint.c:20:
  src/gdb/common/common-defs.h:126:7: note: declared here
   void *memset (T *s, int c, size_t n) = delete;
	 ^

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* ada-lang.c (ada_catchpoint_location): Now a "class".  Remove
	"base" field and inherit from "bp_location" instead.  Add
	non-default ctor.
	(allocate_location_exception): Use new non-default ctor.
	* breakpoint.c (get_first_locp_gte_addr): Remove memset call.
	(init_bp_location): Convert to ...
	(bp_location::bp_location): ... this new ctor, and remove memset
	call.
	(base_breakpoint_allocate_location): Use the new non-default ctor.
	* breakpoint.h (bp_location): Now a class.  Declare default and
	non-default ctors.  In-class initialize all members.
	(init_bp_location): Remove declaration.
---
 gdb/ada-lang.c   | 19 ++++++-----------
 gdb/breakpoint.c | 13 +++---------
 gdb/breakpoint.h | 65 ++++++++++++++++++++++++++++----------------------------
 3 files changed, 43 insertions(+), 54 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 2e5643b..58c8a2e 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12224,14 +12224,14 @@ static char *ada_exception_catchpoint_cond_string (const char *excep_string);
    when symbols change.  */
 
 /* An instance of this type is used to represent an Ada catchpoint
-   breakpoint location.  It includes a "struct bp_location" as a kind
-   of base class; users downcast to "struct bp_location *" when
-   needed.  */
+   breakpoint location.  */
 
-struct ada_catchpoint_location
+class ada_catchpoint_location : public bp_location
 {
-  /* The base class.  */
-  struct bp_location base;
+public:
+  ada_catchpoint_location (const bp_location_ops *ops, breakpoint *owner)
+    : bp_location (ops, owner)
+  {}
 
   /* The condition that checks whether the exception that was raised
      is the specific exception the user specified on catchpoint
@@ -12347,12 +12347,7 @@ static struct bp_location *
 allocate_location_exception (enum ada_exception_catchpoint_kind ex,
 			     struct breakpoint *self)
 {
-  struct ada_catchpoint_location *loc;
-
-  loc = new ada_catchpoint_location ();
-  init_bp_location (&loc->base, &ada_catchpoint_location_ops, self);
-  loc->excep_cond_expr = NULL;
-  return &loc->base;
+  return new ada_catchpoint_location (&ada_catchpoint_location_ops, self);
 }
 
 /* Implement the RE_SET method in the breakpoint_ops structure for all
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ab0c82f..0796313 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -947,7 +947,6 @@ get_first_locp_gte_addr (CORE_ADDR address)
   struct bp_location **locp_found = NULL;
 
   /* Initialize the dummy location's address field.  */
-  memset (&dummy_loc, 0, sizeof (struct bp_location));
   dummy_loc.address = address;
 
   /* Find a close match to the first location at ADDRESS.  */
@@ -7316,11 +7315,9 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
     }
 }
 
-void
-init_bp_location (struct bp_location *loc, const struct bp_location_ops *ops,
-		  struct breakpoint *owner)
+bp_location::bp_location (const bp_location_ops *ops, breakpoint *owner)
 {
-  memset (loc, 0, sizeof (*loc));
+  bp_location *loc = this;
 
   gdb_assert (ops != NULL);
 
@@ -12840,11 +12837,7 @@ base_breakpoint_dtor (struct breakpoint *self)
 static struct bp_location *
 base_breakpoint_allocate_location (struct breakpoint *self)
 {
-  struct bp_location *loc;
-
-  loc = new struct bp_location ();
-  init_bp_location (loc, &bp_location_ops, self);
-  return loc;
+  return new bp_location (&bp_location_ops, self);
 }
 
 static void
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 9b5dc3f..18b284f 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -310,20 +310,25 @@ struct bp_location_ops
   void (*dtor) (struct bp_location *self);
 };
 
-struct bp_location
+class bp_location
 {
+public:
+  bp_location () = default;
+
+  bp_location (const bp_location_ops *ops, breakpoint *owner);
+
   /* Chain pointer to the next breakpoint location for
      the same parent breakpoint.  */
-  struct bp_location *next;
+  bp_location *next = NULL;
 
   /* Methods associated with this location.  */
-  const struct bp_location_ops *ops;
+  const bp_location_ops *ops = NULL;
 
   /* The reference count.  */
-  int refc;
+  int refc = 0;
 
   /* Type of this breakpoint location.  */
-  enum bp_loc_type loc_type;
+  bp_loc_type loc_type {};
 
   /* Each breakpoint location must belong to exactly one higher-level
      breakpoint.  This pointer is NULL iff this bp_location is no
@@ -331,7 +336,7 @@ struct bp_location
      is deleted, its locations may still be found in the
      moribund_locations list, or if we had stopped for it, in
      bpstats.  */
-  struct breakpoint *owner;
+  breakpoint *owner = NULL;
 
   /* Conditional.  Break only if this expression's value is nonzero.
      Unlike string form of condition, which is associated with
@@ -360,32 +365,32 @@ struct bp_location
      duplicates of this location and thus we don't need to call
      force_breakpoint_reinsertion (...) for this location.  */
 
-  enum condition_status condition_changed;
+  condition_status condition_changed {};
 
   agent_expr_up cmd_bytecode;
 
   /* Signals that breakpoint conditions and/or commands need to be
      re-synched with the target.  This has no use other than
      target-side breakpoints.  */
-  char needs_update;
+  bool needs_update = false;
 
   /* This location's address is in an unloaded solib, and so this
      location should not be inserted.  It will be automatically
      enabled when that solib is loaded.  */
-  char shlib_disabled; 
+  bool shlib_disabled = false;
 
   /* Is this particular location enabled.  */
-  char enabled;
+  bool enabled = false;
   
   /* Nonzero if this breakpoint is now inserted.  */
-  char inserted;
+  bool inserted = false;
 
   /* Nonzero if this is a permanent breakpoint.  There is a breakpoint
      instruction hard-wired into the target's code.  Don't try to
      write another breakpoint instruction on top of it, or restore its
      value.  Step over it using the architecture's
      gdbarch_skip_permanent_breakpoint method.  */
-  char permanent;
+  bool permanent = false;
 
   /* Nonzero if this is not the first breakpoint in the list
      for the given address.  location of tracepoint can _never_
@@ -393,7 +398,7 @@ struct bp_location
      kinds of breakpoints, because two locations at the same
      address may have different actions, so both of these locations
      should be downloaded and so that `tfind N' always works.  */
-  char duplicate;
+  bool duplicate = false;
 
   /* If we someday support real thread-specific breakpoints, then
      the breakpoint location will need a thread identifier.  */
@@ -403,7 +408,7 @@ struct bp_location
 
   /* Architecture associated with this location's address.  May be
      different from the breakpoint architecture.  */
-  struct gdbarch *gdbarch;
+  struct gdbarch *gdbarch = NULL;
 
   /* The program space associated with this breakpoint location
      address.  Note that an address space may be represented in more
@@ -411,26 +416,26 @@ struct bp_location
      its own program space, but there will only be one address space
      for all of them), but we must not insert more than one location
      at the same address in the same address space.  */
-  struct program_space *pspace;
+  program_space *pspace = NULL;
 
   /* Note that zero is a perfectly valid code address on some platforms
      (for example, the mn10200 (OBSOLETE) and mn10300 simulators).  NULL
      is not a special value for this field.  Valid for all types except
      bp_loc_other.  */
-  CORE_ADDR address;
+  CORE_ADDR address = 0;
 
   /* For hardware watchpoints, the size of the memory region being
      watched.  For hardware ranged breakpoints, the size of the
      breakpoint range.  */
-  int length;
+  int length = 0;
 
   /* Type of hardware watchpoint.  */
-  enum target_hw_bp_type watchpoint_type;
+  target_hw_bp_type watchpoint_type {};
 
   /* For any breakpoint type with an address, this is the section
      associated with the address.  Used primarily for overlay
      debugging.  */
-  struct obj_section *section;
+  obj_section *section = NULL;
 
   /* Address at which breakpoint was requested, either by the user or
      by GDB for internal breakpoints.  This will usually be the same
@@ -438,24 +443,24 @@ struct bp_location
      ADJUST_BREAKPOINT_ADDRESS has computed a different address at
      which to place the breakpoint in order to comply with a
      processor's architectual constraints.  */
-  CORE_ADDR requested_address;
+  CORE_ADDR requested_address = 0;
 
   /* An additional address assigned with this location.  This is currently
      only used by STT_GNU_IFUNC resolver breakpoints to hold the address
      of the resolver function.  */
-  CORE_ADDR related_address;
+  CORE_ADDR related_address = 0;
 
   /* If the location comes from a probe point, this is the probe associated
      with it.  */
-  struct bound_probe probe;
+  bound_probe probe {};
 
-  char *function_name;
+  char *function_name = NULL;
 
   /* Details of the placed breakpoint, when inserted.  */
-  struct bp_target_info target_info;
+  bp_target_info target_info {};
 
   /* Similarly, for the breakpoint at an overlay's LMA, if necessary.  */
-  struct bp_target_info overlay_target_info;
+  bp_target_info overlay_target_info {};
 
   /* In a non-stop mode, it's possible that we delete a breakpoint,
      but as we do that, some still running thread hits that breakpoint.
@@ -466,19 +471,19 @@ struct bp_location
      breakpoint was deleted, we retire all locations of that breakpoint.
      This variable keeps a number of events still to go, when
      it becomes 0 this location is retired.  */
-  int events_till_retirement;
+  int events_till_retirement = 0;
 
   /* Line number which was used to place this location.
 
      Breakpoint placed into a comment keeps it's user specified line number
      despite ADDRESS resolves into a different line number.  */
 
-  int line_number;
+  int line_number = 0;
 
   /* Symtab which was used to place this location.  This is used
      to find the corresponding source file name.  */
 
-  struct symtab *symtab;
+  struct symtab *symtab = NULL;
 };
 
 /* The possible return values for print_bpstat, print_it_normal,
@@ -1205,10 +1210,6 @@ extern void until_break_command (char *, int, int);
 
 /* Initialize a struct bp_location.  */
 
-extern void init_bp_location (struct bp_location *loc,
-			      const struct bp_location_ops *ops,
-			      struct breakpoint *owner);
-
 extern void update_breakpoint_locations (struct breakpoint *b,
 					 struct program_space *filter_pspace,
 					 struct symtabs_and_lines sals,
-- 
2.5.5

[PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

This patch catches invalid initialization of non-POD types with
memset, at compile time.

This is what I used to catch the problems fixed by the rest of the
series:

  $ make -k 2>&1 | grep "deleted function"
  src/gdb/breakpoint.c:951:53: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = bp_location; <template-parameter-1-2> = void; size_t = long unsigned int]’
  src/gdb/breakpoint.c:7325:32: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = bp_location; <template-parameter-1-2> = void; size_t = long unsigned int]’
  src/gdb/btrace.c:1153:42: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = btrace_insn; <template-parameter-1-2> = void; size_t = long unsigned int]’

I'll move this to the end of the series before pushing (if agreed).

(I've posted another series recently that adds some of the same traits
bits to common/traits.h.  They're really useful.)

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* common/common-defs.h: Include "common/poison.h".
	* common/function-view.h: (Not, Or, Requires): Move to traits.h.
	* common/poison.h: New file.
	* common/traits.h: Include <type_traits>.
	(Not, Or, Requires): New, moved from common/function-view.h.
	(And): New.
---
 gdb/common/common-defs.h   |  1 +
 gdb/common/function-view.h | 40 +++-------------------
 gdb/common/poison.h        | 83 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/common/traits.h        | 55 ++++++++++++++++++++++++++++++
 4 files changed, 143 insertions(+), 36 deletions(-)
 create mode 100644 gdb/common/poison.h

diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index af37111..439bce6 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -82,6 +82,7 @@
 #include "common-debug.h"
 #include "cleanups.h"
 #include "common-exceptions.h"
+#include "common/poison.h"
 
 #define EXTERN_C extern "C"
 #define EXTERN_C_PUSH extern "C" {
diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h
index 66a691b..d4ff2f0 100644
--- a/gdb/common/function-view.h
+++ b/gdb/common/function-view.h
@@ -153,34 +153,6 @@
 
 namespace gdb {
 
-namespace traits {
-  /* A few trait helpers.  */
-  template<typename Predicate>
-  struct Not : public std::integral_constant<bool, !Predicate::value>
-  {};
-
-  template<typename...>
-  struct Or;
-
-  template<>
-  struct Or<> : public std::false_type
-  {};
-
-  template<typename B1>
-  struct Or<B1> : public B1
-  {};
-
-  template<typename B1, typename B2>
-  struct Or<B1, B2>
-    : public std::conditional<B1::value, B1, B2>::type
-  {};
-
-  template<typename B1,typename B2,typename B3, typename... Bn>
-  struct Or<B1, B2, B3, Bn...>
-    : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type
-  {};
-} /* namespace traits */
-
 namespace fv_detail {
 /* Bits shared by all function_view instantiations that do not depend
    on the template parameters.  */
@@ -209,9 +181,9 @@ class function_view<Res (Args...)>
 {
   template<typename From, typename To>
   using CompatibleReturnType
-    = traits::Or<std::is_void<To>,
-		 std::is_same<From, To>,
-		 std::is_convertible<From, To>>;
+    = gdb::Or<std::is_void<To>,
+	      std::is_same<From, To>,
+	      std::is_convertible<From, To>>;
 
   /* True if Func can be called with Args, and either the result is
      Res, convertible to Res or Res is void.  */
@@ -227,10 +199,6 @@ class function_view<Res (Args...)>
     : std::is_same<function_view, typename std::decay<Callable>::type>
   {};
 
-  /* Helper to make SFINAE logic easier to read.  */
-  template<typename Condition>
-  using Requires = typename std::enable_if<Condition::value, void>::type;
-
  public:
 
   /* NULL by default.  */
@@ -248,7 +216,7 @@ class function_view<Res (Args...)>
      compatible.  */
   template
     <typename Callable,
-     typename = Requires<traits::Not<IsFunctionView<Callable>>>,
+     typename = Requires<gdb::Not<IsFunctionView<Callable>>>,
      typename = Requires<IsCompatibleCallable<Callable>>>
   function_view (Callable &&callable) noexcept
   {
diff --git a/gdb/common/poison.h b/gdb/common/poison.h
new file mode 100644
index 0000000..57a1733
--- /dev/null
+++ b/gdb/common/poison.h
@@ -0,0 +1,83 @@
+/* Poison symbols at compile time.
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_POISON_H
+#define COMMON_POISON_H
+
+#include "traits.h"
+
+/* Poison memset of non-POD types.  The idea is catching invalid
+   initialization of non-POD structs that is easy to be introduced as
+   side effect of refactoring.  For example, say this:
+
+ struct S { VEC(foo_s) *m_data; };
+
+is converted to this at some point:
+
+ struct S {
+   S() { m_data.reserve (10); }
+   std::vector<foo> m_data;
+ };
+
+and old code was initializing B objects like this:
+
+ struct B b;
+ memset (&b, 0, sizeof (B)); // whoops, now wipes vector.
+
+Declaring memset as deleted for non-POD types makes the memset above
+be a compile-time error.  */
+
+/* Helper for SFINAE.  True if "T *" is memsettable.  I.e., if T is
+   either void, or POD.  */
+template<typename T>
+struct IsMemsettable
+  : gdb::Or<std::is_void<T>,
+	    std::is_pod<T>>
+{};
+
+template <typename T,
+	  typename = gdb::Requires<gdb::Not<IsMemsettable<T>>>>
+void *memset (T *s, int c, size_t n) = delete;
+
+/* Similarly, poison memcpy and memmove of non trivially-copyable
+   types, which is undefined.  */
+
+/* True if "T *" is relocatable.  I.e., copyable with memcpy/memmove.
+   I.e., T is either trivially copyable, or void.  */
+template<typename T>
+struct IsRelocatable
+  : gdb::Or<std::is_void<T>,
+	    std::is_trivially_copyable<T>>
+{};
+
+/* True if both source and destination are relocatable.  */
+
+template <typename D, typename S>
+using BothAreRelocatable
+  = gdb::And<IsRelocatable<D>, IsRelocatable<S>>;
+
+template <typename D, typename S,
+	  typename = gdb::Requires<gdb::Not<BothAreRelocatable<D, S>>>>
+void *memcpy (D *dest, const S *src, size_t n) = delete;
+
+template <typename D, typename S,
+	  typename = gdb::Requires<gdb::Not<BothAreRelocatable<D, S>>>>
+void *memmove (D *dest, const S *src, size_t n) = delete;
+
+#endif /* COMMON_POISON_H */
diff --git a/gdb/common/traits.h b/gdb/common/traits.h
index 4b7bac3..1ce2327 100644
--- a/gdb/common/traits.h
+++ b/gdb/common/traits.h
@@ -18,6 +18,8 @@
 #ifndef COMMON_TRAITS_H
 #define COMMON_TRAITS_H
 
+#include <type_traits>
+
 namespace gdb {
 
 /* Pre C++14-safe (CWG 1558) version of C++17's std::void_t.  See
@@ -29,6 +31,59 @@ struct make_void { typedef void type; };
 template<typename... Ts>
 using void_t = typename make_void<Ts...>::type;
 
+/* A few trait helpers, mainly stolen from libstdc++.  Uppercase
+   because "and/or", etc. are reserved keywords.  */
+
+template<typename Predicate>
+struct Not : public std::integral_constant<bool, !Predicate::value>
+{};
+
+template<typename...>
+struct Or;
+
+template<>
+struct Or<> : public std::false_type
+{};
+
+template<typename B1>
+struct Or<B1> : public B1
+{};
+
+template<typename B1, typename B2>
+struct Or<B1, B2>
+  : public std::conditional<B1::value, B1, B2>::type
+{};
+
+template<typename B1,typename B2,typename B3, typename... Bn>
+struct Or<B1, B2, B3, Bn...>
+  : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type
+{};
+
+template<typename...>
+struct And;
+
+template<>
+struct And<> : public std::true_type
+{};
+
+template<typename B1>
+struct And<B1> : public B1
+{};
+
+template<typename B1, typename B2>
+struct And<B1, B2>
+  : public std::conditional<B1::value, B2, B1>::type
+{};
+
+template<typename B1, typename B2, typename B3, typename... Bn>
+struct And<B1, B2, B3, Bn...>
+  : public std::conditional<B1::value, And<B2, B3, Bn...>, B1>::type
+{};
+
+/* Helper to make SFINAE logic easier to read.  */
+template<typename Condition>
+using Requires = typename std::enable_if<Condition::value, void>::type;
+
 }
 
 #endif /* COMMON_TRAITS_H */
-- 
2.5.5

[PATCH 4/5] Don't memset non-POD types: struct btrace_insn

[Pedro Alves]

struct btrace_insn is not a POD [1] so we shouldn't be using memset to
initialize it [2].

Use list-initialization instead, wrapped in a "pt insn to btrace insn"
function, which looks like just begging to be added next to the
existing pt_reclassify_insn/pt_btrace_insn_flags functions.

[1] - because its field "flags" is not POD, because enum_flags has a
non-trivial default ctor.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* btrace.c (pt_btrace_insn): New function.
	(ftrace_add_pt): Remove memset call and use pt_btrace_insn.
---
 gdb/btrace.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 95dc7ab..30359b6 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1122,6 +1122,17 @@ pt_btrace_insn_flags (const struct pt_insn *insn)
   return flags;
 }
 
+/* Return the btrace instruction for INSN.  */
+
+static btrace_insn
+pt_btrace_insn (const struct pt_insn &insn)
+{
+  return {(CORE_ADDR) insn.ip, (gdb_byte) insn.size,
+	  pt_reclassify_insn (insn.iclass),
+	  pt_btrace_insn_flags (&insn)};
+}
+
+
 /* Add function branch trace using DECODER.  */
 
 static void
@@ -1138,7 +1149,6 @@ ftrace_add_pt (struct pt_insn_decoder *decoder,
   end = *pend;
   for (;;)
     {
-      struct btrace_insn btinsn;
       struct pt_insn insn;
 
       errcode = pt_insn_sync_forward (decoder);
@@ -1150,7 +1160,6 @@ ftrace_add_pt (struct pt_insn_decoder *decoder,
 	  break;
 	}
 
-      memset (&btinsn, 0, sizeof (btinsn));
       for (;;)
 	{
 	  errcode = pt_insn_next (decoder, &insn, sizeof(insn));
@@ -1207,11 +1216,7 @@ ftrace_add_pt (struct pt_insn_decoder *decoder,
 	  /* Maintain the function level offset.  */
 	  *plevel = std::min (*plevel, end->level);
 
-	  btinsn.pc = (CORE_ADDR) insn.ip;
-	  btinsn.size = (gdb_byte) insn.size;
-	  btinsn.iclass = pt_reclassify_insn (insn.iclass);
-	  btinsn.flags = pt_btrace_insn_flags (&insn);
-
+	  btrace_insn btinsn = pt_btrace_insn (insn);
 	  ftrace_update_insns (end, &btinsn);
 	}
 
-- 
2.5.5

[PATCH 5/5] Don't memset non-POD types: struct breakpoint

[Pedro Alves]

Eh, struct breakpoint was made non-POD just today, with commit
d28cd78ad820e3 ("Change breakpoint event locations to
event_location_up").  :-)

  src/gdb/breakpoint.c: In function ‘void init_raw_breakpoint_without_location(breakpoint*, gdbarch*, bptype, const breakpoint_ops*)’:
  src/gdb/breakpoint.c:7447:28: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = breakpoint; <template-parameter-1-2> = void; size_t = long unsigned int]’
     memset (b, 0, sizeof (*b));
			      ^
  In file included from src/gdb/common/common-defs.h:85:0,
		   from src/gdb/defs.h:28,
		   from src/gdb/breakpoint.c:20:
  src/gdb/common/poison.h:56:7: note: declared here
   void *memset (T *s, int c, size_t n) = delete;
	 ^

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* breakpoint.h (struct breakpoint): In-class initialize all
	fields.  Make boolean fields "bool".
	* breakpoint.c (init_raw_breakpoint_without_location): Remove
	memset call and initializations no longer necessary.
---
 gdb/breakpoint.c | 10 ----------
 gdb/breakpoint.h | 60 ++++++++++++++++++++++++++++----------------------------
 2 files changed, 30 insertions(+), 40 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0796313..0e6aecc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7444,8 +7444,6 @@ init_raw_breakpoint_without_location (struct breakpoint *b,
 				      enum bptype bptype,
 				      const struct breakpoint_ops *ops)
 {
-  memset (b, 0, sizeof (*b));
-
   gdb_assert (ops != NULL);
 
   b->ops = ops;
@@ -7453,17 +7451,9 @@ init_raw_breakpoint_without_location (struct breakpoint *b,
   b->gdbarch = gdbarch;
   b->language = current_language->la_language;
   b->input_radix = input_radix;
-  b->thread = -1;
   b->enable_state = bp_enabled;
-  b->next = 0;
-  b->silent = 0;
-  b->ignore_count = 0;
-  b->commands = NULL;
   b->frame_id = null_frame_id;
-  b->condition_not_parsed = 0;
-  b->py_bp_object = NULL;
   b->related_breakpoint = b;
-  b->location = NULL;
 }
 
 /* Helper to set_raw_breakpoint below.  Creates a breakpoint
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 18b284f..ae84349 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -681,45 +681,45 @@ extern int target_exact_watchpoints;
 struct breakpoint
 {
   /* Methods associated with this breakpoint.  */
-  const struct breakpoint_ops *ops;
+  const breakpoint_ops *ops = NULL;
 
-  struct breakpoint *next;
+  breakpoint *next = NULL;
   /* Type of breakpoint.  */
-  enum bptype type;
+  bptype type {bp_none};
   /* Zero means disabled; remember the info but don't break here.  */
-  enum enable_state enable_state;
+  enum enable_state enable_state {bp_disabled};
   /* What to do with this breakpoint after we hit it.  */
-  enum bpdisp disposition;
+  bpdisp disposition {disp_del};
   /* Number assigned to distinguish breakpoints.  */
-  int number;
+  int number = 0;
 
   /* Location(s) associated with this high-level breakpoint.  */
-  struct bp_location *loc;
+  bp_location *loc = NULL;
 
-  /* Non-zero means a silent breakpoint (don't print frame info if we
-     stop here).  */
-  unsigned char silent;
-  /* Non-zero means display ADDR_STRING to the user verbatim.  */
-  unsigned char display_canonical;
+  /* True means a silent breakpoint (don't print frame info if we stop
+     here).  */
+  bool silent = false;
+  /* True means display ADDR_STRING to the user verbatim.  */
+  bool display_canonical = false;
   /* Number of stops at this breakpoint that should be continued
      automatically before really stopping.  */
-  int ignore_count;
+  int ignore_count = 0;
 
   /* Number of stops at this breakpoint before it will be
      disabled.  */
-  int enable_count;
+  int enable_count = 0;
 
   /* Chain of command lines to execute when this breakpoint is
      hit.  */
-  struct counted_command_line *commands;
+  counted_command_line *commands = NULL;
   /* Stack depth (address of frame).  If nonzero, break only if fp
      equals this.  */
-  struct frame_id frame_id;
+  struct frame_id frame_id {null_frame_id};
 
   /* The program space used to set the breakpoint.  This is only set
      for breakpoints which are specific to a program space; for
      non-thread-specific ordinary breakpoints this is NULL.  */
-  struct program_space *pspace;
+  program_space *pspace = NULL;
 
   /* Location we used to set the breakpoint.  */
   event_location_up location;
@@ -727,60 +727,60 @@ struct breakpoint
   /* The filter that should be passed to decode_line_full when
      re-setting this breakpoint.  This may be NULL, but otherwise is
      allocated with xmalloc.  */
-  char *filter;
+  char *filter = NULL;
 
   /* For a ranged breakpoint, the location we used to find the end of
      the range.  */
   event_location_up location_range_end;
 
   /* Architecture we used to set the breakpoint.  */
-  struct gdbarch *gdbarch;
+  struct gdbarch *gdbarch = NULL;
   /* Language we used to set the breakpoint.  */
-  enum language language;
+  enum language language {language_unknown};
   /* Input radix we used to set the breakpoint.  */
-  int input_radix;
+  int input_radix = 0;
   /* String form of the breakpoint condition (malloc'd), or NULL if
      there is no condition.  */
-  char *cond_string;
+  char *cond_string = NULL;
 
   /* String form of extra parameters, or NULL if there are none.
      Malloc'd.  */
-  char *extra_string;
+  char *extra_string = NULL;
 
   /* Holds the address of the related watchpoint_scope breakpoint when
      using watchpoints on local variables (might the concept of a
      related breakpoint be useful elsewhere, if not just call it the
      watchpoint_scope breakpoint or something like that.  FIXME).  */
-  struct breakpoint *related_breakpoint;
+  breakpoint *related_breakpoint = NULL;
 
   /* Thread number for thread-specific breakpoint, or -1 if don't
      care.  */
-  int thread;
+  int thread = -1;
 
   /* Ada task number for task-specific breakpoint, or 0 if don't
      care.  */
-  int task;
+  int task = 0;
 
   /* Count of the number of times this breakpoint was taken, dumped
      with the info, but not used for anything else.  Useful for seeing
      how many times you hit a break prior to the program aborting, so
      you can back up to just before the abort.  */
-  int hit_count;
+  int hit_count = 0;
 
   /* Is breakpoint's condition not yet parsed because we found no
      location initially so had no context to parse the condition
      in.  */
-  int condition_not_parsed;
+  int condition_not_parsed = 0;
 
   /* With a Python scripting enabled GDB, store a reference to the
      Python object that has been associated with this breakpoint.
      This is always NULL for a GDB that is not script enabled.  It can
      sometimes be NULL for enabled GDBs as not all breakpoint types
      are tracked by the scripting language API.  */
-  struct gdbpy_breakpoint_object *py_bp_object;
+  gdbpy_breakpoint_object *py_bp_object = NULL;
 
   /* Same as py_bp_object, but for Scheme.  */
-  struct gdbscm_breakpoint_object *scm_bp_object;
+  gdbscm_breakpoint_object *scm_bp_object = NULL;
 };
 
 /* An instance of this type is used to represent a watchpoint.  It
-- 
2.5.5

RE: [PATCH 4/5] Don't memset non-POD types: struct btrace_insn

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Thursday, April 13, 2017 4:28 AM
> To: gdb-patches@sourceware.org
> Subject: [PATCH 4/5] Don't memset non-POD types: struct btrace_insn

Hello Pedro,


> +/* Return the btrace instruction for INSN.  */
> +
> +static btrace_insn
> +pt_btrace_insn (const struct pt_insn &insn)
> +{
> +  return {(CORE_ADDR) insn.ip, (gdb_byte) insn.size,
> +	  pt_reclassify_insn (insn.iclass),
> +	  pt_btrace_insn_flags (&insn)};
> +}

This is the only user of pt_btrace_insn_flags.  If you want, you may
change it to take a const & instead of a const *.

The patch looks good to me.

Regards,
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, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Simon Marchi]

On 2017-04-12 22:27, Pedro Alves wrote:
> This patch catches invalid initialization of non-POD types with
> memset, at compile time.
> 
> This is what I used to catch the problems fixed by the rest of the
> series:
> 
>   $ make -k 2>&1 | grep "deleted function"
>   src/gdb/breakpoint.c:951:53: error: use of deleted function ‘void*
> memset(T*, int, size_t) [with T = bp_location;
> <template-parameter-1-2> = void; size_t = long unsigned int]’
>   src/gdb/breakpoint.c:7325:32: error: use of deleted function ‘void*
> memset(T*, int, size_t) [with T = bp_location;
> <template-parameter-1-2> = void; size_t = long unsigned int]’
>   src/gdb/btrace.c:1153:42: error: use of deleted function ‘void*
> memset(T*, int, size_t) [with T = btrace_insn;
> <template-parameter-1-2> = void; size_t = long unsigned int]’
> 
> I'll move this to the end of the series before pushing (if agreed).
> 
> (I've posted another series recently that adds some of the same traits
> bits to common/traits.h.  They're really useful.)

That's really nice.  I'm actually surprised we didn't get random crashes 
because of that yet!

> diff --git a/gdb/common/poison.h b/gdb/common/poison.h
> new file mode 100644
> index 0000000..57a1733
> --- /dev/null
> +++ b/gdb/common/poison.h
> @@ -0,0 +1,83 @@
> +/* Poison symbols at compile time.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or 
> modify
> +   it under the terms of the GNU General Public License as published 
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef COMMON_POISON_H
> +#define COMMON_POISON_H
> +
> +#include "traits.h"
> +
> +/* Poison memset of non-POD types.  The idea is catching invalid
> +   initialization of non-POD structs that is easy to be introduced as
> +   side effect of refactoring.  For example, say this:
> +
> + struct S { VEC(foo_s) *m_data; };
> +
> +is converted to this at some point:
> +
> + struct S {
> +   S() { m_data.reserve (10); }
> +   std::vector<foo> m_data;
> + };

Here it says struct S ...

> +
> +and old code was initializing B objects like this:
> +
> + struct B b;
> + memset (&b, 0, sizeof (B)); // whoops, now wipes vector.

... and here struct B?

Simon

Re: [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable

[Simon Marchi]

On 2017-04-12 22:27, Pedro Alves wrote:
> The delete-memcpy-with-non-trivial-types patch exposed many instances
> of this problem:
> 
>   src/gdb/btrace.h: In function ‘btrace_insn_s*
> VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const
> btrace_insn_s*, const char*, unsigned int)’:
>   src/gdb/common/vec.h:948:62: error: use of deleted function ‘void*
> memmove(T*, const U*, size_t) [with T = btrace_insn; U = btrace_insn;
> <template-parameter-1-3> = void; size_t = long unsigned int]’
>      memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T));    \
> 								^
>   src/gdb/common/vec.h:436:1: note: in expansion of macro 
> ‘DEF_VEC_FUNC_O’
>    DEF_VEC_FUNC_O(T)         \
>    ^
>   src/gdb/btrace.h:84:1: note: in expansion of macro ‘DEF_VEC_O’
>    DEF_VEC_O (btrace_insn_s);
>    ^
> [...]
>   src/gdb/common/vec.h:1060:31: error: use of deleted function ‘void*
> memcpy(T*, const U*, size_t) [with T = btrace_insn; U = btrace_insn;
> <template-parameter-1-3> = void; size_t = long unsigned int]’
> 	  sizeof (T) * vec2_->num);       \
> 				 ^
>   src/gdb/common/vec.h:437:1: note: in expansion of macro 
> ‘DEF_VEC_ALLOC_FUNC_O’
>    DEF_VEC_ALLOC_FUNC_O(T)         \
>    ^
>   src/gdb/btrace.h:84:1: note: in expansion of macro ‘DEF_VEC_O’
>    DEF_VEC_O (btrace_insn_s);
>    ^
> 
> So, VECs (given it's C roots) rely on memcpy/memcpy of VEC elements to
> be well defined, in order to grow/reallocate its internal elements
> array.  This means that we can only put trivially copyable types in
> VECs.  E.g., if a type requires using a custom copy/move ctor to
> relocate, then we can't put it in a VEC (so we use std::vector
> instead).  But, as shown above, we're violating that requirement.
> 
> btrace_insn is currently not trivially copyable, because it contains
> an enum_flags field, and that is itself not trivially copyable.  This
> patch corrects that.
> 
> Note that std::vector relies on std::is_trivially_copyable too to know
> whether it can reallocate its elements with memcpy/memmove instead of
> having to call copy/move ctors and dtors, so if we have types in
> std::vectors that weren't trivially copyable because of enum_flags,
> this will make such vectors more efficient.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* common/enum-flags.h (enum_flags): Define copy/move ctors/op= as
> 	defaulted.
> ---
>  gdb/common/enum-flags.h | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/common/enum-flags.h b/gdb/common/enum-flags.h
> index e63c8a4..bea0ad5 100644
> --- a/gdb/common/enum-flags.h
> +++ b/gdb/common/enum-flags.h
> @@ -120,15 +120,12 @@ public:
>      : m_enum_value ((enum_type) 0)
>    {}
> 
> -  enum_flags (const enum_flags &other)
> -    : m_enum_value (other.m_enum_value)
> -  {}
> -
> -  enum_flags &operator= (const enum_flags &other)
> -  {
> -    m_enum_value = other.m_enum_value;
> -    return *this;
> -  }
> +  /* Define copy/move ctor/op= as defaulted so that enum_flags is
> +     trivially copyable.  */
> +  enum_flags (const enum_flags &other) = default;
> +  enum_flags (enum_flags &&) noexcept = default;
> +  enum_flags &operator= (const enum_flags &other) = default;
> +  enum_flags &operator= (enum_flags &&) = default;

What's the difference between defining these as default, and not 
defining them at all (which I assume will still make the compiler emit 
the default versions)?

Do you want to add a static_assert(is_pod...) for that class?  For now 
we have the VECs that will warn us if it becomes non-POD, but eventually 
they'll be gone.

Simon

Re: [PATCH 5/5] Don't memset non-POD types: struct breakpoint

[Simon Marchi]

On 2017-04-12 22:27, Pedro Alves wrote:
> Eh, struct breakpoint was made non-POD just today, with commit
> d28cd78ad820e3 ("Change breakpoint event locations to
> event_location_up").  :-)
> 
>   src/gdb/breakpoint.c: In function ‘void
> init_raw_breakpoint_without_location(breakpoint*, gdbarch*, bptype,
> const breakpoint_ops*)’:
>   src/gdb/breakpoint.c:7447:28: error: use of deleted function ‘void*
> memset(T*, int, size_t) [with T = breakpoint; <template-parameter-1-2>
> = void; size_t = long unsigned int]’
>      memset (b, 0, sizeof (*b));
> 			      ^
>   In file included from src/gdb/common/common-defs.h:85:0,
> 		   from src/gdb/defs.h:28,
> 		   from src/gdb/breakpoint.c:20:
>   src/gdb/common/poison.h:56:7: note: declared here
>    void *memset (T *s, int c, size_t n) = delete;
> 	 ^
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* breakpoint.h (struct breakpoint): In-class initialize all
> 	fields.  Make boolean fields "bool".
> 	* breakpoint.c (init_raw_breakpoint_without_location): Remove
> 	memset call and initializations no longer necessary.
> ---
>  gdb/breakpoint.c | 10 ----------
>  gdb/breakpoint.h | 60 
> ++++++++++++++++++++++++++++----------------------------
>  2 files changed, 30 insertions(+), 40 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 0796313..0e6aecc 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7444,8 +7444,6 @@ init_raw_breakpoint_without_location (struct
> breakpoint *b,
>  				      enum bptype bptype,
>  				      const struct breakpoint_ops *ops)
>  {
> -  memset (b, 0, sizeof (*b));
> -
>    gdb_assert (ops != NULL);
> 
>    b->ops = ops;
> @@ -7453,17 +7451,9 @@ init_raw_breakpoint_without_location (struct
> breakpoint *b,
>    b->gdbarch = gdbarch;
>    b->language = current_language->la_language;
>    b->input_radix = input_radix;
> -  b->thread = -1;
>    b->enable_state = bp_enabled;

Is there a reason you don't assign bp_enabled in-class directly?

> -  b->next = 0;
> -  b->silent = 0;
> -  b->ignore_count = 0;
> -  b->commands = NULL;
>    b->frame_id = null_frame_id;

I think you can remove the assignment to frame_id, since it's done 
in-class.

> -  b->condition_not_parsed = 0;
> -  b->py_bp_object = NULL;
>    b->related_breakpoint = b;
> -  b->location = NULL;
>  }
> 
>  /* Helper to set_raw_breakpoint below.  Creates a breakpoint
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 18b284f..ae84349 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -681,45 +681,45 @@ extern int target_exact_watchpoints;
>  struct breakpoint
>  {
>    /* Methods associated with this breakpoint.  */
> -  const struct breakpoint_ops *ops;
> +  const breakpoint_ops *ops = NULL;
> 
> -  struct breakpoint *next;
> +  breakpoint *next = NULL;
>    /* Type of breakpoint.  */
> -  enum bptype type;
> +  bptype type {bp_none};

For consistency, I think it would be nice to use = when possible

   bptype type = bp_none;

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Simon Marchi]

On 2017-04-12 22:27, Pedro Alves wrote:
> This patch catches invalid initialization of non-POD types with
> memset, at compile time.

Would it be possible to do something similar but to catch uses of 
XNEW/XCNEW with types that need new?  XNEW is defined as:

#define XNEW(T) ((T *) xmalloc (sizeof (T)))

I just tried this, and it seems to work well:

#define assert_pod(T) static_assert(std::is_pod<T>::value)

#undef XNEW
#define XNEW(T) ({ assert_pod(T); (T *) xmalloc (sizeof (T)); })
#undef XCNEW
#define XCNEW(T)  ({ assert_pod(T); (T *) xcalloc (1, sizeof (T)); })

assuming the compiler knows about statement expressions.

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Simon Marchi]

On 2017-04-23 21:12, Simon Marchi wrote:
> On 2017-04-12 22:27, Pedro Alves wrote:
>> This patch catches invalid initialization of non-POD types with
>> memset, at compile time.
> 
> Would it be possible to do something similar but to catch uses of
> XNEW/XCNEW with types that need new?  XNEW is defined as:
> 
> #define XNEW(T) ((T *) xmalloc (sizeof (T)))
> 
> I just tried this, and it seems to work well:
> 
> #define assert_pod(T) static_assert(std::is_pod<T>::value)
> 
> #undef XNEW
> #define XNEW(T) ({ assert_pod(T); (T *) xmalloc (sizeof (T)); })
> #undef XCNEW
> #define XCNEW(T)  ({ assert_pod(T); (T *) xcalloc (1, sizeof (T)); })
> 
> assuming the compiler knows about statement expressions.

Actually, it should probably use std::is_trivially_constructible.  And I 
suppose we could do the same with xfree, delete it when 
!std::is_trivially_destructible.

Re: [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable

[Pedro Alves]

On 04/20/2017 04:34 AM, Simon Marchi wrote:
> On 2017-04-12 22:27, Pedro Alves wrote:

>> +  /* Define copy/move ctor/op= as defaulted so that enum_flags is
>> +     trivially copyable.  */
>> +  enum_flags (const enum_flags &other) = default;
>> +  enum_flags (enum_flags &&) noexcept = default;
>> +  enum_flags &operator= (const enum_flags &other) = default;
>> +  enum_flags &operator= (enum_flags &&) = default;
> 
> What's the difference between defining these as default, and not
> defining them at all (which I assume will still make the compiler emit
> the default versions)?

Yeah, there's no difference.  I'll just remove them, since it clearly
confuses things otherwise.  Thanks.

> Do you want to add a static_assert(is_pod...) for that class?  For now
> we have the VECs that will warn us if it becomes non-POD, but eventually
> they'll be gone.

I do, but enum_flags is a template, and we need to add the assertion to
some template instantiation.  I'd like to add it in the enum
flags unit tests file, currently here [1]:

  https://github.com/palves/gdb/blob/palves/cxx11-enum-flags/gdb/enum-flags-selftests.c

which I need to rebase and repost.  I should be reposting that soon.

[1] - I'll move it to unittests/

Thanks,
Pedro Alves

From 23bcc18f470ee4364bd362a8b78c6c1415a9dadb Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Apr 2017 01:27:42 +0100
Subject: [PATCH] Don't memcpy non-trivially-copyable types: Make enum_flags
 triv. copyable
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The delete-memcpy-with-non-trivial-types patch exposed many instances
of this problem:

  src/gdb/btrace.h: In function ‘btrace_insn_s* VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const btrace_insn_s*, const char*, unsigned int)’:
  src/gdb/common/vec.h:948:62: error: use of deleted function ‘void* memmove(T*, const U*, size_t) [with T = btrace_insn; U = btrace_insn; <template-parameter-1-3> = void; size_t = long unsigned int]’
     memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T));    \
								^
  src/gdb/common/vec.h:436:1: note: in expansion of macro ‘DEF_VEC_FUNC_O’
   DEF_VEC_FUNC_O(T)         \
   ^
  src/gdb/btrace.h:84:1: note: in expansion of macro ‘DEF_VEC_O’
   DEF_VEC_O (btrace_insn_s);
   ^
[...]
  src/gdb/common/vec.h:1060:31: error: use of deleted function ‘void* memcpy(T*, const U*, size_t) [with T = btrace_insn; U = btrace_insn; <template-parameter-1-3> = void; size_t = long unsigned int]’
	  sizeof (T) * vec2_->num);       \
				 ^
  src/gdb/common/vec.h:437:1: note: in expansion of macro ‘DEF_VEC_ALLOC_FUNC_O’
   DEF_VEC_ALLOC_FUNC_O(T)         \
   ^
  src/gdb/btrace.h:84:1: note: in expansion of macro ‘DEF_VEC_O’
   DEF_VEC_O (btrace_insn_s);
   ^

So, VECs (given it's C roots) rely on memcpy/memcpy of VEC elements to
be well defined, in order to grow/reallocate its internal elements
array.  This means that we can only put trivially copyable types in
VECs.  E.g., if a type requires using a custom copy/move ctor to
relocate, then we can't put it in a VEC (so we use std::vector
instead).  But, as shown above, we're violating that requirement.

btrace_insn is currently not trivially copyable, because it contains
an enum_flags field, and that is itself not trivially copyable.  This
patch corrects that, by simply removing the user-provided copy
constructor and assignment operator.  The compiler-generated versions
work just fine.

Note that std::vector relies on std::is_trivially_copyable too to know
whether it can reallocate its elements with memcpy/memmove instead of
having to call copy/move ctors and dtors, so if we have types in
std::vectors that weren't trivially copyable because of enum_flags,
this will make such vectors more efficient.

gdb/ChangeLog:
2017-04-25  Pedro Alves  <palves@redhat.com>

	* common/enum-flags.h (enum_flags): Don't implement copy ctor and
	assignment operator.
---
 gdb/ChangeLog           |  5 +++++
 gdb/common/enum-flags.h | 10 ----------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 33dd53b..f5419db 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-04-25  Pedro Alves  <palves@redhat.com>
+
+	* common/enum-flags.h (enum_flags): Don't implement copy ctor and
+	assignment operator.
+
 2017-04-24  Yao Qi  <yao.qi@linaro.org>
 
 	* doublest.c (convert_doublest_to_floatformat): Call
diff --git a/gdb/common/enum-flags.h b/gdb/common/enum-flags.h
index e63c8a4..ddfcddf 100644
--- a/gdb/common/enum-flags.h
+++ b/gdb/common/enum-flags.h
@@ -120,16 +120,6 @@ public:
     : m_enum_value ((enum_type) 0)
   {}
 
-  enum_flags (const enum_flags &other)
-    : m_enum_value (other.m_enum_value)
-  {}
-
-  enum_flags &operator= (const enum_flags &other)
-  {
-    m_enum_value = other.m_enum_value;
-    return *this;
-  }
-
   /* If you get an error saying these two overloads are ambiguous,
      then you tried to mix values of different enum types.  */
   enum_flags (enum_type e)
-- 
2.5.5

Re: [PATCH 4/5] Don't memset non-POD types: struct btrace_insn

[Pedro Alves]

On 04/13/2017 08:53 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
>> owner@sourceware.org] On Behalf Of Pedro Alves
>> Sent: Thursday, April 13, 2017 4:28 AM
>> To: gdb-patches@sourceware.org
>> Subject: [PATCH 4/5] Don't memset non-POD types: struct btrace_insn
> 
> Hello Pedro,

Hi!

> 
>> +/* Return the btrace instruction for INSN.  */
>> +
>> +static btrace_insn
>> +pt_btrace_insn (const struct pt_insn &insn)
>> +{
>> +  return {(CORE_ADDR) insn.ip, (gdb_byte) insn.size,
>> +	  pt_reclassify_insn (insn.iclass),
>> +	  pt_btrace_insn_flags (&insn)};
>> +}
> 
> This is the only user of pt_btrace_insn_flags.  If you want, you may
> change it to take a const & instead of a const *.
> 
> The patch looks good to me.

Thanks Markus.  Here's what I pushed now, then.

From b5c3668253b909fd1f5b011893a35bb8dfd3be9b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Apr 2017 01:27:42 +0100
Subject: [PATCH] Don't memset non-POD types: struct btrace_insn

struct btrace_insn is not a POD [1] so we shouldn't be using memset to
initialize it [2].

Use list-initialization instead, wrapped in a "pt insn to btrace insn"
function, which looks like just begging to be added next to the
existing pt_reclassify_insn/pt_btrace_insn_flags functions.

[1] - because its field "flags" is not POD, because enum_flags has a
non-trivial default ctor.

gdb/ChangeLog:
2017-04-25  Pedro Alves  <palves@redhat.com>

	* btrace.c (pt_btrace_insn_flags): Change parameter type to
	reference.
	(pt_btrace_insn): New function.
	(ftrace_add_pt): Remove memset call and use pt_btrace_insn.
---
 gdb/ChangeLog |  7 +++++++
 gdb/btrace.c  | 23 ++++++++++++++---------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5df4a96..1b983b9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2017-04-25  Pedro Alves  <palves@redhat.com>
 
+	* btrace.c (pt_btrace_insn_flags): Change parameter type to
+	reference.
+	(pt_btrace_insn): New function.
+	(ftrace_add_pt): Remove memset call and use pt_btrace_insn.
+
+2017-04-25  Pedro Alves  <palves@redhat.com>
+
 	* ada-lang.c (ada_catchpoint_location): Now a "class".  Remove
 	"base" field and inherit from "bp_location" instead.  Add
 	non-default ctor.
diff --git a/gdb/btrace.c b/gdb/btrace.c
index 95dc7ab..238df0a 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1112,16 +1112,27 @@ pt_reclassify_insn (enum pt_insn_class iclass)
 /* Return the btrace instruction flags for INSN.  */
 
 static btrace_insn_flags
-pt_btrace_insn_flags (const struct pt_insn *insn)
+pt_btrace_insn_flags (const struct pt_insn &insn)
 {
   btrace_insn_flags flags = 0;
 
-  if (insn->speculative)
+  if (insn.speculative)
     flags |= BTRACE_INSN_FLAG_SPECULATIVE;
 
   return flags;
 }
 
+/* Return the btrace instruction for INSN.  */
+
+static btrace_insn
+pt_btrace_insn (const struct pt_insn &insn)
+{
+  return {(CORE_ADDR) insn.ip, (gdb_byte) insn.size,
+	  pt_reclassify_insn (insn.iclass),
+	  pt_btrace_insn_flags (insn)};
+}
+
+
 /* Add function branch trace using DECODER.  */
 
 static void
@@ -1138,7 +1149,6 @@ ftrace_add_pt (struct pt_insn_decoder *decoder,
   end = *pend;
   for (;;)
     {
-      struct btrace_insn btinsn;
       struct pt_insn insn;
 
       errcode = pt_insn_sync_forward (decoder);
@@ -1150,7 +1160,6 @@ ftrace_add_pt (struct pt_insn_decoder *decoder,
 	  break;
 	}
 
-      memset (&btinsn, 0, sizeof (btinsn));
       for (;;)
 	{
 	  errcode = pt_insn_next (decoder, &insn, sizeof(insn));
@@ -1207,11 +1216,7 @@ ftrace_add_pt (struct pt_insn_decoder *decoder,
 	  /* Maintain the function level offset.  */
 	  *plevel = std::min (*plevel, end->level);
 
-	  btinsn.pc = (CORE_ADDR) insn.ip;
-	  btinsn.size = (gdb_byte) insn.size;
-	  btinsn.iclass = pt_reclassify_insn (insn.iclass);
-	  btinsn.flags = pt_btrace_insn_flags (&insn);
-
+	  btrace_insn btinsn = pt_btrace_insn (insn);
 	  ftrace_update_insns (end, &btinsn);
 	}
 
-- 
2.5.5

Re: [PATCH 5/5] Don't memset non-POD types: struct breakpoint

[Pedro Alves]

On 04/20/2017 04:58 AM, Simon Marchi wrote:
>>
>>    b->ops = ops;
>> @@ -7453,17 +7451,9 @@ init_raw_breakpoint_without_location (struct
>> breakpoint *b,
>>    b->gdbarch = gdbarch;
>>    b->language = current_language->la_language;
>>    b->input_radix = input_radix;
>> -  b->thread = -1;
>>    b->enable_state = bp_enabled;
> 
> Is there a reason you don't assign bp_enabled in-class directly?
> 

Yes -- the reason is that I missed it.  :-)

> I think you can remove the assignment to frame_id, since it's done
> in-class.

Indeed.  Removed.

>> -  struct breakpoint *next;
>> +  breakpoint *next = NULL;
>>    /* Type of breakpoint.  */
>> -  enum bptype type;
>> +  bptype type {bp_none};
> 
> For consistency, I think it would be nice to use = when possible
> 
>   bptype type = bp_none;

Agreed.  I started out by writing "{}" in all the enums, to get
zero initialization without bothering to look up what the "zero"
enum value was, and then when I found it, I didn't think to use =...

Below's what I pushed.

Thanks,
Pedro Alves

From 16c4d54a71d8052988ed9c8005a03a7f934245f4 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Apr 2017 01:27:42 +0100
Subject: [PATCH] Don't memset non-POD types: struct breakpoint
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Eh, struct breakpoint was made non-POD just today, with commit
d28cd78ad820e3 ("Change breakpoint event locations to
event_location_up").  :-)

  src/gdb/breakpoint.c: In function ‘void init_raw_breakpoint_without_location(breakpoint*, gdbarch*, bptype, const breakpoint_ops*)’:
  src/gdb/breakpoint.c:7447:28: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = breakpoint; <template-parameter-1-2> = void; size_t = long unsigned int]’
     memset (b, 0, sizeof (*b));
			      ^
  In file included from src/gdb/common/common-defs.h:85:0,
		   from src/gdb/defs.h:28,
		   from src/gdb/breakpoint.c:20:
  src/gdb/common/poison.h:56:7: note: declared here
   void *memset (T *s, int c, size_t n) = delete;
	 ^

gdb/ChangeLog:
2017-04-25  Pedro Alves  <palves@redhat.com>

	* breakpoint.h (struct breakpoint): In-class initialize all
	fields.  Make boolean fields "bool".
	* breakpoint.c (init_raw_breakpoint_without_location): Remove
	memset call and initializations no longer necessary.
---
 gdb/ChangeLog    |  7 +++++++
 gdb/breakpoint.c | 12 ------------
 gdb/breakpoint.h | 60 ++++++++++++++++++++++++++++----------------------------
 3 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1b983b9..dc321a2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2017-04-25  Pedro Alves  <palves@redhat.com>
 
+	* breakpoint.h (struct breakpoint): In-class initialize all
+	fields.  Make boolean fields "bool".
+	* breakpoint.c (init_raw_breakpoint_without_location): Remove
+	memset call and initializations no longer necessary.
+
+2017-04-25  Pedro Alves  <palves@redhat.com>
+
 	* btrace.c (pt_btrace_insn_flags): Change parameter type to
 	reference.
 	(pt_btrace_insn): New function.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4db32ab..67d5988 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7428,8 +7428,6 @@ init_raw_breakpoint_without_location (struct breakpoint *b,
 				      enum bptype bptype,
 				      const struct breakpoint_ops *ops)
 {
-  memset (b, 0, sizeof (*b));
-
   gdb_assert (ops != NULL);
 
   b->ops = ops;
@@ -7437,17 +7435,7 @@ init_raw_breakpoint_without_location (struct breakpoint *b,
   b->gdbarch = gdbarch;
   b->language = current_language->la_language;
   b->input_radix = input_radix;
-  b->thread = -1;
-  b->enable_state = bp_enabled;
-  b->next = 0;
-  b->silent = 0;
-  b->ignore_count = 0;
-  b->commands = NULL;
-  b->frame_id = null_frame_id;
-  b->condition_not_parsed = 0;
-  b->py_bp_object = NULL;
   b->related_breakpoint = b;
-  b->location = NULL;
 }
 
 /* Helper to set_raw_breakpoint below.  Creates a breakpoint
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 18b284f..26b0aa5 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -681,45 +681,45 @@ extern int target_exact_watchpoints;
 struct breakpoint
 {
   /* Methods associated with this breakpoint.  */
-  const struct breakpoint_ops *ops;
+  const breakpoint_ops *ops = NULL;
 
-  struct breakpoint *next;
+  breakpoint *next = NULL;
   /* Type of breakpoint.  */
-  enum bptype type;
+  bptype type = bp_none;
   /* Zero means disabled; remember the info but don't break here.  */
-  enum enable_state enable_state;
+  enum enable_state enable_state = bp_enabled;
   /* What to do with this breakpoint after we hit it.  */
-  enum bpdisp disposition;
+  bpdisp disposition = disp_del;
   /* Number assigned to distinguish breakpoints.  */
-  int number;
+  int number = 0;
 
   /* Location(s) associated with this high-level breakpoint.  */
-  struct bp_location *loc;
+  bp_location *loc = NULL;
 
-  /* Non-zero means a silent breakpoint (don't print frame info if we
-     stop here).  */
-  unsigned char silent;
-  /* Non-zero means display ADDR_STRING to the user verbatim.  */
-  unsigned char display_canonical;
+  /* True means a silent breakpoint (don't print frame info if we stop
+     here).  */
+  bool silent = false;
+  /* True means display ADDR_STRING to the user verbatim.  */
+  bool display_canonical = false;
   /* Number of stops at this breakpoint that should be continued
      automatically before really stopping.  */
-  int ignore_count;
+  int ignore_count = 0;
 
   /* Number of stops at this breakpoint before it will be
      disabled.  */
-  int enable_count;
+  int enable_count = 0;
 
   /* Chain of command lines to execute when this breakpoint is
      hit.  */
-  struct counted_command_line *commands;
+  counted_command_line *commands = NULL;
   /* Stack depth (address of frame).  If nonzero, break only if fp
      equals this.  */
-  struct frame_id frame_id;
+  struct frame_id frame_id = null_frame_id;
 
   /* The program space used to set the breakpoint.  This is only set
      for breakpoints which are specific to a program space; for
      non-thread-specific ordinary breakpoints this is NULL.  */
-  struct program_space *pspace;
+  program_space *pspace = NULL;
 
   /* Location we used to set the breakpoint.  */
   event_location_up location;
@@ -727,60 +727,60 @@ struct breakpoint
   /* The filter that should be passed to decode_line_full when
      re-setting this breakpoint.  This may be NULL, but otherwise is
      allocated with xmalloc.  */
-  char *filter;
+  char *filter = NULL;
 
   /* For a ranged breakpoint, the location we used to find the end of
      the range.  */
   event_location_up location_range_end;
 
   /* Architecture we used to set the breakpoint.  */
-  struct gdbarch *gdbarch;
+  struct gdbarch *gdbarch = NULL;
   /* Language we used to set the breakpoint.  */
-  enum language language;
+  enum language language = language_unknown;
   /* Input radix we used to set the breakpoint.  */
-  int input_radix;
+  int input_radix = 0;
   /* String form of the breakpoint condition (malloc'd), or NULL if
      there is no condition.  */
-  char *cond_string;
+  char *cond_string = NULL;
 
   /* String form of extra parameters, or NULL if there are none.
      Malloc'd.  */
-  char *extra_string;
+  char *extra_string = NULL;
 
   /* Holds the address of the related watchpoint_scope breakpoint when
      using watchpoints on local variables (might the concept of a
      related breakpoint be useful elsewhere, if not just call it the
      watchpoint_scope breakpoint or something like that.  FIXME).  */
-  struct breakpoint *related_breakpoint;
+  breakpoint *related_breakpoint = NULL;
 
   /* Thread number for thread-specific breakpoint, or -1 if don't
      care.  */
-  int thread;
+  int thread = -1;
 
   /* Ada task number for task-specific breakpoint, or 0 if don't
      care.  */
-  int task;
+  int task = 0;
 
   /* Count of the number of times this breakpoint was taken, dumped
      with the info, but not used for anything else.  Useful for seeing
      how many times you hit a break prior to the program aborting, so
      you can back up to just before the abort.  */
-  int hit_count;
+  int hit_count = 0;
 
   /* Is breakpoint's condition not yet parsed because we found no
      location initially so had no context to parse the condition
      in.  */
-  int condition_not_parsed;
+  int condition_not_parsed = 0;
 
   /* With a Python scripting enabled GDB, store a reference to the
      Python object that has been associated with this breakpoint.
      This is always NULL for a GDB that is not script enabled.  It can
      sometimes be NULL for enabled GDBs as not all breakpoint types
      are tracked by the scripting language API.  */
-  struct gdbpy_breakpoint_object *py_bp_object;
+  gdbpy_breakpoint_object *py_bp_object = NULL;
 
   /* Same as py_bp_object, but for Scheme.  */
-  struct gdbscm_breakpoint_object *scm_bp_object;
+  gdbscm_breakpoint_object *scm_bp_object = NULL;
 };
 
 /* An instance of this type is used to represent a watchpoint.  It
-- 
2.5.5

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

On 04/20/2017 04:27 AM, Simon Marchi wrote:

>> I'll move this to the end of the series before pushing (if agreed).

(I've moved it to the end of the series before pushing, and removed
that comment from the commit log.)

> That's really nice.  I'm actually surprised we didn't get random crashes
> because of that yet!

Yeah.

>> + struct S {
>> +   S() { m_data.reserve (10); }
>> +   std::vector<foo> m_data;
>> + };
> 
> Here it says struct S ...
> 
>> +
>> +and old code was initializing B objects like this:
>> +
>> + struct B b;
>> + memset (&b, 0, sizeof (B)); // whoops, now wipes vector.
> 
> ... and here struct B?

Bah.  These should of course be S, not B.
Thanks for spotting this!  Below's what I pushed.

From b0b92aeb3828219075fce23543fb39fee8608e99 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Apr 2017 01:27:41 +0100
Subject: [PATCH] Poison non-POD memset & non-trivially-copyable memcpy/memmove
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This patch catches invalid initialization of non-POD types with
memset, at compile time.

This is what I used to catch the problems fixed by the previous
patches in the series:

  $ make -k 2>&1 | grep "deleted function"
  src/gdb/breakpoint.c:951:53: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = bp_location; <template-parameter-1-2> = void; size_t = long unsigned int]’
  src/gdb/breakpoint.c:7325:32: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = bp_location; <template-parameter-1-2> = void; size_t = long unsigned int]’
  src/gdb/btrace.c:1153:42: error: use of deleted function ‘void* memset(T*, int, size_t) [with T = btrace_insn; <template-parameter-1-2> = void; size_t = long unsigned int]’
...

gdb/ChangeLog:
2017-04-25  Pedro Alves  <palves@redhat.com>

	* common/common-defs.h: Include "common/poison.h".
	* common/function-view.h: (Not, Or, Requires): Move to traits.h
	and adjust.
	* common/poison.h: New file.
	* common/traits.h: Include <type_traits>.
	(Not, Or, Requires): New, moved from common/function-view.h.
---
 gdb/ChangeLog              |  9 +++++
 gdb/common/common-defs.h   |  1 +
 gdb/common/function-view.h | 40 +++-------------------
 gdb/common/poison.h        | 83 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/common/traits.h        | 30 ++++++++++++++++-
 5 files changed, 126 insertions(+), 37 deletions(-)
 create mode 100644 gdb/common/poison.h

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index dc321a2..26e6370 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@
 2017-04-25  Pedro Alves  <palves@redhat.com>
 
+	* common/common-defs.h: Include "common/poison.h".
+	* common/function-view.h: (Not, Or, Requires): Move to traits.h
+	and adjust.
+	* common/poison.h: New file.
+	* common/traits.h: Include <type_traits>.
+	(Not, Or, Requires): New, moved from common/function-view.h.
+
+2017-04-25  Pedro Alves  <palves@redhat.com>
+
 	* breakpoint.h (struct breakpoint): In-class initialize all
 	fields.  Make boolean fields "bool".
 	* breakpoint.c (init_raw_breakpoint_without_location): Remove
diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index af37111..439bce6 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -82,6 +82,7 @@
 #include "common-debug.h"
 #include "cleanups.h"
 #include "common-exceptions.h"
+#include "common/poison.h"
 
 #define EXTERN_C extern "C"
 #define EXTERN_C_PUSH extern "C" {
diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h
index 66a691b..de9a634 100644
--- a/gdb/common/function-view.h
+++ b/gdb/common/function-view.h
@@ -153,34 +153,6 @@
 
 namespace gdb {
 
-namespace traits {
-  /* A few trait helpers.  */
-  template<typename Predicate>
-  struct Not : public std::integral_constant<bool, !Predicate::value>
-  {};
-
-  template<typename...>
-  struct Or;
-
-  template<>
-  struct Or<> : public std::false_type
-  {};
-
-  template<typename B1>
-  struct Or<B1> : public B1
-  {};
-
-  template<typename B1, typename B2>
-  struct Or<B1, B2>
-    : public std::conditional<B1::value, B1, B2>::type
-  {};
-
-  template<typename B1,typename B2,typename B3, typename... Bn>
-  struct Or<B1, B2, B3, Bn...>
-    : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type
-  {};
-} /* namespace traits */
-
 namespace fv_detail {
 /* Bits shared by all function_view instantiations that do not depend
    on the template parameters.  */
@@ -209,9 +181,9 @@ class function_view<Res (Args...)>
 {
   template<typename From, typename To>
   using CompatibleReturnType
-    = traits::Or<std::is_void<To>,
-		 std::is_same<From, To>,
-		 std::is_convertible<From, To>>;
+    = Or<std::is_void<To>,
+	 std::is_same<From, To>,
+	 std::is_convertible<From, To>>;
 
   /* True if Func can be called with Args, and either the result is
      Res, convertible to Res or Res is void.  */
@@ -227,10 +199,6 @@ class function_view<Res (Args...)>
     : std::is_same<function_view, typename std::decay<Callable>::type>
   {};
 
-  /* Helper to make SFINAE logic easier to read.  */
-  template<typename Condition>
-  using Requires = typename std::enable_if<Condition::value, void>::type;
-
  public:
 
   /* NULL by default.  */
@@ -248,7 +216,7 @@ class function_view<Res (Args...)>
      compatible.  */
   template
     <typename Callable,
-     typename = Requires<traits::Not<IsFunctionView<Callable>>>,
+     typename = Requires<Not<IsFunctionView<Callable>>>,
      typename = Requires<IsCompatibleCallable<Callable>>>
   function_view (Callable &&callable) noexcept
   {
diff --git a/gdb/common/poison.h b/gdb/common/poison.h
new file mode 100644
index 0000000..a875568
--- /dev/null
+++ b/gdb/common/poison.h
@@ -0,0 +1,83 @@
+/* Poison symbols at compile time.
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_POISON_H
+#define COMMON_POISON_H
+
+#include "traits.h"
+
+/* Poison memset of non-POD types.  The idea is catching invalid
+   initialization of non-POD structs that is easy to be introduced as
+   side effect of refactoring.  For example, say this:
+
+ struct S { VEC(foo_s) *m_data; };
+
+is converted to this at some point:
+
+ struct S {
+   S() { m_data.reserve (10); }
+   std::vector<foo> m_data;
+ };
+
+and old code was initializing S objects like this:
+
+ struct S s;
+ memset (&s, 0, sizeof (S)); // whoops, now wipes vector.
+
+Declaring memset as deleted for non-POD types makes the memset above
+be a compile-time error.  */
+
+/* Helper for SFINAE.  True if "T *" is memsettable.  I.e., if T is
+   either void, or POD.  */
+template<typename T>
+struct IsMemsettable
+  : gdb::Or<std::is_void<T>,
+	    std::is_pod<T>>
+{};
+
+template <typename T,
+	  typename = gdb::Requires<gdb::Not<IsMemsettable<T>>>>
+void *memset (T *s, int c, size_t n) = delete;
+
+/* Similarly, poison memcpy and memmove of non trivially-copyable
+   types, which is undefined.  */
+
+/* True if "T *" is relocatable.  I.e., copyable with memcpy/memmove.
+   I.e., T is either trivially copyable, or void.  */
+template<typename T>
+struct IsRelocatable
+  : gdb::Or<std::is_void<T>,
+	    std::is_trivially_copyable<T>>
+{};
+
+/* True if both source and destination are relocatable.  */
+
+template <typename D, typename S>
+using BothAreRelocatable
+  = gdb::And<IsRelocatable<D>, IsRelocatable<S>>;
+
+template <typename D, typename S,
+	  typename = gdb::Requires<gdb::Not<BothAreRelocatable<D, S>>>>
+void *memcpy (D *dest, const S *src, size_t n) = delete;
+
+template <typename D, typename S,
+	  typename = gdb::Requires<gdb::Not<BothAreRelocatable<D, S>>>>
+void *memmove (D *dest, const S *src, size_t n) = delete;
+
+#endif /* COMMON_POISON_H */
diff --git a/gdb/common/traits.h b/gdb/common/traits.h
index c8f29cc..4f7161b 100644
--- a/gdb/common/traits.h
+++ b/gdb/common/traits.h
@@ -32,7 +32,32 @@ template<typename... Ts>
 using void_t = typename make_void<Ts...>::type;
 
 /* A few trait helpers, mainly stolen from libstdc++.  Uppercase
-   because "and" is a keyword.  */
+   because "and/or", etc. are reserved keywords.  */
+
+template<typename Predicate>
+struct Not : public std::integral_constant<bool, !Predicate::value>
+{};
+
+template<typename...>
+struct Or;
+
+template<>
+struct Or<> : public std::false_type
+{};
+
+template<typename B1>
+struct Or<B1> : public B1
+{};
+
+template<typename B1, typename B2>
+struct Or<B1, B2>
+  : public std::conditional<B1::value, B1, B2>::type
+{};
+
+template<typename B1,typename B2,typename B3, typename... Bn>
+struct Or<B1, B2, B3, Bn...>
+  : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type
+{};
 
 template<typename...>
 struct And;
@@ -55,6 +80,9 @@ struct And<B1, B2, B3, Bn...>
   : public std::conditional<B1::value, And<B2, B3, Bn...>, B1>::type
 {};
 
+/* Concepts-light-like helper to make SFINAE logic easier to read.  */
+template<typename Condition>
+using Requires = typename std::enable_if<Condition::value, void>::type;
 }
 
 #endif /* COMMON_TRAITS_H */
-- 
2.5.5

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

On 04/25/2017 02:14 AM, Pedro Alves wrote:
> Below's what I pushed.

BTW, if this ever reveals problematic in such a way that it'd be desirable
to have a convenient way to disable it, we can make the overloads
trigger warnings instead, so that folks can disable the build errors
with --disable-werror (which is the default on releases).

From 232eda6a5fca7347c6b8b1d553e792803bccba0a Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Apr 2017 00:42:17 +0100
Subject: [PATCH] warnings-instead

---
 gdb/common/common-defs.h |  8 ++++++++
 gdb/common/poison.h      | 12 ++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index 439bce6..d00543b 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -69,6 +69,14 @@
 #undef ATTRIBUTE_PRINTF
 #define ATTRIBUTE_PRINTF _GL_ATTRIBUTE_FORMAT_PRINTF
 
+#ifdef __GNUC__
+/* Introduced in gcc versions 3.1, which is older than our minimum
+   requirement.  */
+# define ATTRIBUTE_DEPRECATED __attribute__ ((__deprecated__))
+#else
+# define ATTRIBUTE_DEPRECATED
+#endif
+
 #include "libiberty.h"
 #include "pathmax.h"
 #include "gdb/signals.h"
diff --git a/gdb/common/poison.h b/gdb/common/poison.h
index a875568..ac65484 100644
--- a/gdb/common/poison.h
+++ b/gdb/common/poison.h
@@ -74,10 +74,18 @@ using BothAreRelocatable
 
 template <typename D, typename S,
 	  typename = gdb::Requires<gdb::Not<BothAreRelocatable<D, S>>>>
-void *memcpy (D *dest, const S *src, size_t n) = delete;
+ATTRIBUTE_DEPRECATED
+void *memcpy (D *dest, const S *src, size_t n)
+{
+  return memcpy ((void *) dest, (void *) src, n);
+}
 
 template <typename D, typename S,
 	  typename = gdb::Requires<gdb::Not<BothAreRelocatable<D, S>>>>
-void *memmove (D *dest, const S *src, size_t n) = delete;
+ATTRIBUTE_DEPRECATED
+void *memmove (D *dest, const S *src, size_t n)
+{
+  return memmove ((void *) dest, (void *) src, n);
+}
 
 #endif /* COMMON_POISON_H */
-- 
2.5.5

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

> +/* True if "T *" is relocatable.  I.e., copyable with memcpy/memmove.
> +   I.e., T is either trivially copyable, or void.  */
> +template<typename T>
> +struct IsRelocatable
> +  : gdb::Or<std::is_void<T>,
> +	    std::is_trivially_copyable<T>>
> +{};

This breaks the build with gcc 4.8,

In file included from ../../binutils-gdb/gdb/common/common-defs.h:85:0,
                 from ../../binutils-gdb/gdb/defs.h:28,
                 from ../../binutils-gdb/gdb/gdb.c:19:
../../binutils-gdb/gdb/common/poison.h:66:6: error: ‘is_trivially_copyable’ is not a member of ‘std’
      std::is_trivially_copyable<T>>
      ^

you probably have already received the buildbot fail message
https://gdb-build.sergiodj.net/builders/Ubuntu-AArch64-m64/builds/1845

is_trivially_copyable is missing on 4.9 too,
https://gcc.gnu.org/onlinedocs/gcc-4.9.4/libstdc++/manual/manual/status.html#status.iso.2011
and it was added in gcc 5, as far as I know.

-- 
Yao (齐尧)

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

On 04/25/2017 09:24 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> Hi Pedro,
> 
>> +/* True if "T *" is relocatable.  I.e., copyable with memcpy/memmove.
>> +   I.e., T is either trivially copyable, or void.  */
>> +template<typename T>
>> +struct IsRelocatable
>> +  : gdb::Or<std::is_void<T>,
>> +	    std::is_trivially_copyable<T>>
>> +{};
> 
> This breaks the build with gcc 4.8,
> 
> In file included from ../../binutils-gdb/gdb/common/common-defs.h:85:0,
>                  from ../../binutils-gdb/gdb/defs.h:28,
>                  from ../../binutils-gdb/gdb/gdb.c:19:
> ../../binutils-gdb/gdb/common/poison.h:66:6: error: ‘is_trivially_copyable’ is not a member of ‘std’
>       std::is_trivially_copyable<T>>
>       ^
> 

Sorry, I thought I had tested gcc 4.8, but clearly I did not.  I'll fix it
as soon as I have a chance, probably by disabling the poisoning on
older compilers.

> you probably have already received the buildbot fail message
> https://gdb-build.sergiodj.net/builders/Ubuntu-AArch64-m64/builds/1845
> 
> is_trivially_copyable is missing on 4.9 too,
> https://gcc.gnu.org/onlinedocs/gcc-4.9.4/libstdc++/manual/manual/status.html#status.iso.2011
> and it was added in gcc 5, as far as I know.

Thanks,
Pedro Alves

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

On 04/25/2017 10:24 AM, Pedro Alves wrote:
> On 04/25/2017 09:24 AM, Yao Qi wrote:
>> Pedro Alves <palves@redhat.com> writes:
>>
>> Hi Pedro,
>>
>>> +/* True if "T *" is relocatable.  I.e., copyable with memcpy/memmove.
>>> +   I.e., T is either trivially copyable, or void.  */
>>> +template<typename T>
>>> +struct IsRelocatable
>>> +  : gdb::Or<std::is_void<T>,
>>> +	    std::is_trivially_copyable<T>>
>>> +{};
>>
>> This breaks the build with gcc 4.8,
>>
>> In file included from ../../binutils-gdb/gdb/common/common-defs.h:85:0,
>>                  from ../../binutils-gdb/gdb/defs.h:28,
>>                  from ../../binutils-gdb/gdb/gdb.c:19:
>> ../../binutils-gdb/gdb/common/poison.h:66:6: error: ‘is_trivially_copyable’ is not a member of ‘std’
>>       std::is_trivially_copyable<T>>
>>       ^
>>
> 
> Sorry, I thought I had tested gcc 4.8, but clearly I did not.  I'll fix it
> as soon as I have a chance, probably by disabling the poisoning on
> older compilers.

Like this.  I went ahead and pushed it.

From debed3db4887483552103da36d180967ef0dca5f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 25 Apr 2017 10:58:57 +0100
Subject: [PATCH] Fix build on gcc < 5 (std::is_trivially_copyable missing)

Ref: https://sourceware.org/ml/gdb-patches/2017-04/msg00660.html

Simply skip the poisoning on older compilers.

gdb/ChangeLog:
2017-04-25  Pedro Alves  <palves@redhat.com>

	* common/poison.h [!HAVE_IS_TRIVIALLY_COPYABLE] (IsRelocatable)
	(BothAreRelocatable, memcopy, memmove): Don't define.
	* common/traits.h (__has_feature, HAVE_IS_TRIVIALLY_COPYABLE): New
	macros.
---
 gdb/ChangeLog       |  7 +++++++
 gdb/common/poison.h |  4 ++++
 gdb/common/traits.h | 13 +++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 26e6370..d1c1942 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2017-04-25  Pedro Alves  <palves@redhat.com>
 
+	* common/poison.h [!HAVE_IS_TRIVIALLY_COPYABLE] (IsRelocatable)
+	(BothAreRelocatable, memcopy, memmove): Don't define.
+	* common/traits.h (__has_feature, HAVE_IS_TRIVIALLY_COPYABLE): New
+	macros.
+
+2017-04-25  Pedro Alves  <palves@redhat.com>
+
 	* common/common-defs.h: Include "common/poison.h".
 	* common/function-view.h: (Not, Or, Requires): Move to traits.h
 	and adjust.
diff --git a/gdb/common/poison.h b/gdb/common/poison.h
index a875568..37dd35e 100644
--- a/gdb/common/poison.h
+++ b/gdb/common/poison.h
@@ -55,6 +55,8 @@ template <typename T,
 	  typename = gdb::Requires<gdb::Not<IsMemsettable<T>>>>
 void *memset (T *s, int c, size_t n) = delete;
 
+#if HAVE_IS_TRIVIALLY_COPYABLE
+
 /* Similarly, poison memcpy and memmove of non trivially-copyable
    types, which is undefined.  */
 
@@ -80,4 +82,6 @@ template <typename D, typename S,
 	  typename = gdb::Requires<gdb::Not<BothAreRelocatable<D, S>>>>
 void *memmove (D *dest, const S *src, size_t n) = delete;
 
+#endif /* HAVE_IS_TRIVIALLY_COPYABLE */
+
 #endif /* COMMON_POISON_H */
diff --git a/gdb/common/traits.h b/gdb/common/traits.h
index 4f7161b..8c41b03 100644
--- a/gdb/common/traits.h
+++ b/gdb/common/traits.h
@@ -20,6 +20,19 @@
 
 #include <type_traits>
 
+/* GCC does not understand __has_feature.  */
+#if !defined(__has_feature)
+# define __has_feature(x) 0
+#endif
+
+/* HAVE_IS_TRIVIALLY_COPYABLE is defined as 1 iff
+   std::is_trivially_copyable is available.  GCC only implemented it
+   in GCC 5.  */
+#if (__has_feature(is_trivially_copyable) \
+     || (defined __GNUC__ && __GNUC__ >= 5))
+# define HAVE_IS_TRIVIALLY_COPYABLE 1
+#endif
+
 namespace gdb {
 
 /* Pre C++14-safe (CWG 1558) version of C++17's std::void_t.  See
-- 
2.5.5

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

Hi Simon,

Sorry for the delay.  Finally managed to get back to this.

On 04/24/2017 02:12 AM, Simon Marchi wrote:
> On 2017-04-12 22:27, Pedro Alves wrote:
>> This patch catches invalid initialization of non-POD types with
>> memset, at compile time.
> 
> Would it be possible to do something similar but to catch uses of
> XNEW/XCNEW with types that need new?  XNEW is defined as:
> 
> #define XNEW(T) ((T *) xmalloc (sizeof (T)))
> 
> I just tried this, and it seems to work well:
> 
> #define assert_pod(T) static_assert(std::is_pod<T>::value)
> 
> #undef XNEW
> #define XNEW(T) ({ assert_pod(T); (T *) xmalloc (sizeof (T)); })
> #undef XCNEW
> #define XCNEW(T)  ({ assert_pod(T); (T *) xcalloc (1, sizeof (T)); })
> 
> assuming the compiler knows about statement expressions.

I think that that's a great idea!  I tried that locally and see that
this already catches two bad cases (btrace_function and objfile).

We don't need to use non-standard statement expressions though.
Function templates should work just as well here:

template<typename T>
T *xnew ()
{
  static_assert (std::is_pod<T>::value, "use operator new instead");
  return (T *) xmalloc (sizeof (T));
}

template<typename T>
T *xcnew ()
{
  static_assert (std::is_pod<T>::value, "use operator new instead");
  return (T *) xcalloc (1, sizeof (T));
}

#undef XNEW
#define XNEW(T) xnew<T>()
#undef XCNEW
#define XCNEW(T) xcnew<T>()

As should lambdas:

#undef XNEW
#define XNEW(T) [] () -> T *						\
  {									\
    static_assert (std::is_pod<T>::value, "use operator new instead");	\
    return (T *) xmalloc (sizeof (T));					\
  } ()

#undef XCNEW
#define XCNEW(T) [] () -> T *						\
  {									\
    static_assert (std::is_pod<T>::value, "use operator new instead");	\
    return (T *) xcalloc (1, sizeof (T));				\
  } ()

I think the template version is likely a little bit easier
to understand and debug (e.g., easy to put a breakpoint on the function
template, not so easy to put a breakpoint on a lambda).  I'd just
confirm that the template/lambda is completely optimized out on an
optimized build (e.g., compare out of "$ size gdb" before and after
patch).

Thanks,
Pedro Alves

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

On 04/24/2017 02:53 AM, Simon Marchi wrote:
> On 2017-04-23 21:12, Simon Marchi wrote:
>> On 2017-04-12 22:27, Pedro Alves wrote:
>>> This patch catches invalid initialization of non-POD types with
>>> memset, at compile time.
>>
>> Would it be possible to do something similar but to catch uses of
>> XNEW/XCNEW with types that need new?  XNEW is defined as:
>>
>> #define XNEW(T) ((T *) xmalloc (sizeof (T)))
>>
>> I just tried this, and it seems to work well:
>>
>> #define assert_pod(T) static_assert(std::is_pod<T>::value)
>>
>> #undef XNEW
>> #define XNEW(T) ({ assert_pod(T); (T *) xmalloc (sizeof (T)); })
>> #undef XCNEW
>> #define XCNEW(T)  ({ assert_pod(T); (T *) xcalloc (1, sizeof (T)); })
>>
>> assuming the compiler knows about statement expressions.
> 
> Actually, it should probably use std::is_trivially_constructible.  
> And I
> suppose we could do the same with xfree, delete it when
> !std::is_trivially_destructible.


I think you wanted std::is_trivially_default_constructible
for XNEW.  I think that we want _both_ conditions (*constructible
and *destructible) on both XNEW and xfree.  For example, it'll be
good to catch the mismatching new/delete that could sneak in otherwise:

 // type with trivial constructor
 struct A
 {
   // A() = default;
   ~A() { /* do something with side effects */ } // not trivial
 };
  
 // type with trivial destructor
 struct B
 {
   B() { /* do something with side effects */ } // not trivial
   //~B() = default;
 };
 
 void foo ()
 {
   A *a = XNEW (struct A);
   delete a;
   B *b = new B;
   xfree (b);
 }

Calling delete on a pointer not allocated with new is undefined behavior.
These mismatches are also flagged by -fsanitize=address, but
making them compile-time errors would be even better.

This wouldn't catch allocating types that are both trivially
default constructible and trivially destructible, and which _also_
have non-default ctors, like this, for example:

 struct C
 {
   C() = default;
   explicit C(int) { /* some side effects */ }
 };

 static_assert (std::is_trivially_default_constructible<C>::value, "");
 static_assert (std::is_trivially_destructible<C>::value, "");

 C *b = new C(1);
 xfree (b); // whoops, technically undefined.  -fsanitify=address likely complains.

but std::is_pod wouldn't either.

If we make a type non-standard-layout, then it no longer is POD:

 struct D
 {
  // Mix of public/private fields => not POD
 public:
   int a;
 private:
   int b;
 };

This (D) case is likely to not really be problematic in practice WRT
to allocation/deallocation with malloc/free, but it still feels
like a code smell to me.  I'd be willing to try forcing use
of new/delete for these types too.  This would suggest using the
bigger std::is_pod hammer in XNEW/xfree instead of just
std::is_trivially_*ctible.  But I'd understand if others disagree.

Thanks,
Pedro Alves

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Simon Marchi]

On 2017-04-27 09:58, Pedro Alves wrote:
> On 04/24/2017 02:53 AM, Simon Marchi wrote:
>> Actually, it should probably use std::is_trivially_constructible.
>> And I
>> suppose we could do the same with xfree, delete it when
>> !std::is_trivially_destructible.
> 
> 
> I think you wanted std::is_trivially_default_constructible
> for XNEW.

 From what I understand, using is_trivially_default_constructible<T> is 
the same as is_trivially_constructible<T>.  We can of course use 
is_trivially_default_constructible if it's clearer.

> I think that we want _both_ conditions (*constructible
> and *destructible) on both XNEW and xfree.  For example, it'll be
> good to catch the mismatching new/delete that could sneak in otherwise:

That seems reasonnable.  We want to "upgrade" to new and delete in a 
lock step anyway.

>  // type with trivial constructor
>  struct A
>  {
>    // A() = default;
>    ~A() { /* do something with side effects */ } // not trivial
>  };
> 
>  // type with trivial destructor
>  struct B
>  {
>    B() { /* do something with side effects */ } // not trivial
>    //~B() = default;
>  };
> 
>  void foo ()
>  {
>    A *a = XNEW (struct A);
>    delete a;
>    B *b = new B;
>    xfree (b);
>  }
> 
> Calling delete on a pointer not allocated with new is undefined 
> behavior.
> These mismatches are also flagged by -fsanitize=address, but
> making them compile-time errors would be even better.
> 
> This wouldn't catch allocating types that are both trivially
> default constructible and trivially destructible, and which _also_
> have non-default ctors, like this, for example:
> 
>  struct C
>  {
>    C() = default;
>    explicit C(int) { /* some side effects */ }
>  };
> 
>  static_assert (std::is_trivially_default_constructible<C>::value, "");
>  static_assert (std::is_trivially_destructible<C>::value, "");
> 
>  C *b = new C(1);
>  xfree (b); // whoops, technically undefined.  -fsanitify=address
> likely complains.
> 
> but std::is_pod wouldn't either.
> 
> If we make a type non-standard-layout, then it no longer is POD:
> 
>  struct D
>  {
>   // Mix of public/private fields => not POD
>  public:
>    int a;
>  private:
>    int b;
>  };
> 
> This (D) case is likely to not really be problematic in practice WRT
> to allocation/deallocation with malloc/free, but it still feels
> like a code smell to me.  I'd be willing to try forcing use
> of new/delete for these types too.  This would suggest using the
> bigger std::is_pod hammer in XNEW/xfree instead of just
> std::is_trivially_*ctible.  But I'd understand if others disagree.

I think it would be a good guideline to use new/delete for types that 
have some C++-related stuff in them, even if it's not technically 
necessary.

Note that this won't be bulletproof also because at many places xfree is 
used on a void pointer, so we don't know what we're really free'ing.  In 
some other cases, objects are freed using a pointer to their "C base 
class".

Simon

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

On 04/30/2017 02:51 AM, Simon Marchi wrote:

> I think it would be a good guideline to use new/delete for types that
> have some C++-related stuff in them, even if it's not technically
> necessary.
> 
> Note that this won't be bulletproof also because at many places xfree is
> used on a void pointer, so we don't know what we're really free'ing.  In
> some other cases, objects are freed using a pointer to their "C base
> class".

Yeah.  Still, better than nothing.

BTW, GCC ran into similar issues almost at the same time
we started discussing this, and I've been discussing
with the GCC folks about a new GCC warning that flags invalid
memcpy/memset misuses.  Martin Sebor has been working on a patch
and it's getting close to be merged, AFAICT.

See:
 https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01527.html
First version of the GCC patch here:
 https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01571.html
Discussion crossed month boundary here:
 https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00925.html
Latest patch is here:
 https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00976.html

I won't be a full replacement since we'll still want our
poisoning for other functions (xmalloc, xfree, etc.).  And
then there's current/older gccs.  But still, pretty neat, IMO.

Thanks,
Pedro Alves

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Simon Marchi]

On 2017-05-17 07:35, Pedro Alves wrote:
> On 04/30/2017 02:51 AM, Simon Marchi wrote:
> 
>> I think it would be a good guideline to use new/delete for types that
>> have some C++-related stuff in them, even if it's not technically
>> necessary.
>> 
>> Note that this won't be bulletproof also because at many places xfree 
>> is
>> used on a void pointer, so we don't know what we're really free'ing.  
>> In
>> some other cases, objects are freed using a pointer to their "C base
>> class".
> 
> Yeah.  Still, better than nothing.
> 
> BTW, GCC ran into similar issues almost at the same time
> we started discussing this, and I've been discussing
> with the GCC folks about a new GCC warning that flags invalid
> memcpy/memset misuses.  Martin Sebor has been working on a patch
> and it's getting close to be merged, AFAICT.
> 
> See:
>  https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01527.html
> First version of the GCC patch here:
>  https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01571.html
> Discussion crossed month boundary here:
>  https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00925.html
> Latest patch is here:
>  https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00976.html
> 
> I won't be a full replacement since we'll still want our
> poisoning for other functions (xmalloc, xfree, etc.).  And
> then there's current/older gccs.  But still, pretty neat, IMO.

Thanks for the info!

I have a branch in progress about poisoning XNEW and friends:
https://github.com/simark/binutils-gdb/commits/poison-xnew

I won't have time to look at it until at least next week, if anybody 
wants to pick it up, they are free to do so.

Simon

Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove

[Pedro Alves]

On 05/17/2017 02:11 PM, Simon Marchi wrote:

> I have a branch in progress about poisoning XNEW and friends:
> https://github.com/simark/binutils-gdb/commits/poison-xnew

Nice!

> 
> I won't have time to look at it until at least next week, if anybody
> wants to pick it up, they are free to do so.

I won't have time either, so I'll just wait.  :-)

Thanks,
Pedro Alves