* [PATCH 0/4] Poison non-POD memset & non-trivially-copyable memcpy/memmove
@ 2017-04-13 2:27 Pedro Alves
2017-04-13 2:28 ` [PATCH 4/5] Don't memset non-POD types: struct btrace_insn Pedro Alves
` (4 more replies)
0 siblings, 5 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-13 2:27 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable
2017-04-13 2:27 [PATCH 0/4] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
2017-04-13 2:28 ` [PATCH 4/5] Don't memset non-POD types: struct btrace_insn Pedro Alves
2017-04-13 2:28 ` [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
@ 2017-04-13 2:28 ` Pedro Alves
2017-04-20 3:34 ` Simon Marchi
2017-04-13 2:28 ` [PATCH 3/5] Don't memset non-POD types: struct bp_location Pedro Alves
2017-04-13 2:35 ` [PATCH 5/5] Don't memset non-POD types: struct breakpoint Pedro Alves
4 siblings, 1 reply; 26+ messages in thread
From: Pedro Alves @ 2017-04-13 2:28 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/5] Don't memset non-POD types: struct bp_location
2017-04-13 2:27 [PATCH 0/4] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
` (2 preceding siblings ...)
2017-04-13 2:28 ` [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable Pedro Alves
@ 2017-04-13 2:28 ` Pedro Alves
2017-04-13 2:35 ` [PATCH 5/5] Don't memset non-POD types: struct breakpoint Pedro Alves
4 siblings, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-13 2:28 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-13 2:27 [PATCH 0/4] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
2017-04-13 2:28 ` [PATCH 4/5] Don't memset non-POD types: struct btrace_insn Pedro Alves
@ 2017-04-13 2:28 ` Pedro Alves
2017-04-20 3:27 ` Simon Marchi
2017-04-24 1:12 ` Simon Marchi
2017-04-13 2:28 ` [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable Pedro Alves
` (2 subsequent siblings)
4 siblings, 2 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-13 2:28 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/5] Don't memset non-POD types: struct btrace_insn
2017-04-13 2:27 [PATCH 0/4] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
@ 2017-04-13 2:28 ` Pedro Alves
2017-04-13 7:57 ` Metzger, Markus T
2017-04-13 2:28 ` [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
` (3 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Pedro Alves @ 2017-04-13 2:28 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/5] Don't memset non-POD types: struct breakpoint
2017-04-13 2:27 [PATCH 0/4] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
` (3 preceding siblings ...)
2017-04-13 2:28 ` [PATCH 3/5] Don't memset non-POD types: struct bp_location Pedro Alves
@ 2017-04-13 2:35 ` Pedro Alves
2017-04-20 4:00 ` Simon Marchi
4 siblings, 1 reply; 26+ messages in thread
From: Pedro Alves @ 2017-04-13 2:35 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 4/5] Don't memset non-POD types: struct btrace_insn
2017-04-13 2:28 ` [PATCH 4/5] Don't memset non-POD types: struct btrace_insn Pedro Alves
@ 2017-04-13 7:57 ` Metzger, Markus T
2017-04-25 1:11 ` Pedro Alves
0 siblings, 1 reply; 26+ messages in thread
From: Metzger, Markus T @ 2017-04-13 7:57 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
> -----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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-13 2:28 ` [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
@ 2017-04-20 3:27 ` Simon Marchi
2017-04-25 1:14 ` Pedro Alves
2017-04-24 1:12 ` Simon Marchi
1 sibling, 1 reply; 26+ messages in thread
From: Simon Marchi @ 2017-04-20 3:27 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable
2017-04-13 2:28 ` [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable Pedro Alves
@ 2017-04-20 3:34 ` Simon Marchi
2017-04-25 1:10 ` Pedro Alves
0 siblings, 1 reply; 26+ messages in thread
From: Simon Marchi @ 2017-04-20 3:34 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] Don't memset non-POD types: struct breakpoint
2017-04-13 2:35 ` [PATCH 5/5] Don't memset non-POD types: struct breakpoint Pedro Alves
@ 2017-04-20 4:00 ` Simon Marchi
2017-04-25 1:11 ` Pedro Alves
0 siblings, 1 reply; 26+ messages in thread
From: Simon Marchi @ 2017-04-20 4:00 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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;
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-13 2:28 ` [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
2017-04-20 3:27 ` Simon Marchi
@ 2017-04-24 1:12 ` Simon Marchi
2017-04-24 1:53 ` Simon Marchi
2017-04-27 13:57 ` Pedro Alves
1 sibling, 2 replies; 26+ messages in thread
From: Simon Marchi @ 2017-04-24 1:12 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-24 1:12 ` Simon Marchi
@ 2017-04-24 1:53 ` Simon Marchi
2017-04-27 13:58 ` Pedro Alves
2017-04-27 13:57 ` Pedro Alves
1 sibling, 1 reply; 26+ messages in thread
From: Simon Marchi @ 2017-04-24 1:53 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable
2017-04-20 3:34 ` Simon Marchi
@ 2017-04-25 1:10 ` Pedro Alves
0 siblings, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-25 1:10 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/5] Don't memset non-POD types: struct btrace_insn
2017-04-13 7:57 ` Metzger, Markus T
@ 2017-04-25 1:11 ` Pedro Alves
0 siblings, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-25 1:11 UTC (permalink / raw)
To: Metzger, Markus T, gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] Don't memset non-POD types: struct breakpoint
2017-04-20 4:00 ` Simon Marchi
@ 2017-04-25 1:11 ` Pedro Alves
0 siblings, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-25 1:11 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-20 3:27 ` Simon Marchi
@ 2017-04-25 1:14 ` Pedro Alves
2017-04-25 1:19 ` Pedro Alves
2017-04-25 8:24 ` Yao Qi
0 siblings, 2 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-25 1:14 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-25 1:14 ` Pedro Alves
@ 2017-04-25 1:19 ` Pedro Alves
2017-04-25 8:24 ` Yao Qi
1 sibling, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-25 1:19 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-25 1:14 ` Pedro Alves
2017-04-25 1:19 ` Pedro Alves
@ 2017-04-25 8:24 ` Yao Qi
2017-04-25 9:24 ` Pedro Alves
1 sibling, 1 reply; 26+ messages in thread
From: Yao Qi @ 2017-04-25 8:24 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
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 (齐尧)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-25 8:24 ` Yao Qi
@ 2017-04-25 9:24 ` Pedro Alves
2017-04-25 10:02 ` Pedro Alves
0 siblings, 1 reply; 26+ messages in thread
From: Pedro Alves @ 2017-04-25 9:24 UTC (permalink / raw)
To: Yao Qi; +Cc: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-25 9:24 ` Pedro Alves
@ 2017-04-25 10:02 ` Pedro Alves
0 siblings, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-25 10:02 UTC (permalink / raw)
To: Yao Qi; +Cc: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-24 1:12 ` Simon Marchi
2017-04-24 1:53 ` Simon Marchi
@ 2017-04-27 13:57 ` Pedro Alves
1 sibling, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2017-04-27 13:57 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-24 1:53 ` Simon Marchi
@ 2017-04-27 13:58 ` Pedro Alves
2017-04-30 1:51 ` Simon Marchi
0 siblings, 1 reply; 26+ messages in thread
From: Pedro Alves @ 2017-04-27 13:58 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-27 13:58 ` Pedro Alves
@ 2017-04-30 1:51 ` Simon Marchi
2017-05-17 11:35 ` Pedro Alves
0 siblings, 1 reply; 26+ messages in thread
From: Simon Marchi @ 2017-04-30 1:51 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-04-30 1:51 ` Simon Marchi
@ 2017-05-17 11:35 ` Pedro Alves
2017-05-17 13:11 ` Simon Marchi
0 siblings, 1 reply; 26+ messages in thread
From: Pedro Alves @ 2017-05-17 11:35 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-05-17 11:35 ` Pedro Alves
@ 2017-05-17 13:11 ` Simon Marchi
2017-05-17 13:20 ` Pedro Alves
0 siblings, 1 reply; 26+ messages in thread
From: Simon Marchi @ 2017-05-17 13:11 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove
2017-05-17 13:11 ` Simon Marchi
@ 2017-05-17 13:20 ` Pedro Alves
0 siblings, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2017-05-17 13:20 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-05-17 13:20 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-13 2:27 [PATCH 0/4] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
2017-04-13 2:28 ` [PATCH 4/5] Don't memset non-POD types: struct btrace_insn Pedro Alves
2017-04-13 7:57 ` Metzger, Markus T
2017-04-25 1:11 ` Pedro Alves
2017-04-13 2:28 ` [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove Pedro Alves
2017-04-20 3:27 ` Simon Marchi
2017-04-25 1:14 ` Pedro Alves
2017-04-25 1:19 ` Pedro Alves
2017-04-25 8:24 ` Yao Qi
2017-04-25 9:24 ` Pedro Alves
2017-04-25 10:02 ` Pedro Alves
2017-04-24 1:12 ` Simon Marchi
2017-04-24 1:53 ` Simon Marchi
2017-04-27 13:58 ` Pedro Alves
2017-04-30 1:51 ` Simon Marchi
2017-05-17 11:35 ` Pedro Alves
2017-05-17 13:11 ` Simon Marchi
2017-05-17 13:20 ` Pedro Alves
2017-04-27 13:57 ` Pedro Alves
2017-04-13 2:28 ` [PATCH 2/5] Don't memcpy non-trivially-copyable types: Make enum_flags triv. copyable Pedro Alves
2017-04-20 3:34 ` Simon Marchi
2017-04-25 1:10 ` Pedro Alves
2017-04-13 2:28 ` [PATCH 3/5] Don't memset non-POD types: struct bp_location Pedro Alves
2017-04-13 2:35 ` [PATCH 5/5] Don't memset non-POD types: struct breakpoint Pedro Alves
2017-04-20 4:00 ` Simon Marchi
2017-04-25 1:11 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).